flex items "flash" in sidebar on some sites

VERIFIED FIXED in Firefox 65

Status

defect
P3
normal
VERIFIED FIXED
6 months ago
4 months ago

People

(Reporter: mbalfanz, Assigned: gl)

Tracking

65 Branch
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 verified)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

6 months ago
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
Assignee

Updated

6 months ago
Priority: -- → P3
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.
Summary: Flex items "flash" in sidebar → flex items "flash" in sidebar on some sites
Assignee

Updated

6 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
Assignee

Comment 2

6 months ago
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.
Assignee

Comment 3

6 months ago
Posted patch 1507750.patch [1.0] (obsolete) — Splinter Review
Will also add unit tests in Part 2.
Attachment #9027432 - Flags: review?(pbrosset)
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.
Attachment #9027432 - Flags: review?(pbrosset)
Assignee

Comment 5

6 months ago
Posted patch 1507750.patch [2.0] (obsolete) — Splinter Review
Attachment #9027432 - Attachment is obsolete: true
Attachment #9027816 - Flags: review?(pbrosset)
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.
Attachment #9027816 - Flags: review?(pbrosset)
Assignee

Comment 7

6 months ago
Attachment #9027816 - Attachment is obsolete: true
Attachment #9027970 - Flags: review?(pbrosset)
Attachment #9027970 - Flags: review?(pbrosset) → review+

Comment 8

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0013f9a06d
Compare the flexbox state for any changes before updating on reflows. r=pbro

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d0013f9a06d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+

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!

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