1498669 - flex devtools are broken if a flex container is a grandchild of a flex container

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit this data URL:

data:text/html,<div id="outer" style="display:flex"><div><div id="inner" style="display:flex">aaa

 2. Inspect the page, and click the "flex" badge for <div id="outer">, and be sure the "Layout" pane is selected on the right edge of devtools.

 3. Now expand the DOM tree (on the left side of devtools) and click the row for <div id="inner">.  (Don't click its flex badge, just click its entry in the DOM tree.)

ACTUAL RESULTS:
 In Nightly 2018-10-11: The right part of devtools just entirely disappears.
 In Nightly 2018-10-12: Nothing disappears, but the right part of devtools mistakenly says "Flex item of div#outer"... but this node is *not* a flex item of div#outer.


EXPECTED RESULTS:
 - The flex container pane should be available for this "inner" element (because it is a flex container)
 - No flex item pane should be available for this "inner" element (because it is not a flex item).


I think the behavior change here may've come from Bug 1496458, BTW. (Though, clearly, the outcome here was problematic both before & after that change.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of right pane disappearing

> ACTUAL RESULTS:
>  In Nightly 2018-10-11: The right part of devtools just entirely disappears.

Here's a screencast of this, BTW.  Also, when this happens, I get this in my browser console:

NetUtil.jsm:266:12
TypeError: "flexItem is undefined; can't access its "nodeFront" property"
render resource://devtools/client/inspector/flexbox/components/FlexItemSelector.js:73:11

(To be clear, this may've been somewhat addressed, as it doesn't happen in latest Nightly from 10-12 - we get a different misrendering there, and no browser console errors.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screencast of element mistakenly labeled as a flex item

>  In Nightly 2018-10-12: Nothing disappears, but the right part
> of devtools mistakenly says "Flex item of div#outer"...
> but this node is *not* a flex item of div#outer.

And here's a screencast of this outcome.

(In the screencast here, I've added a few non-flex-container sibling nodes to the data URL testcase, just to demonstrate that these siblings are *not* mistakenly labeled as flex items. It's only the flex container that is mistakenly labeled as a flex item.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screencast showing flex container incorrectly being labeled as a flex item

>  In Nightly 2018-10-12: Nothing disappears, but the right part
> of devtools mistakenly says "Flex item of div#outer"...
> but this node is *not* a flex item of div#outer.

And here's a screencast of this outcome.

(In the screencast here, I've added a few non-flex-container sibling nodes to the data URL testcase, just to demonstrate that these siblings are *not* mistakenly labeled as flex items. It's only the flex container that is mistakenly labeled as a flex item.)

Comment 4

[Patrick Brosset <:pbro>]

I would actually flag this as a P1 because the panel is broken, and can't be used after this happens.
We should aim to fix this in 64 (uplift to beta if we need more time to fix it).

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Patrick Brosset <:pbro> from comment #4)
> I would actually flag this as a P1 because the panel is broken, and can't be
> used after this happens.

That's no longer the case after Oct 11, I think.   In more recent nightlies, the panel can still be used after this issue happens, but it's just got one element mistakenly labeled as a flex item.  (See screencast in attachment 9016768 [details])

Comment 6

[Micah [:mtigley] (she/her)]

Attachment: Bug 1498669 - flex devtools are broken if a flex container is a grandchild of a flex container. r=gl

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dfafc01cf47
flex devtools are broken if a flex container is a grandchild of a flex container. r=gl

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7dfafc01cf47

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Pulsebot from comment #7)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/7dfafc01cf47
> flex devtools are broken if a flex container is a grandchild of a flex container. r=gl

:mtigley, one drive-by review nit for subsequent patches: it seems like you often use the bug title as the commit message, but that's usually suboptimal.

The commit message best-practice (for mozilla code at least) is to describe-the-change, not to describe-the-problem.  Typically bug titles are a description of the problem (as was the case here), which makes them unsuitable as commit messages most of the time.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment for more.