Convert toolbarbutton bindings to custom elements

RESOLVED FIXED in Firefox 69

Status

()

task
P3
normal
RESOLVED FIXED
5 months ago
17 days ago

People

(Reporter: timdream, Assigned: aswan)

Tracking

(Blocks 1 bug)

unspecified
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox69 fixed)

Details

(Whiteboard: [xbl-available])

Attachments

(6 attachments)

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]
Depends on: 1538983
Summary: Convert button/toolbarbutton bindings to custom elements → Convert toolbarbutton bindings to custom elements
Assignee

Updated

2 months ago
Depends on: 1546541
Blocks: 1538547
Blocks: 1546352
Blocks: 1547229
Blocks: 1547233
Assignee

Updated

2 months ago
Assignee: nobody → aswan
Assignee

Comment 1

2 months ago

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)

Comment 2

2 months ago

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)
Assignee

Comment 4

2 months ago

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)

Comment 5

2 months ago

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.

Assignee

Comment 7

2 months ago

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

Assignee

Comment 9

Last month

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.

Assignee

Comment 11

Last month

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.

Assignee

Comment 12

Last month

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 15

Last month
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

Comment 16

Last month
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5192370c4f79
Fix browser chrome failures. r=aswan

Comment 18

Last month
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
Assignee

Updated

Last month
Flags: needinfo?(aswan)
Assignee

Updated

Last month
Regressions: 1554437

Updated

26 days ago
Regressions: 1555496

Updated

26 days ago
Regressions: 1555501

Updated

19 days ago
Type: enhancement → task

Updated

17 days ago
Regressions: 1557871
You need to log in before you can comment on or make changes to this bug.