1429464 - Remove toolbox binding

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, task)

Description

[Tim Nguyen :ntim]

As of bug 1428938, the toolbox binding is only used for the `palette` field and has `-moz-appearance: toolbox;` associated to it.

The `palette` field can probably be folded with the CustomizableUI.jsm module, while the styles can be applied where applicable.

Comment 1

[Brian Grinstead [:bgrins]]

I'm assuming this also requires loading toolbars.css as a UA sheet so that we don't lose those styles from toolbar-base

Comment 2

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> I'm assuming this also requires loading toolbars.css as a UA sheet so that
> we don't lose those styles from toolbar-base

There's really not much from toolbars.css that toolbox needs. It just needs -moz-appearance: toolbox; which may not require everything bug 1420229 is doing.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #2)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > I'm assuming this also requires loading toolbars.css as a UA sheet so that
> > we don't lose those styles from toolbar-base
> 
> There's really not much from toolbars.css that toolbox needs. It just needs
> -moz-appearance: toolbox; which may not require everything bug 1420229 is
> doing.

Indeed - it looks like we could move the `-moz-appearance: toolbox` rule into xul.css since that's the only relevant thing set in toolbars.css (https://searchfox.org/mozilla-central/search?q=toolbox+%7B&path=).

Comment 4

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

Attachment: Bug 1429464 - Remove toolbox binding

- Move -moz-appearance: toolbox to xul.css.
- Remove the markup that initialize the palette field to null,
  effectively initialize the value to undefined, which is still
  falsey.

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8948320 [details]
Bug 1429464 - Remove toolbox binding

https://reviewboard.mozilla.org/r/217802/#review223672

Thanks! 

Note that the try push is broken because of the cert stuff that's closing trees at the moment, but it loooooks like we might be OK here.

::: commit-message-ef1fe:4
(Diff revision 1)
> +- Remove the markup that initialize the palette field to null,
> +  effectively initialize the value to undefined, which is still
> +  falsey.

Can you add here that the toolbar binding remains responsible for initializing the toolbox's palette property with an actual DOM node? Thanks!

Comment 6

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

Comment on attachment 8948320 [details]
Bug 1429464 - Remove toolbox binding

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df6d2c3ee671
Remove toolbox binding r=Gijs

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/df6d2c3ee671