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.

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!)

Thanks Patrick for providing a patch for this! I will take it from here.

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; }

Try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8de0ebd01f80035fbeeed549965e2e8b53cd9d95

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

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:

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.

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.