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

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ntim, Assigned: Paolo)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(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.
Component: XUL Widgets → Toolbars and Customization
Product: Toolkit → Firefox
(Reporter)

Comment 2

a year ago
(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 :)
(Reporter)

Updated

a year ago
Keywords: good-first-bug

Comment 4

a year ago
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.
Flags: needinfo?(ntim.bugs)
Keywords: good-first-bug
Priority: -- → P3
Summary: Remove toolbarpaletteitem-palette and combine toolbarpaletteitem and toolbarpaletteitem-palette-wrapping-label → Remove toolbarpaletteitem-palette binding and combine toolbarpaletteitem and toolbarpaletteitem-palette-wrapping-label bindings
(Reporter)

Comment 5

a year ago
(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.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 6

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: P3 → P1

Comment 9

a year ago
mozreview-review
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.
Attachment #8979237 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

a year ago
Blocks: 1463179
(Assignee)

Updated

a year ago
Blocks: 1463225
(Assignee)

Comment 10

a year ago
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 12

a year ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06bdb8c0ced8
Combine the "toolbarpaletteitem" bindings in the "browser" folder. r=Gijs

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06bdb8c0ced8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.