Closed Bug 1422386 Opened 6 years ago Closed 6 years ago

Remove the 'toolbardecoration' binding

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: timdream)

References

Details

Attachments

(1 file)

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
Priority: -- → P5
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 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+
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
(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=try&newRev=3d916350228ddddb1c4cb56c52adabcedd133804
(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"
(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."
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64c976234100
Remove toolbardecoration, toolbar, and toolbar-base binding, r=Paolo
https://hg.mozilla.org/mozilla-central/rev/64c976234100
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: