Open Bug 1509104 Opened Last year Updated 3 months ago

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

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(Not tracked)

REOPENED

People

(Reporter: miker, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached image 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.
(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.
(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.
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → WORKSFORME
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 :-)
Flags: needinfo?(victoria)
Blocks: 1470379
Status: RESOLVED → REOPENED
Priority: P2 → P3
Resolution: WORKSFORME → ---
Summary: Google input box is displayed as both a flex item and flex container → Google input box, being both a flex item and flex container, makes the highlighter look funky
For context, this is how we draw an empty flex container.
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
Flags: needinfo?(victoria)
(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 ;)
(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.
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)
Summary: Google input box, being both a flex item and flex container, makes the highlighter look funky → Google input box looks funky in flex highlighter (generally, elements that have "display:flex" but don't generate flex container are treated as if they did)
Blocks: dt-flexbox
No longer blocks: 1470379

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.

Summary: Google input box looks funky in flex highlighter (generally, elements that have "display:flex" but don't generate flex container are treated as if they did) → Don't display grid and flex info in the markup-view, rule-view, layout-view when selected elements aren't grid or flex containers

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

Assignee: mratcliffe → nobody
You need to log in before you can comment on or make changes to this bug.