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

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: 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.

Comment 1

[Patrick Brosset <:pbro>]

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.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: 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.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: fix.diff

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

Attachment: Bug 1513500 - Fixed it so getCurrentDisplay also checks if a flex container is actually a flex item. r=pbro

Comment 7

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

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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 9

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 10

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

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 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0d66c5be649b

Comment 13

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.