Flex item drawer is shown for elements that are not actually flex items

VERIFIED FIXED in Firefox 65

Status

defect
P1
normal
VERIFIED FIXED
7 months ago
7 months ago

People

(Reporter: mbalfanz, Assigned: mtigley)

Tracking

(Blocks 1 bug, {regression})

65 Branch
Firefox 66
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 verified, firefox66 verified)

Details

Attachments

(4 attachments)

Posted video flex.mp4
STR:
- Visit https://bugzilla.mozilla.org/show_bug.cgi?id=1409965
- Open the inspector and select #header-item > a

ER:
- sidebar should show flex container section

AR:
- sidebar shows flex container as well as flex item section, though the item section is empty.  the selected element is actually not a flex item of div.inner


The problem can be reproduced in multiple places, e.g. for div.dinner.
Thanks for catching that Martin. I'm fairly sure this must be a regression from bug 1507723, so this is my fault.
We need to find a fix for this and uplift it to 65.
I don't think this is too hard to fix, I think I must have accidentally removed an early return from getCurrentDisplay in devtools/server/actors/layout.js when I refactored that part, and unfortunately we didn't have a test covering that specific case.
This is a reduced test case. If you select the deepest div.flex element, then you'll see 2 accordion items in the sidebar, one because it's a container (correct), and one because it's an item (incorrect).
This happens because we walk the DOM up too far.
Posted patch fix.diffSplinter Review
Micah, I think this might be the right fix, but I'd like your point of view on it.
This is code you modified recently, and then I changed some parts of it. So I would like you to verify the reasoning here (and also pick up the bug if you have time, that'd be great!)
Flags: needinfo?(tigleym)
Flags: needinfo?(tigleym) → needinfo?(mtigley)
Thanks Patrick for providing a patch for this! I will take it from here.
Assignee: nobody → mtigley
Status: NEW → ASSIGNED
Flags: needinfo?(mtigley)
I noticed this code will still evaluate a non-flex item node as a flex item if it also happens to be a flex container. This can be reproduced here at https://webflow.com/ by selecting the element .nav_main.

One way we can solve this is using the `dontRecurse` check to know if we are only looking at the node's parent and not the node itself as flex containers. We could also rename `dontRecurse` to `onlyLookAtContainer`. So maybe something like this:

      if (flexType && displayType.includes("flex")) {
        if (!onlyLookAtContainer) {
          return new FlexboxActor(this, node);
        }

        const container = findFlexOrGridParentContainerForNode(node, type, this.walker);

        if (container && flexType) {
          return new FlexboxActor(this, container);
        }

        return null;
      }
Priority: -- → P1
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ce9fdbbc4b0
Fixed it so getCurrentDisplay also checks if a flex container is actually a flex item. r=pbro
https://hg.mozilla.org/mozilla-central/rev/7ce9fdbbc4b0
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Comment on attachment 9030879 [details]
Bug 1513500 - Fixed it so getCurrentDisplay also checks if a flex container is actually a flex item. r=pbro

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1507723

User impact if declined: In DevTools' Flexbox inspector, users will be shown flex containers as a flex item of a grandparent flex container. This is incorrect.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The changes are small and only affects how the Flexbox Inspector knows what elements to display as flex items and/or containers.

String changes made/needed:
Attachment #9030879 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+
Comment on attachment 9030879 [details]
Bug 1513500 - Fixed it so getCurrentDisplay also checks if a flex container is actually a flex item. r=pbro

[Triage Comment]
Fixes incorrect attribution of flex containers in the Flexbox inspector. Approved for 65.0b5.
Attachment #9030879 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox  65.0a1 (2018.12.07) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 65.0b5 build from taskcluster and 66.0a1 on Win 10 x64, Mac OS X 10.14 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.