1420990 - Remove the preference 'container' XBL binding and instead build the DOM directly in gContainersPane._rebuildView

Status: RESOLVED FIXED

(Firefox :: Settings UI, task, P3)

Description

[Brian Grinstead [:bgrins]]

There's a binding "container" in the prefs UI that adds content inside a richlistitem in about:preferences#containers

- https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/browser/components/preferences/handlers.xml#74-95
- https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/browser/components/preferences/handlers.css#13-15

The richlist item is created here:

https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/browser/components/preferences/in-content/containers.js#29-43

Because it uses entities it's causing some extra work in Bug 1419547. But since it's only adding content, I suspect that the DOM can be built directly in containers.js as an alternative.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

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

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

I don't love the extra verbosity here. I wish we had some way other that innerHTML to be more declarative with how the markup will be in JS - going to look into that for when we start shipping Custom Elements in chrome.

But IMO it ends up more straightforward with the change (due to no more attribute mapping via `inherits` and no <children> in XBL), and it gets rid of one more binding. Let me know if you have ideas on how to make this better.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

https://reviewboard.mozilla.org/r/203204/#review240866

::: browser/components/preferences/in-content/containers.js:35
(Diff revision 4)
> +      let outer = document.createElement("hbox");
> +      outer.setAttribute("flex", 1);
> +      outer.setAttribute("equalsize", "always");
> +      item.appendChild(outer);
> +
> +      let inner = document.createElement("hbox");
> +      inner.setAttribute("flex", 1);
> +      inner.setAttribute("align", "center");
> +      outer.appendChild(inner);

You might want to look into combining outer with inner to simplify the markup. It would make it less verbose.

I used bug 1437230 as an opportunity to also simplify the markup there too.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

https://reviewboard.mozilla.org/r/203204/#review241874

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

https://reviewboard.mozilla.org/r/203204/#review241886

::: browser/components/preferences/in-content/containers.js:35
(Diff revision 4)
> +      let outer = document.createElement("hbox");
> +      outer.setAttribute("flex", 1);
> +      outer.setAttribute("equalsize", "always");
> +      item.appendChild(outer);
> +
> +      let inner = document.createElement("hbox");
> +      inner.setAttribute("flex", 1);
> +      inner.setAttribute("align", "center");
> +      outer.appendChild(inner);

Good point. I checked locally and moving `align=center` up to outer and removing `equalsize=always` then removing `inner` renders the same

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

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

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8932161 [details]
Bug 1420990 - Build the DOM for container items directly in containers.js instead of with XBL;

https://reviewboard.mozilla.org/r/203204/#review241896

::: browser/components/preferences/handlers.css:9
(Diff revision 5)
>  #containersView > richlistitem {
> -  -moz-binding: url("chrome://browser/content/preferences/handlers.xml#container");
> +  -moz-binding: none;
>  }

You can avoid this by using a vbox/hbox instead.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #10)
> Comment on attachment 8932161 [details]
> Bug 1420990 - Build the DOM for container items directly in containers.js
> instead of with XBL;
> 
> https://reviewboard.mozilla.org/r/203204/#review241896
> 
> ::: browser/components/preferences/handlers.css:9
> (Diff revision 5)
> >  #containersView > richlistitem {
> > -  -moz-binding: url("chrome://browser/content/preferences/handlers.xml#container");
> > +  -moz-binding: none;
> >  }
> 
> You can avoid this by using a vbox/hbox instead.

I thought about that, but didn't want to lose any styling or make it inconsistent with other lists in prefs. If we were OK with that we could also turn this into a Custom Element.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19dfac734e3e
Build the DOM for container items directly in containers.js instead of with XBL;r=jaws

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/19dfac734e3e