Add a flex label to the end of the info bar, with a separator between dimensions and the flex label.

Idea 1: We can start with text that just says 'flex' to match the Markup badges.

Idea 2: Since the new combined highlighter shows a different design for Flex Containers vs Flex Items, I think it would help folks' understanding if we label it differently for each time of element.

This gets pretty long, but matches the panel names:

Flex Container
Flex Item
Flex Container/Item

This might need to wait for a taller infobar design, however I'd love see how it looks with the current 1-line infobar as well.

Ah also I recommend Gray 40 for the flex label to give it some contrast from the dimensions color.

Hi,

I started to play with this bug.
Here how it looks.

Regards,
Thomas

Thanks Thomas - this looks good to me!

Thank you Victoria!
Would you see different versions (as with the "flex-container/item" wording maybe ?)

Gabriel, could you assign me the bug ?
Also I used parentFlexElement (for the item) and getComputedStyle("display")from this.win for the parent.
Is it the way to go ?

(In reply to Thomas from comment #4)

Thank you Victoria!
Would you see different versions (as with the "flex-container/item" wording maybe ?)

Gabriel, could you assign me the bug ?
Also I used parentFlexElement (for the item) and getComputedStyle("display")from this.win for the parent.
Is it the way to go ?

Hi Thomas, thank you for working on this!

getComputedStyle("display") is definitely the way to go. I am gonna link you to our current code which figures out whether or not an element is a flex container/item since there may be other edge cases to consider.

get displayType() does getComputedStyle("display") essentially. https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js#229

parentFlexElement will definitely get you the flex item, but you will want to make sure that text nodes are also properly identified as flex items. https://searchfox.org/mozilla-central/source/devtools/server/actors/layout.js#333

I was talking to Thomas just now on Slack. So for the sake of clarification, here's what I said there:

/devtools/server/actors/highlighters/box-model.js is the right place to add the code that displays the type of element being highlighted.

The first thing we need to decide is what the labels look like.
Just flex, or the more complete version of Flex Container and Flex Item depending on the type of element?
And what about if the element is both an item and a container?
The GIF attached in comment 2 looks good to me, but I'd like Victoria to take a decision here.

The second thing is about grid. It feels to me like if we do this for flex, we might as well do it for grid, since this is so similar. And it would be more useful and consistent to people.
Victoria: any problem with us adding similar labels for grids too?

Now, technically speaking, here are the APIs that we should be able to use from the box-model.js file to detect the type of nodes we are highlighting:

• node.parentFlexElement tells you whether node is a flex item. It will be falsy if not.
• node.getAsFlexContainer() tells you whether node is a flex container. Same here, it will be falsy if not.
• node.getGridFragments() returns a non-empty array if node is a grid container.

These 3 APIs account for all edge cases that are a bit harder to test for. So you can simply use them and be certain that they return the right info in all cases.

There's one more case though: checking for grid items.
This isn’t as easy though, you need to check if the element's parent is a grid: node.parentNode.getGridFragments().
But that won’t work if the parent is display:contents. In that case you would need to walk up to the parent’s parent, and so on.

There's a bit of JS code we use somewhere else in devtools for this. It would be great to use the same code here and avoid duplication, so we minimize the places where bugs can creep in.
This code is the findGridParentContainerForNode function. If you search for it in the source code, you'll find it. It's exported on the module it currently is in, so you should be able to just import it in box-model.js and use it from there with minimal work.

Re: Flex wording: I just realized that formatting it like 'flex-item' and 'flex-container' makes it look like they're CSS keywords. I think it would be a bit more clear to say Flex Item, Flex Container, and Flex Container/Item.

And yes, would be awesome to do this for Grid as well!

Here the result Victoria with the new wording

Grid Container as a Flex Item proposal

These screenshots look great Thomas. Once you are happy with your code changes to do this, feel free to attach a patch here (via phabricator if you know how to use it: https://docs.firefox-dev.tools/contributing/code-reviews-setup.html) and set either me or :gl as reviewers on it.

These look awesome! Thanks Thomas for working on this!

Thank you!

@Patrick: yes, I will soon, I just need to determine where I put the tests, if you could give me an hint.
I see tests for the infobar, but it is on the "client" side of tests.

What you are seeing is correct, the highlighter tests do live on the client-side of devtools.
You should make sure the test named devtools/client/inspector/test/browser_inspector_infobar_*.js still pass with your changes. You should also one more test to this collection to specifically test flex and grid containers/items.

Indicate in the infobar hilighter if the element is of kind grid or flex, and if it is a container or an item.

Patrick, Gabriel : as I said in the "Test" part I did not succeed to get the text node from the grid container with the walker to test it (I used a snippet I saw somewhere else in the tests).

Let me mark this as leave-open since this won't fix this bug. It's just a simplification that will make Thomas' patch easier.

https://hg.mozilla.org/mozilla-central/rev/1d79999a70f3

Bug 1521188 - Use findGridParentContainerForNode function to assert grid item. r=pbro,r=gl

Hi Thomas, I’m wondering why there are 2 review requests for this now in phabricator.
I see https://phabricator.services.mozilla.com/D29964 and https://phabricator.services.mozilla.com/D33403 and they are slightly different.
Which one do you need me to review?

Hi Patrick, as I said in phabricator the new diff is a mistake. Arc created a new one because I did not use --update. You can review the D29964.

https://hg.mozilla.org/mozilla-central/rev/c99100c3a71f

Adding @ccomorasu, for verification.

Added content about adding flex information to the infobar to How to Examine Flex items In the infobar

Also added a note to the Firefox 69 release notes.

If I missed something, please let me know.

This looks great, thank you Irene!

I can confirm the fix, I verified using Fx 69.0b15.