Remove the 'toolbardecoration' binding

RESOLVED FIXED in Firefox 60



2 years ago
11 months ago


(Reporter: bgrins, Assigned: timdream)


(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)



(1 attachment)



2 years ago
Just like what we did for menuseparator: we have three tags tied to toolbardecoration: toolbarseparator, toolbarspacer, toolbarspring.


Example of what a patch might look like (although the place where the CSS file gets referenced may change based on the outcome of Bug 1420229)


2 years ago
Priority: -- → P5

Comment 1

a year ago
The accessibility portion of this will be handled in Bug 1428930. There'll still need to be work done here to move the CSS into components.css and remove toolbardecoration/toolbar-base bindings similar to what the changeset linked in the description does.
Depends on: 1428930
Summary: Remove the 'toolbardecoration' binding and move accessibility into XUL based on tag name instead of XBL role → Remove the 'toolbardecoration' binding
This is unblocked by bug 1428930.
Assignee: bgrinstead → timdream

Comment 4

a year ago
Comment on attachment 8950569 [details]
Bug 1422386 - Remove toolbardecoration, toolbar, and toolbar-base binding,

Looks good to me, but the commit message is outdated, please update it to reflect which bindings could be removed and why.
Attachment #8950569 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a year ago
It's probably worth doing a mozscreenshots run since this is changing the CSS to a UA style. The way I do it usually is: 

1. Remove the patch and do `./mach try fuzzy -q "browser-screenshots"`
2. Apply the patch and do `./mach try fuzzy -q "browser-screenshots"`

Then plug them into this URL (replacing 1 and 2 with the actual revisions):
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > It's probably worth doing a mozscreenshots run since this is changing the
> > CSS to a UA style.
> ?oldProject=try&oldRev=ffda3ab9975f01ea4091f769753f96735c75d62f&newProject=tr
> y&newRev=3d916350228ddddb1c4cb56c52adabcedd133804

Awesome, looks like loading it as a UA style didn't cause any changes (at least on osx/linux as win builds aren't working right now)
OK. We cannot land this yet because there is still one test failure that I haven't figured out why:

So far I can "fix" it locally with

diff --git a/toolkit/content/widgets/toolbar.xml b/toolkit/content/widgets/toolbar.xml
--- a/toolkit/content/widgets/toolbar.xml
+++ b/toolkit/content/widgets/toolbar.xml
@@ -170,5 +170,5 @@

-  <binding id="toolbarpaletteitem" display="xul:button">
+  <binding id="toolbarpaletteitem" extends="chrome://browser/content/tabbrowser.xml#empty" display="xul:button">
       <xul:hbox class="toolbarpaletteitem-box" flex="1" xbl:inherits="type,place">

I am in the process of figuring out the root cause.

Comment 13

a year ago
You need to use extends="xul:button"
Comment hidden (mozreview-request)
(In reply to Neil Deakin from comment #13)
> You need to use extends="xul:button"

Ah, thanks for the pointer.

"The 'display' attribute does not work unless the extends attribute is set, see bug 119389. In that case, put the value of the display attribute into the extends attribute."
Comment hidden (mozreview-request)

Comment 18

a year ago
Pushed by
Remove toolbardecoration, toolbar, and toolbar-base binding, r=Paolo

Comment 19

a year ago
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Moving to Core:XUL per
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.