1509104 - Don't display grid and flex info in the markup-view, rule-view, layout-view when selected elements aren't grid or flex containers

Status: REOPENED

(DevTools :: Inspector, defect, P3)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Google input box.png

The Google input box is displayed as both a flex item and flex container... it's an input box so it can't be a container.

Comment 1

[Patrick Brosset <:pbro>]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #0)
> it's an input box so it can't be a container.
Why can't it be a container though? The display:flex declaration seems to be set on it, and the computed style confirms that it is indeed applied.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset <:pbro> from comment #1)
> (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from
> comment #0)
> > it's an input box so it can't be a container.
> Why can't it be a container though? The display:flex declaration seems to be
> set on it, and the computed style confirms that it is indeed applied.

Martin reported that the highlighting looks funky but it is correct if the input box is an empty flex container.

This bug was just about checking that we are doing everything correctly and we are.

Comment 3

[Martin Balfanz [:mbalfanz]]

While it might be correct that the input field is both a container and a flex item, the highlighter still looks funky.  To me, this is an UX issue and a case that we should try to address with a different visual language.  I'd really like Victoria's take on this :-)

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Empty flex container.png

For context, this is how we draw an empty flex container.

Comment 5

[Victoria Wang [:victoria]]

I'm confused why the highlight in the google screenshot looks different from the usual empty flex container highlight (with thinner outline and diagonal pattern going in the other direction).

...I do see lots of interestingly confusing flexboxes on google.com, plus a use of <center> :D

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Victoria Wang [:victoria] from comment #5)
> I'm confused why the highlight in the google screenshot looks different from
> the usual empty flex container highlight (with thinner outline and diagonal
> pattern going in the other direction).
> 
> ...I do see lots of interestingly confusing flexboxes on google.com, plus a
> use of <center> :D

To be honest I think I used an external monitor for one screenshot and my mac display for the other, which explains the difference.

The use of display:flex on an input box isn't really valid... it just acts like display:block but we highlight them 'cos we are nice ;)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #6)
> The use of display:flex on an input box isn't really valid... it just acts
> like display:block but we highlight them 'cos we are nice ;)

This is spot on.  The <input> box is *not* a flex container -- some elements (like input) always get the same box type regardless of their styling, and <input> is one of those elements.

Can we test whether the GetAsFlexContainer() API returns anything before we draw the overlay etc?  That should be a more thorough check than just testing the computed style.

Note also that we don't have this problem [or not as much] for *grid *devtools right now. Compare these two testcases:
 (1)  data:text/html,<input style="display:grid">
 (2)  data:text/html,<input style="display:flex">

For the first one (grid), we show the grid badge in the DOM view and the waffle icon in specified style, *but* the Grid devtools panel correctly still says "CSS Grid is not in use on this page".  In contrast, for the flex example, we populate devtools with information to say it's a flex container with no flex items.  These should probably be consistent, one way or the other.

Comment 8

[Daniel Holbert [:dholbert]]

I suspect GetAsFlexContainer might also return null if we're inside of a display:none subtree.  So if we condition on whether that API returns null, then we'll turn stuff off for display:none subtrees. And that's probably fine...?

Sample that triggers that scenario:
 data:text/html,<div style="display:none"><div style="display:flex;flex-flow:column wrap"><div>abc

(Right now we show a flex panel for this ^^ example, but with no flex items.  Just like in comment 7, we behave differently for grid -- we say "CSS Grid is not in use on this page", which I think is kinda more accurate, in terms of box geometry.

This is kind of the same scenario as <input>, in the sense that it's an element where we have "display:flex" styling (or display:grid styling) but where no actual flex container box gets generated. (And where GetAsFlexContainer probably returns null)

Comment 9

[Patrick Brosset <:pbro>]

Thanks Daniel.

I think using GetContainerAsFlex to guard against displaying this information is the right thing to do.
And we should do this to avoid showing info in the layout sidebar, but also avoid showing the icon in the CSS rule-view, and the badge in the markup-view.
I also think we should do exactly the same thing for grid containers (using the GetGridFragments API).

So I'm re-titling this bug to that effect.

I think one part of the fix is to change this function:
https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/devtools/server/actors/inspector/node.js#223-250
right now it only uses elements' computed styles to return their display types. In case that computed display value is grid/inline-grid/flex/inline-flex, then I think we should double-check by calling GetContainerAsFlex or GetGridFragments before returning those values.

Now, for the CSS rule-view, it's a little more complex, because today we just display the flex and grid icons based on the properties we see in the rule. If display:grid is found, then we display the grid icon next to it. Now if the element isn't actually a grid, clicking on the icon won't do anything, so it's not too big a deal. But still, it'd be nice to avoid showing the icon, or maybe show a warning message?
That'd be a more complex project, I think, maybe better as its own bug.

Comment 10

[Victoria Wang [:victoria]]

Attachment: non-flexbox with flex badges/buttons

Another example: universalNav on https://www.nytimes.com/subscription.html

Comment 11

[Daniel Holbert [:dholbert]]

Victoria's comment 10 example (universalNav on nytimes) is actually really a flex container, and we're accurately labeling/overlaying it as far as I can tell. (So, it's not an example of this bug.)

It's just a trivial flex container, with one zero sized flex item, one nonzero flex item that takes up all the space, and one abspos child (not a flex item).

(Its first child, the zero-sized flex item with class="universalNav__logo", has no in-flow content of its own. It only contains an abspos thing, which doesn't contribute to its content size. Hence, that first flex item ends up happily 0-sized.)

(EDIT: Ah, per slack conversation, it seems there's a subtle issue shown in victoria's screenshot -- the "flex" pane on the right side isn't populated! But neither she nor I can reproduce that issue right now for this element. Anyway, if we figure out how to make that happen reliably, that's probably a different bug from this bug here, since in this nytimes case, the element is indeed a flex container and should be getting its sidebar populated.)

Comment 12

[Daniel Holbert [:dholbert]]

Ah, Victoria's example is really bug 1518943 (just filed). So, disregard comment 10 and comment 11 for the purposes of this bug here.