Convert button and button-menu bindings to a Custom Element
Categories
(Toolkit :: XUL Widgets, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: bgrins, Assigned: aswan)
References
Details
Attachments
(3 files, 1 obsolete file)
Splitting this out of Bug 1519577, since I think this can be tackled separately.
It would include these two bindings, plus button-base: https://bgrins.github.io/xbl-analysis/tree/#button. Then toolbarbutton bindings could be handled separately.
I think I'd implement the base as a JS class, then extend it with a single MozButton class which switches markup based on the type attribute.
A couple notes:
- We also have to worry about buttons which inject their own DOM that doesn't match one of observes|template|menupopup|panel|tooltip, in which case all of the anonymous content gets dropped
- I'm a bit worried about perf here - there are something like 50 buttons in browser.xul at startup and although XBL does construct the anonymous DOM eagerly, no JS references are made. We've been able to avoid requiring a callback that runs when layout happens so far, but this is the type of binding where something like that would be handy. Perhaps making initializeAttributeInheritance and DOM construction happen on the C++ side stored through CustomElementRegistry would help, but since there are cases where we want to customize the behavior (i.e. (1)) that might not solve our problem.
Reporter | ||
Comment 1•6 years ago
|
||
General outline of what needs to happen:
- Make a new button.js file in toolkit/content/widgets. We'll eventually want to
hg cp button.xml
before landing, but it's usually to deal with rebase if you just make a new file - Add this file to the jar and load it as per https://searchfox.org/mozilla-central/rev/3d469329a42644b41e66944d8da22d78d8c0f5ec/toolkit/content/customElements.js#505.
- Import MozButtonBase with no customElement.define(): https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/ButtonBase.js. As per https://bgrins.github.io/xbl-analysis/tree/#button-base, this class isn't actually bound to anything. You can drop the connectedCallback in that generated source, it's not doing anything useful.
- See Button (https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Button.js) and ButtonMenu (https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/ButtonMenu.js) implementations for reference, but they'll need to change as per (1) in comment 0. These are the classes that will be defined as custom elements
- Remove the -moz-binding CSS rules for the two button bindings at https://searchfox.org/mozilla-central/search?q=button.xml&path=css
- ButtonMenu should be registered as a Customized Built-in (like <button is="button-menu">) - see https://bugzilla.mozilla.org/show_bug.cgi?id=1519486#c0 for more details on how we did that with richlistitem.
Assignee | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #0)
- We also have to worry about buttons which inject their own DOM that doesn't match one of observes|template|menupopup|panel|tooltip, in which case all of the anonymous content gets dropped
I don't think we ever use panel/tooltip/template/observes, we do use menupopup. Would be worth checking before trying to port that over.
ButtonMenu should be registered as a Customized Built-in (like <button is="button-menu">) - see https://bugzilla.mozilla.org/show_bug.cgi?id=1519486#c0 for more details on how we did that with richlistitem.
We could combine Button/ButtonMenu into one single CE to avoid re-writing all consumers to use is="button-menu"
. That CE could check the type attribute.
Updated•6 years ago
|
Comment 3•6 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #2)
(In reply to Brian Grinstead [:bgrins] from comment #0)
- We also have to worry about buttons which inject their own DOM that doesn't match one of observes|template|menupopup|panel|tooltip, in which case all of the anonymous content gets dropped
I don't think we ever use panel/tooltip/template/observes, we do use menupopup. Would be worth checking before trying to port that over.
there's a panel inside a button, see https://searchfox.org/mozilla-central/source/browser/components/customizableui/content/customizeMode.inc.xul#67, however it seems it should be a menupopup.
Assignee | ||
Comment 4•6 years ago
|
||
wip
Assignee | ||
Comment 5•6 years ago
|
||
wip
Assignee | ||
Comment 6•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
Our standard <button> binding has been carrying around an extra <label>
just to support search highlighting in about:preferences. This patch
removes that overhead by moving the tweaks that about:preferences needs
into a custom element used only within about:preferences.
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/159eaec1337e
https://hg.mozilla.org/mozilla-central/rev/936c2d201405
https://hg.mozilla.org/mozilla-central/rev/63fcd49131d7
Comment 10•6 years ago
|
||
Using mozregression Vlad and I discovered it regressed pref dialogs:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a885e5f28d3141921d9235adfdd0b88552e507a2&tochange=63fcd49131d7303b868a34241024d4a72ed1aafc
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Description
•