1450816 - Remove toolbarpaletteitem-palette binding and combine toolbarpaletteitem and toolbarpaletteitem-palette-wrapping-label bindings

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P1)

Description

[Tim Nguyen :ntim]

toolbarpaletteitem-palette looks unused.

toolbarpaletteitem-palette-wrapping-label is the same as toolbarpaletteitem, but with an extra label.

It should be trivial to convert them together.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #0)
> toolbarpaletteitem-palette looks unused.

Indeed - it looks like the CSS rule in xul.css for toolbarpaletteitem[place="palette"] is overridden by the same rule rule in browser.css that applies toolbarpaletteitem-palette-wrapping-label.

> toolbarpaletteitem-palette-wrapping-label is the same as toolbarpaletteitem,
> but with an extra label.
> 
> It should be trivial to convert them together.

What are you thinking here? I guess just hiding the label by default and then showing it for toolbarpaletteitem[place="palette"] would do the trick.

Comment 2

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> What are you thinking here? I guess just hiding the label by default and
> then showing it for toolbarpaletteitem[place="palette"] would do the trick.

Yes, exactly this :)

Comment 3

[Tim Nguyen :ntim]

The bindings are located here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#172

Comment 4

[:Gijs (he/him)]

The reason all this is the way it is is the toolkit/browser separation. If we're consolidating this we should probably do it in browser/ rather than toolkit/, to avoid messing things up for TB/SM (who could just resurrect the toolkit bindings in comm-central).


I'm not convinced this is a good-first-bug, but if you really think so it needs more detailed instructions and a mentor. Removing keyword for now to avoid confusing contributors, and ni for :ntim to see what they want to do here.

Comment 5

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #4)
> The reason all this is the way it is is the toolkit/browser separation. If
> we're consolidating this we should probably do it in browser/ rather than
> toolkit/, to avoid messing things up for TB/SM (who could just resurrect the
> toolkit bindings in comm-central).

I don't understand, all those bindings are in toolkit/, if we merge them, and change the toolkit styles accordingly, TB/SM won't need to do anything. Moving them to browser/ is also fine, but I don't see the need atm.

Comment 6

[Tim Nguyen :ntim]

Oh right, the binding is overriden by browser.css. I guess we can move the new combined toolbarpalette bindings to browser/ and get rid of the other two.

Comment 7

[:Paolo Amadini]

Attachment: Bug 1450816 - Combine the "toolbarpaletteitem" bindings in the "browser" folder.

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

Comment 8

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ddf0441e47b877d1bea6b4ff62d8eb3be978d5

Comment 9

[:Gijs (he/him)]

Comment on attachment 8979237 [details]
Bug 1450816 - Combine the "toolbarpaletteitem" bindings in the "browser" folder.

https://reviewboard.mozilla.org/r/245458/#review251416

Please give TB/SM a heads-up that they will need to replace these.

Comment 10

[:Paolo Amadini]

The remaining bindings in "toolbar.xml" are used only by one test file on Mac, but are still used on Windows. For now I'm simply updating the whitelist, since we want to remove the bindings anyways in the end.

Comment 11

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa309b567b6530e543022624f16bc79f2dec9060

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06bdb8c0ced8
Combine the "toolbarpaletteitem" bindings in the "browser" folder. r=Gijs

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/06bdb8c0ced8