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

Status: RESOLVED WORKSFORME

(Toolkit :: UI Widgets, enhancement, P4)

Description

[Hazel Ward]

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.

Comment 1

[Florian Quèze [:florian]]

http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/toolkit/content/widgets/popup.xml#267
                if (accel.boxObject.width > width)
                  width = accel.boxObject.width;

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

Review commit: https://reviewboard.mozilla.org/r/226284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226284/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I'll add a test in a second commit on top of the one I just posted.

Comment 5

[:Gijs (he/him)]

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. :-)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[maggus.staab]

would it make sense to 

> requestAnimationFrame() 

only when the array actually contains elements?

Comment 8

[Florian Quèze [:florian]]

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 9

[Mike Conley (:mconley) (:⚙️)]

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. :/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Florian Quèze [:florian]]

(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 :-).

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I'm throwing this back into the pond. Might pick it up again down the road.

Comment 13

[:Gijs (he/him)]

bug 1555497 removed the popupshowing handler entirely from the XBL binding.