Flexbox - indicate flex containers and items in the picker infobar

RESOLVED FIXED in Firefox 69

Status

enhancement
P3
normal
RESOLVED FIXED
5 months ago
14 days ago

People

(Reporter: victoria, Assigned: tgerard79, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 69
Dependency tree / graph

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(6 attachments)

Reporter

Description

5 months ago

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.

Reporter

Comment 1

5 months ago

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

Blocks: dt-flexbox
Priority: -- → P3
Mentor: gl
Assignee

Comment 2

2 months ago

Hi,

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

Regards,
Thomas

Attachment #9059669 - Flags: feedback+
Reporter

Comment 3

2 months ago

Thanks Thomas - this looks good to me!

Assignee

Comment 4

2 months ago

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

Assignee: nobody → tgerard79
Status: NEW → ASSIGNED

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.

Flags: needinfo?(victoria)
Reporter

Comment 7

2 months ago

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!

Flags: needinfo?(victoria)
Assignee

Comment 8

2 months ago

Here the result Victoria with the new wording

Attachment #9061601 - Flags: feedback+
Assignee

Comment 9

2 months ago

Grid Container as a Flex Item proposal

Attachment #9061635 - Flags: feedback+

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.

Reporter

Comment 11

2 months ago

These look awesome! Thanks Thomas for working on this!

Assignee

Comment 12

2 months ago

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.

Assignee

Comment 14

2 months ago

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

Assignee

Comment 15

2 months ago

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).

Comment 17

Last month
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d79999a70f3
Make findGridParentContainerForNode not depend on the walker; r=gl

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.

Keywords: leave-open
Assignee

Comment 20

23 days ago

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

Attachment #9069175 - Attachment description: Address revision issues → Address revision issues (duplicate of D29964#988422)

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?

Flags: needinfo?(tgerard79)
Assignee

Comment 22

20 days ago

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.

Flags: needinfo?(tgerard79)

Comment 23

14 days ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c99100c3a71f
Indicate grid/flex container/item in infobar highlighter. r=pbro

Comment 24

14 days ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 14 days ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
You need to log in before you can comment on or make changes to this bug.