Closed Bug 1513500 Opened 2 years ago Closed 2 years ago

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


(DevTools :: Inspector, defect, P1)

65 Branch


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

Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- verified
firefox66 --- verified


(Reporter: mbalfanz, Assigned: mtigley)


(Blocks 1 open bug)


(Keywords: regression)


(4 files)

Attached video flex.mp4
- Visit
- Open the inspector and select #header-item > a

- sidebar should show flex container section

- 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.
Attached file grand-parent-flex.html
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.
Attached 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
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 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
Fixed it so getCurrentDisplay also checks if a flex container is actually a flex item. r=pbro
Closed: 2 years 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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.