Closed Bug 1433897 Opened 6 years ago Closed 6 years ago

Flexbox highlighter overlay don't get updated in response to dynamic changes

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: dholbert, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

STR:
 0. Set pref devtools.inspector.flexboxHighlighter.enabled to true

 1. View this testcase:
data:text/html,<div style="display:flex; align-items:start; height: 500px; background: yellow"><div>abc

 2. Right-click the yellow flex container, choose "Inspect", and click the little icon next to "display:flex" in devtools to activate the flexbox highlighter.

 3. In devtools (right below the "display:flex" whose icon you just clicked), click "align-items:start" and change it to "end".

EXPECTED RESULTS:
Highlighted overlay should change to show new position for flex item.

ACTUAL RESULTS:
Highlighted overlay does not change (though the flex item itself does move).
Attached image screenshot
Assignee: nobody → gl
Blocks: dt-flexbox
Status: NEW → ASSIGNED
Priority: -- → P3
This requires implementing the `hasMoved` method in the FlexboxHighlighter class in a way that it returns true when the flexbox layout changes.
For now it only returns true when the container has moved or changed size.

I believe we can use the same thing we do in the CSS Grid highlighter, whereby we serialize all of the info we know about the flexbox container/lines/items and keep comparing this everytime we try to update.
Attached patch hasMoved.diffSplinter Review
I don't really have time to work on this, but this diff might be a helpful start.
I couldn't make it work completely by just comparing the data from getAsFlexContainer because that doesn't change when certain things change (justify-content for example).
Which is why I also added some checks for the computed values of justify-content and align-items.
It's still not complete though: if you change align-self on just one of the items, the hasMoved function still returns false.
Comment on attachment 8966108 [details]
Bug 1433897 - Implement hasMoved method in the flexbox highlighter to update the highlighter on changes.

https://reviewboard.mozilla.org/r/234860/#review240522

::: devtools/server/actors/highlighters/flexbox.js:269
(Diff revision 1)
> +    let oldFlexData = stringifyFlexData(this.flexData, this.win);
> +    this.flexData = this.currentNode.getAsFlexContainer();
> +    let newFlexData = stringifyFlexData(this.flexData, this.win);

See my comment at the end about stringifying, which is slower than just comparing objects.
And, btw, in my test, I only stringified once, so it would be even slower here.

::: devtools/server/actors/utils/flexbox-utils.js:51
(Diff revision 1)
> +    }
> +
> +    stringifiableData.lines.push(stringifiedLine);
> +  }
> +
> +  return JSON.stringify(stringifiableData);

I remember using this technique in the grid highlighter because it had one very nice advantage: we'd be able to just compare 2 strings very easily.
The idea was that it would be very easy to stringify the grid data, store it, and then compare with the new one at each iteration.

Turns out, to stringify, we needed to write some specific code because the output of getGridFragments wasn't stringifiable straight away unfortunately. So we had to iterate over everything, then store as a string, and then compare.

We're doing the same thing here, and while I was looking at the code, I realized we had never tested this technique for performance.

So I did, just now.
I ran this in a loop 100k times and measured the time. And did the same with object comparison instead.
Object comparison is at least twice as fast. And this is expected in fact: 
when stringifying the data, we iterate on all items and all properties already, so while we're doing this, we could compare with another object at the same time. And we'd be done.
No need to stringify.

So, because this does run in a rAF loop all the time, I think we should use object comparison instead.
Attachment #8966108 - Flags: review?(pbrosset)
Comment on attachment 8966108 [details]
Bug 1433897 - Implement hasMoved method in the flexbox highlighter to update the highlighter on changes.

https://reviewboard.mozilla.org/r/234860/#review244534
Attachment #8966108 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/728f77ddf74e
Implement hasMoved method in the flexbox highlighter to update the highlighter on changes. r=pbro
https://hg.mozilla.org/mozilla-central/rev/728f77ddf74e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
Blocks: 1470379
You need to log in before you can comment on or make changes to this bug.