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

RESOLVED FIXED in Firefox 64

Status

defect
P1
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: dholbert, Assigned: mtigley)

Tracking

unspecified
Firefox 64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 months ago
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.)
(Reporter)

Comment 1

6 months ago
> 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.)
(Reporter)

Comment 2

6 months ago
>  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 hidden (typo)
(Reporter)

Updated

6 months ago
Attachment #9016769 - Attachment is obsolete: true
Blocks: 1478396
(Assignee)

Updated

6 months ago
Assignee: nobody → mtigley
Status: NEW → ASSIGNED
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).
Priority: P2 → P1
(Reporter)

Comment 5

6 months ago
(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])
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 7

6 months ago
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7dfafc01cf47
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Reporter)

Comment 9

6 months ago
(In reply to Pulsebot from comment #7)
> Pushed by archaeopteryx@coole-files.de:
> 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.
You need to log in before you can comment on or make changes to this bug.