Remove the 'toolbardecoration' binding

RESOLVED FIXED in Firefox 60

Status

()

P5
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

(Blocks: 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment)

(Reporter)

Description

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

- https://dxr.mozilla.org/mozilla-central/search?q=toolbardecoratio&redirect=false

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) https://hg.mozilla.org/try/rev/8b6f96efe45b915f28bc6c89c29b60bad598abc0
status-firefox57: --- → wontfix
status-firefox58: --- → wontfix
status-firefox59: --- → fix-optional
Priority: -- → P5
(Reporter)

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
mozreview-review
Comment on attachment 8950569 [details]
Bug 1422386 - Remove toolbardecoration, toolbar, and toolbar-base binding,

https://reviewboard.mozilla.org/r/219846/#review225632

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)
(Reporter)

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): https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=1&newProject=try&newRev=2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

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.
> 
> https://screenshots.mattn.ca/compare/
> ?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:
browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js

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>

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

I am in the process of figuring out the root cause.
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.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Elements#binding

"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 timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64c976234100
Remove toolbardecoration, toolbar, and toolbar-base binding, r=Paolo

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64c976234100
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
status-firefox59: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.