Open Bug 1356765 Opened 4 years ago Updated 5 months ago

6.9ms uninterruptible reflow at onxblpopupshowing@chrome://global/content/bindings/popup.xml:268:1

Categories

(Toolkit :: XUL Widgets, enhancement, P4)

enhancement

Tracking

()

People

(Reporter: rjward0, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [ohnoreflow][qf:p3][fxperf:p3])

Attachments

(1 file)

Here's the stack:

onxblpopupshowing@chrome://global/content/bindings/popup.xml:268:1

triggered by hitting alt to reveal the menu bar and clicking on file.
Flags: qe-verify?
Priority: -- → P2
Component: Untriaged → XUL Widgets
Product: Firefox → Toolkit
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Duplicate of this bug: 1388504
Priority: P3 → P4
Keywords: perf
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Blocks: 1384733
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf:p2]
I'll add a test in a second commit on top of the one I just posted.
Assignee: nobody → mconley
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf:p2] → [ohnoreflow][qf:f61][qf:p1][fxperf:p1]
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232206

r=me assuming this works as evidenced by a test - but see below... I'm a bit surprised no reviewbots or commit hook eslint things turned this up!

::: toolkit/content/widgets/popup.xml:293
(Diff revision 1)
> -            }
> +              }
> -          }
> +            }
> -          for (var i = 0; i < array.length; i++)
> +          }
> +          window.requestAnimationFrame(() => {
> +            for (let i = 0; i < array.length; i++) {
> -            array[i].width = width;
> +              array[i].width = width;

ahould be maxWidth on the RHS. :-)
Attachment #8957358 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232206

> ahould be maxWidth on the RHS. :-)

Ooof - how embarassing! Thanks - will correct and re-test.
would it make sense to 

> requestAnimationFrame() 

only when the array actually contains elements?
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232342

::: toolkit/content/widgets/popup.xml:283
(Diff revision 1)
>              if (menuitem.localName == "menuitem" && menuitem.hasAttribute("acceltext")) {
> -              var accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
> +              let accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
>                if (accel && accel.boxObject) {
>                  array.push(accel);
> -                if (accel.boxObject.width > width)
> -                  width = accel.boxObject.width;
> +                // The popup is showing, which means it has already been laid
> +                // out, so these bounds should still be accurate.

How confident are you about this? Isn't the popupshowing event listener used by other code to insert menu items dynamically? Or are we relying (both with and without your patch) on dynamically inserted menu items never having any accel key?

::: toolkit/content/widgets/popup.xml:284
(Diff revision 1)
> -              var accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
> +              let accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
>                if (accel && accel.boxObject) {
>                  array.push(accel);
> -                if (accel.boxObject.width > width)
> -                  width = accel.boxObject.width;
> +                // The popup is showing, which means it has already been laid
> +                // out, so these bounds should still be accurate.
> +                let itemWidth = dwu.getBoundsWithoutFlushing(accel);

I doubt getBoundsWithoutFlushing returns a width.

::: toolkit/content/widgets/popup.xml:292
(Diff revision 1)
> -              }
> +                }
> -            }
> +              }
> -          }
> +            }
> -          for (var i = 0; i < array.length; i++)
> +          }
> +          window.requestAnimationFrame(() => {
> +            for (let i = 0; i < array.length; i++) {

nit: Touching this line seems like a good opportunity to switch this to for..of
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232342

> I doubt getBoundsWithoutFlushing returns a width.

Yeah, holy smokes - this'll teach me to cobble together a quick patch just before going home. :/
So having thought about this more, I think florian has found a pretty fatal weakness in my patch here - in the event that an event handler injects a menuitem during the popupshowing event, it'll have no dimensions calculated for it unless I flush, which means I have to wait for it to appear. Which means that there's chance of flicker if I want to get the widths of all accel labels and then adjust them.

I think what'd be best is if we could somehow just get layout to calculate all of this stuff for us rather than doing it in JS. I wonder if the new CSS grid stuff can be used in here.
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #10)
> So having thought about this more, I think florian has found a pretty fatal
> weakness in my patch here - in the event that an event handler injects a
> menuitem during the popupshowing event, it'll have no dimensions calculated
> for it

Do we have a way to detect this edge case? Eg. would getBoundsWithoutFlushing return a 0x0 rect that could be used as a hint that we need a flush?

I wouldn't want the rare edge case to stand in the way of a significant perf win for the usual case :-).
I'm throwing this back into the pond. Might pick it up again down the road.
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p1] → [ohnoreflow][qf:f61][qf:p1][fxperf:p3]
Assignee: mconley → nobody
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p3] → [ohnoreflow][qf:f64][qf:p3][fxperf:p3]
Whiteboard: [ohnoreflow][qf:f64][qf:p3][fxperf:p3] → [ohnoreflow][qf:p3:f64][fxperf:p3]
Whiteboard: [ohnoreflow][qf:p3:f64][fxperf:p3] → [ohnoreflow][qf:p3][fxperf:p3]
No longer blocks: 1384733
You need to log in before you can comment on or make changes to this bug.