1639925 - Strip toolbarbutton inner DOM to the minimum required to reduce memory usage and CPU overhead

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task)

Description

[:Gijs (he/him)]

Toolbarbuttons were ported from XBL into custom elements, and they inherited the following DOM:

https://searchfox.org/mozilla-central/rev/df4c90d4b8c92c99f76334acfe4813c573c12661/toolkit/content/widgets/toolbarbutton.js#49-52

        <image class="toolbarbutton-icon"></image>
        <label class="toolbarbutton-text" crop="right" flex="1"></label>
        <label class="toolbarbutton-multiline-text" flex="1"></label>
        <dropmarker type="menu" class="toolbarbutton-menu-dropmarker"></dropmarker>

Pretty much all toolbarbuttons show their icon, but:

1. only one of the two labels is ever visible (which of the two depends on the wrap attribute on the item)
2. the dropmarker is only used for type=menu toolbarbuttons , but we pay the cost of having it for every toolbarbutton. There are only 2 menu buttons in browser.xhtml: one is the bookmarks menu button, and the other the overflow chevron on the bookmarks toolbar. Neither shows the dropmarker, and neither is visible by default anyway. We also use it for bookmarks folders in the toolbar, but do not display the dropmarker. We also dynamically add the type=menu attribute on the back/fwd buttons and in some cases to the new tab button, but again, do not show the dropmarker. There are a few instances in the library window that do show the dropmarker (but only on macOS).

When the button was XBL, we didn't have much choice about paying these costs (though we may have been able to avoid some, but not all, by marking the inner elements display: none), but at this point there's no reason to pay the costs for all this overhead for every button anymore.

To solve these issues:

1. we can unify the two toolbarbutton text labels. The only difference is whether we end up adding the label as a value attribute or as text content, and with custom elements we can control this behaviour dynamically (based on the existing wrap attribute), without paying any more costs than we already do (as attribute inheritance is already JS-implemented).
2. We should not include the dropmarker except when buttons explicitly ask for it via a new, separate attribute, and add this to the places buttons and nowhere else.

The same issues apply to the badged toolbarbutton fragment, so it should be included in these changes.

When constructing and render()ing 1024 toolbarbuttons, even on a fast machine we spend roughly 300ms, more than half of which is inside append/clone and the CE inheritance code, so I'm hoping to cut that time in half (or maybe better).

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1639925 - stop including dropmarkers in all toolbarbuttons, r?mak

This makes dropmarkers 'optional extras', effectively, and opts the existing
consumers I could find (the places window on macOS, and the 'more engines'
item in the search field popup) into these dropmarkers.

This allows removing all the CSS that was hiding these items in various
places.

We deliberately do not support adding dropmarkers at runtime. I also noticed
that the 'more engines' item already adds the 'badged' attribute after
connecting the node to the DOM, which may result in it not applying properly,
and fixed this.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1639925 - implement 'wrap' support in toolbarbuttons on a single element instead of 2, r?bgrins

This unifies toolbarbutton-text and toolbarbutton-multiline-text. We now
always use toolbarbutton-text for the button's text, but can either use
textContent or the value attribute, depending on the value of the wrap
attribute. This reduces DOM size and consumer complexity, at the cost of
adding some logic to toolbarbutton.js itself.

Depends on D76382

Comment 3

[Phabricator Automation]

Comment on attachment 9150855 [details]
Bug 1639925 - stop including dropmarkers in all toolbarbuttons, r?mak

Revision D76382 was moved to bug 1640493. Setting attachment 9150855 [details] to obsolete.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/983a55f50d60
implement 'wrap' support in toolbarbuttons on a single element instead of 2, r=bgrins

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/983a55f50d60