1507750 - flex items "flash" in sidebar on some sites

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: flex-item-blinking.mp4

STR:
- visit any website that has flex items
- open the inspector
- select the a flex container
- hover over a flex item in the sidebar

ER:
- flex item should be highlighted with background color continuously

AR:
- flex item background highlight flashes

Comment 1

[Daniel Holbert [:dholbert]]

I can reproduce this at the site you used in your screencast ( https://stripe.com/ ), but I can't reproduce at other sites, e.g. on the top toolbar at this bugzilla page (e.g. for the "Advanced Search" flex container, which has 2 flex items "span.icon" and "span.label").

So this isn't universal (thankfully), and there's something in particular going on at stripe.com that needs to be identified I think.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

This is mainly caused by the constant reflows that stripe is throwing at us. Currently, we update the flexbox panel on every reflow hence the blinking. This was resolved for the grid inspector previously for stripe as well, and the fix involves checking if any of the flexbox redux data has changed before performing an update.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1507750.patch [1.0]

Will also add unit tests in Part 2.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 9027432 [details] [diff] [review]
1507750.patch [1.0]

Review of attachment 9027432 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this Gabriel. I only have a few comments for code simplification.

::: devtools/client/inspector/flexbox/flexbox.js
@@ +327,2 @@
>  
> +      if (JSON.stringify(oldFlexContainer) !== JSON.stringify(newFlexContainer)) {

Could you please move these 3 lines into a helper function, to hide away the actual comparison. Something like this:

if (hasFlexContainerChanged(flexbox.flexContainer, flexContainer)) {
  this.update(flexContainer)
}

The hasFlexContainerChanged function would therefore be something like this:

function hasFlexContainerChanged(old, new) {
  return JSON.stringify(getComparableFlexContainerProperties(old)) !==
         JSON.stringify(getComparableFlexContainerProperties(new));
}

@@ +342,5 @@
> +      const newFlexItemContainer = getComparableFlexContainerProperties(
> +        flexItemContainer);
> +
> +      if (JSON.stringify(oldFlexItemContainer) !==
> +          JSON.stringify(newFlexItemContainer)) {

You can then reuse the same hasFlexContainerChanged helper to simplify this part too.

@@ +343,5 @@
> +        flexItemContainer);
> +
> +      if (JSON.stringify(oldFlexItemContainer) !==
> +          JSON.stringify(newFlexItemContainer)) {
> +        this.update(flexContainer, flexItemContainer);

Isn't it sub-optimal to be updating again here?
We have already updated earlier if the flex container has changed. So maybe we need to not do that, but just store a boolean variable like hasContainerChanged. 
Then do this part, also store a boolean like hasItemContainerChanged.

And at the very end, do the update once only, passing the right arguments depending on what has changed.

if (hasContainerChanged || hasItemContainerChanged) {
  this.update(flexContainer, hasItemContainerChanged ? flexItemContainer : null);
}

@@ +438,2 @@
>     */
> +  async update(flexContainer = null, flexItemContainer = null) {

There isn't really a need to give them these null default values. By default, if these arguments aren't being passed, they will default to undefined.
And undefined is falsy, just like null. So your code below will continue working.

@@ +501,5 @@
> +  };
> +}
> +
> +/**
> + * For a given flex item object, returns the relevant flex item properties that can be

This comment is a bit misleading, we are passing a list of flex items, not just 1 single flex item object.

@@ +506,5 @@
> + * compared to check if any changes has occurred.
> + *
> + * @param  {Array} flexItems
> + *         Array of objects containing the flex item properties.
> + * @return {Object} consisting of the comparable flex item's properties.

The return type should be array, not object.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1507750.patch [2.0]

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 9027816 [details] [diff] [review]
1507750.patch [2.0]

Review of attachment 9027816 [details] [diff] [review]:
-----------------------------------------------------------------

Code changes look good. I would like these 3 tests to be simplified though. In particular, the part about waiting for reflows might not be needed at all. Or if it is, I think using testActor.reflow() after evaluating the code might make it far simpler. I can give a quick R+ after this is done.

::: devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_updates_on_change.js
@@ +30,5 @@
> +
> +  ok(flexSizingContainer, "The flex sizing info is rendered.");
> +
> +  info("Changing the flexbox in the page.");
> +  const onReflow = new Promise(resolve => {

The flexbox inspector already tracks reflows, so in theory, you should be able to just wait for the DOM to be in the state you want it to be, and that should be enough.
If you really need to listen for a reflow, I think it would be fewer lines of codes to do it by forcing another reflow and waiting for that (you can do this with testActor.reflow())

::: devtools/client/inspector/flexbox/test/browser_flexbox_item_list_updates_on_change.js
@@ +31,5 @@
> +  is(flexItemList.querySelectorAll("button").length, 1,
> +    "Got the correct number of flex items in the list.");
> +
> +  info("Changing the flexbox in the page.");
> +  const onReflow = new Promise(resolve => {

Same comment here.

::: devtools/client/inspector/flexbox/test/browser_flexbox_sizing_info_updates_on_change.js
@@ +29,5 @@
> +  is(flexItemSizingContainer.querySelectorAll("li").length, 2,
> +    "Got the correct number of flex item sizing properties in the list.");
> +
> +  info("Changing the flexbox in the page.");
> +  const onReflow = new Promise(resolve => {

And again here.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1507750.patch [3.0]

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0013f9a06d
Compare the flexbox state for any changes before updating on reflows. r=pbro

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8d0013f9a06d

Comment 10

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 65.0a1(2018-11-16), on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0b10 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS.
Cheers!