Closed Bug 1519577 Opened 1 year ago Closed 1 year ago

Convert toolbarbutton bindings to custom elements

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: timdream, Assigned: aswan)

References

Details

(Whiteboard: [xbl-available])

Attachments

(6 files)

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

Priority: -- → P3
Whiteboard: [xbl-available]
Summary: Convert button/toolbarbutton bindings to custom elements → Convert toolbarbutton bindings to custom elements
Depends on: 1546541
Assignee: nobody → 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)?

Flags: needinfo?(enndeakin)

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.

Flags: needinfo?(enndeakin) → needinfo?(mak77)

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.

Flags: needinfo?(mak77)

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

Flags: needinfo?(enndeakin)

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

Flags: needinfo?(enndeakin)

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

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

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.

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.

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.

Pushed by aswan@mozilla.com:
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
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5192370c4f79
Fix browser chrome failures. r=aswan
Pushed by aswan@mozilla.com:
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
Flags: needinfo?(aswan)
Regressions: 1554437
Regressions: 1555496
Regressions: 1555501
Type: enhancement → task
Regressions: 1557871
Regressions: 1564077
Regressions: 1574819
You need to log in before you can comment on or make changes to this bug.