1519577 - Convert toolbarbutton bindings to custom elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

These bindings are ready to be converted to custom element. Its anonymous content can be prepended/appended when CE connects.

• toolbarbutton
• toolbarbutton.xml#menu
• toolbarbutton-badged
• toolbarbutton-badged-menu
• button

They will inherit a button-base base class that may be implemented as a mixin. button-base will inherit basetext.

The use of display feature will need to be removed first.

Noted that the toolbarbutton.xml#menu binding is unrelated to menu.xml#menu, which is covered in bug 1519502.

raw note (note on menu is obsolete because of the fact above):

button-base
Already converted, but we need to remove all XBL inherited bindings before this can be removed. Bug can be filed to track dependency.
toolbarbutton
menu
See menu-base above. Could first move the menu selector from xul.css to menu-base binding, although it doesn’t cover the removal of this binding
toolbarbutton-badged
This uses <children> added in https://bugzilla.mozilla.org/show_bug.cgi?id=1419005, only for download toolbar button. May be able to move the caller?
https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/browser/base/content/browser.xul#1088-1097
toolbarbutton-badged-menu https://bugzilla.mozilla.org/show_bug.cgi?id=1512993
button

Comment 1

[Andrew Swan [:aswan]]

One thing I've stumbled over here is the anchor attribute on toolbarbuttons. It appears to be used in exactly one place:
https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/browser/base/content/browser.xul#1207

This is a problem since the value of that attribute is looked up as an anonid attribute on anonymous toolbarbutton content:
https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/layout/xul/nsMenuFrame.cpp#636-637

But with the conversion from XBL to a CE, this stops working. I can convert this to keep it working but I'm wondering if its actually still used?

I can't figure out how to get the dropmarker to appear next to the bookmark button, in some cases it is explicitly hidden:
https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/browser/themes/shared/customizableui/panelUI.inc.css#1342-1345

In other cases, more generic rules hide it, eg:
https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/browser/themes/shared/toolbarbuttons.inc.css#133-135

Neil, it looks like you added this originally in bug 941409. If it is still applicable, can you show me how to get a dropmarker to appear next to that button? If not, can we just remove the anchor= handling from nsMenuFrame (ie, basically revert bug 941409)?

Comment 2

[Neil Deakin]

The anchor attribute is used to specify an alternate element that the menu will be aligned with instead of the button. Aligning with the bookmarks button dropmarker was actually added in bug 855805 so Marco would be a better person to ask about whether this is needed.

Comment 3

[Marco Bonardo [:mak]]

Sorry for the delay in answering, I think that attribute and the css rules hiding the dropmarker explicitly on bookmarks-menu-button are leftovers.
In the past the bookmarks menu button used to contain both the Star and the menu, and the 2 parts were working separately. But we moved back the star to the urlbar in bug 1352120, and from then it became a "menu" instead of a "menu-button". The code was left in, waiting for Photon to become the default. When Photon shipped, the legacy code was removed, but some parts of it were likely missed. And this is part of those leftovers.

Comment 4

[Andrew Swan [:aswan]]

Thanks. So back to you Neil, any objections to removing the supporting code from nsMenuFrame.cpp? The coupling between that code and the internal structure of <toolbarbutton> is awkward...

Comment 5

[Neil Deakin]

It could be removed, but you could also just change the GetAnonymousElementByAttribute line to use mContent->ShadowRoot()->GetElementById() instead.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #5)

It could be removed, but you could also just change the GetAnonymousElementByAttribute line to use mContent->ShadowRoot()->GetElementById() instead.

Sounds like it's not used anymore in the frontend, so I'd opt for removal.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Neil Deakin from comment #5)

It could be removed, but you could also just change the GetAnonymousElementByAttribute line to use mContent->ShadowRoot()->GetElementById() instead.

Except we're not using shadow dom yet for other reasons. If we're not using this feature, I'd definitely prefer to remove it to avoid having platform code depend on the structure of the CE.

Comment 8

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Update test_bug467123 to not rely on button.xml r=smaug

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Remove test_bug562554.xul dependence on button.xml r=bzbarsky

This is essentially reverting the test changes from bug 653881. The
platform changes that motivated bug 653881 were reverted in bug 888787
but the test changes were left in place at that time.

Comment 10

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Update devtools test to use a test-only xbl binding r=jdescottes

Comment 11

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Remove support for unused toolbarbutton anchor attribute r=NeilDeakin

This code contains an undesirable dependency between the layout code and
the implementation of toolbarbutton. Since it isn't actually used anywhere
by the firefox UI, remove it here.

Comment 12

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Mark badged page action toolbarbuttons before inserting r=Gijs

The "add search engine" page action wants to use badged toolbarbuttons,
but it does this by adding the "badged-button" class (which changes the
XBL binding) after the button has already been created and added to
the DOM. To avoid having to switch between non-badged and badged buttons
when toolbarbutton is converted to custom elements, add a property to
page actions so that the badged status can be determined when the element
is created.

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1519577 Convert toolbarbutton to a custom element r=surkov

Comment 14

[Andrew Swan [:aswan]]

try/talos results:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0484b3ac419191282ba35e1542adb60115734b37
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=454a99ca27434e84d6ad52af99d5f5e9b7ba3b25&newProject=try&newRevision=294ab33cd650d63bb0307c87e6d79edaabeaa188

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f53ad1e3674
Update test_bug467123 to not rely on button.xml r=smaug
https://hg.mozilla.org/integration/autoland/rev/cf4e91d423b4
Remove test_bug562554.xul dependence on button.xml r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/9f311461ad1e
Update devtools test to use a test-only xbl binding r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/293af339be51
Remove support for unused toolbarbutton anchor attribute r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/bcedf1e9b09c
Mark badged page action toolbarbuttons before inserting r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2999bbc0422b
Convert toolbarbutton to a custom element r=surkov

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5192370c4f79
Fix browser chrome failures. r=aswan

Comment 17

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed&revision=2999bbc0422b544bbeab7097d3ca8349d606a2ea&searchStr=mochitest&selectedJob=247634066

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247634066&repo=autoland&lineNumber=15209
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247634064&repo=autoland&lineNumber=5098

Backout link: https://hg.mozilla.org/integration/autoland/rev/fd68158a926029c0f7669d56e34bd22fef461c6f

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bf01d96579
Update test_bug467123 to not rely on button.xml r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de5e0880f85
Remove test_bug562554.xul dependence on button.xml r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0abb76f7bb
Update devtools test to use a test-only xbl binding r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d77e63f4d3
Remove support for unused toolbarbutton anchor attribute r=NeilDeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/595c3065e9ac
Mark badged page action toolbarbuttons before inserting r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e9b6a81194
Convert toolbarbutton to a custom element r=surkov

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/57bf01d96579
https://hg.mozilla.org/mozilla-central/rev/8de5e0880f85
https://hg.mozilla.org/mozilla-central/rev/8f0abb76f7bb
https://hg.mozilla.org/mozilla-central/rev/82d77e63f4d3
https://hg.mozilla.org/mozilla-central/rev/595c3065e9ac
https://hg.mozilla.org/mozilla-central/rev/c8e9b6a81194