Closed Bug 1468910 Opened 2 years ago Closed 2 years ago

Flexbox highlighter doesn't update when height or alignment of flex items changes

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox63 fixed, firefox65 verified)

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed
firefox65 --- verified

People

(Reporter: sebo, Assigned: gl)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

The height of the flex items highlight areas doesn't change with the height or alignment of the flex items.

Test case:
1. Go to data:text/html,<style>%23container{width:600px;height:200px;display:flex;background-color:yellow;}div > div{background-color:snow;flex:1;border:1px solid aliceblue;margin:10px;}</style><div id="container"><div></div><div></div><div></div></div>
2. Toggle the flexbox highlighter
3. Set 'height: 50px;' on the flex items
=> The height of the highlighted areas doesn't change.
4. Set 'align-self: center;' on the flex items
=> The highlighted areas aren't repositioned.

Sebastian
Priority: -- → P3
Blocks: 1470379
Assignee: nobody → gl
Status: NEW → ASSIGNED
Blocks: 1472561
Comment on attachment 8989069 [details]
Bug 1468910 - Flexbox highlighter should update on size or alignment changes of flex items.

https://reviewboard.mozilla.org/r/254144/#review260908

::: devtools/server/actors/highlighters/flexbox.js:270
(Diff revision 1)
>      if (!this.computedStyle) {
>        this.computedStyle = getComputedStyle(this.currentNode);
>      }
>  
>      const oldFlexData = this.flexData;
> -    this.flexData = this.currentNode.getAsFlexContainer();
> +    this.flexData = getFlexData(this.currentNode.getAsFlexContainer(), this.win);

Please add `this.flexData = null` in the `destroy` function too. I don't think we release this property, but we really should now since it also contains DOM nodes.

::: devtools/server/actors/highlighters/flexbox.js:761
(Diff revision 1)
> +            mainBaseSize: item.mainBaseSize,
> +            mainDeltaSize: item.mainDeltaSize,
> +            mainMaxSize: item.mainMaxSize,
> +            mainMinSize: item.mainMinSize,
> +            node: item.node,
> +            quads: getAdjustedQuads(win, item.node, "border"),

Why get the border box here? I believe this is used in the code that renders the justify-content areas, and that are only applies in between margin boxes.

::: devtools/server/actors/highlighters/flexbox.js:829
(Diff revision 1)
> +      const { bounds: oldItemBounds } = oldItemQuads[0];
> +      const { bounds: newItemBounds } = newItemQuads[0];
> +
> +      if (oldItemBounds.bottom !== newItemBounds.bottom ||
> +          oldItemBounds.height !== newItemBounds.height ||
> +          oldItemBounds.left !== newItemBounds.left ||
> +          oldItemBounds.right !== newItemBounds.right ||
> +          oldItemBounds.top !== newItemBounds.top ||
> +          oldItemBounds.width !== newItemBounds.width ||
> +          oldItemBounds.x !== newItemBounds.x ||
> +          oldItemBounds.y !== newItemBounds.y) {
> +        return true;
> +      }

I'm not sure whether fragmentation can happen at item level or whether we are good just always checking [0] here.
Could you please check? And please add a comment explaining why it's fine to only check the first quad.
Attachment #8989069 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f947d902ed91
Flexbox highlighter should update on size or alignment changes of flex items. r=pbro
https://hg.mozilla.org/mozilla-central/rev/f947d902ed91
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

I reproduced this issue using Fx 62.0a1 (2018-06-15) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0b15, on Windows 10 x64, macOS 10.13.6 and Ubuntu 16.04 LTS

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.