1422386 - Remove the 'toolbardecoration' binding

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This is unblocked by bug 1428930.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1422386 - Remove toolbardecoration, toolbar, and toolbar-base binding,

Review commit: https://reviewboard.mozilla.org/r/219846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219846/

Comment 4

[:Paolo Amadini]

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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/1-2/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/2-3/

Comment 7

[Brian Grinstead [:bgrins]]

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 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/3-4/

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/4-5/

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 13

[Neil Deakin]

You need to use extends="xul:button"

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/5-6/

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219846/diff/6-7/

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

New mozscreenshot URL: 
https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=ffda3ab9975f01ea4091f769753f96735c75d62f&newProject=try&newRev=7557bc7b31e6bdff9e2ee946bd030d6dc654fd90

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64c976234100
Remove toolbardecoration, toolbar, and toolbar-base binding, r=Paolo

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/64c976234100

Comment 20

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336