1503824 - Migrate the treecols binding into a custom element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: remove-treebody-2018-11-13-1738.diff

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-16-0902.diff

Hacky way to work around <children>.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-16-0950.diff

In addition to comment 2, looks like delayConnectedCallback makes the tree blank so I removed that.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 9025573 [details] [diff] [review]
remove-treecols-2018-11-16-0950.diff

Review of attachment 9025573 [details] [diff] [review]:
-----------------------------------------------------------------

- The commit message says treerows but should be treecols
- Do you know of an example where the columns are scrollable so I can test that locally?

Comment 5

[Brian Grinstead [:bgrins]]

ni? for Comment 4

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-20-0922.diff

I searched all our tree usage from the migration document and couldn't find any single instance where the three was horizontally scrollable.

Updated commit message.

Running through try now.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-20-1056.diff

In addition to comment 6, I've fixed an issue that I've found while working on bug 1507704: the "restore column order" from the column picker wasn't working properly anymore with the ordinal changes in this patch.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 9026329 [details] [diff] [review]
remove-treecols-2018-11-20-1056.diff

Review of attachment 9026329 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/tree.js
@@ +411,5 @@
>  
>    customElements.define("treecol", MozTreecol);
>  
> +  class MozTreecols extends MozElements.BaseControl {
> +    connectedCallback() {

As with the other CEs, start this with:

if (this.delayConnectedCallback()) { return; }

@@ +412,5 @@
>    customElements.define("treecol", MozTreecol);
>  
> +  class MozTreecols extends MozElements.BaseControl {
> +    connectedCallback() {
> +      this.textContent = "";

This doesn't seem right. We shouldn't wipe out the existing DOM here - the XBL version slots them into a box, and even if we don't need to do that we shouldn't be removing them.

Just picking a random consumer from a search: https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/browser/base/content/pageinfo/pageInfo.xul#145, there are <treecol> and <splitter> children, and this line would remove those and replace them with just a treecolpicker.

If this is intending to handle multiple connections then I guess we should do something like:

if (!this.querySelector("treecolpicker")) {
  this.appendChild(MozXULElement.parseXULToFragment( .... ))
}

@@ +413,5 @@
>  
> +  class MozTreecols extends MozElements.BaseControl {
> +    connectedCallback() {
> +      this.textContent = "";
> +      this.appendChild(MozXULElement.parseXULToFragment(`

Could you try attaching a shadow root with an `<html:slot>` inside of an <hbox class="tree-scrollable-columns">? I suspect there may be some bugs with that, but this would be a good potential consumer since the anonymous content is very lightly styled from the document side, and it would flag any platform issues that we can get on file ahead of the bigger <tree> migration.

So it'd look like:

#shadow-root
  <hbox.tree-scrollable-columns>
    <html:slot>
  </hbox>
  <treecolpicker.treecol-image />

@@ +414,5 @@
> +  class MozTreecols extends MozElements.BaseControl {
> +    connectedCallback() {
> +      this.textContent = "";
> +      this.appendChild(MozXULElement.parseXULToFragment(`
> +        <treecolpicker class="treecol-image" fixed="true" inherits="tooltiptext=pickertooltiptext"></treecolpicker>

This [inherits] won't work. You'd need to get a reference to the treecolpicker and call this.inheritAttribute(treecolpicker, "tooltiptext=pickertooltiptext"). I don't think there's any need to wire this up to attributeChangedCallback, doing it here should be fine.

::: toolkit/content/widgets/tree.xml
@@ -907,5 @@
>      </handlers>
>    </binding>
>  
> -  <binding id="treecols">
> -    <content orient="horizontal">

When there's an attr on `<xbl:content>` it copies that onto the host node. So in this case it means `<xul:treecols>` will turn into <xul:treecols orient="horizontal"> (which then sets `-moz-box-orient: horizontal` https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/orient.

AFAICT -moz-box-orient:horizontal is the default value so this can safely be removed (without either copying this logic into the CE or changing xul.css to set -moz-box-orient explicitly).

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-22-1115.diff

Addressed comments.

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 9026329 [details] [diff] [review]
> remove-treecols-2018-11-20-1056.diff
> 
> Review of attachment 9026329 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +413,5 @@
> >  
> > +  class MozTreecols extends MozElements.BaseControl {
> > +    connectedCallback() {
> > +      this.textContent = "";
> > +      this.appendChild(MozXULElement.parseXULToFragment(`
> 
> Could you try attaching a shadow root with an `<html:slot>` inside of an
> <hbox class="tree-scrollable-columns">? I suspect there may be some bugs
> with that, but this would be a good potential consumer since the anonymous
> content is very lightly styled from the document side, and it would flag any
> platform issues that we can get on file ahead of the bigger <tree> migration.
> 

I suggest we deal with this same thing more directly in bug 1503826 where it's clear that this is needed. As it stands, I think that if we can get away with not using shadow DOM or slots, with insignificant drive-by changes, we should do it.

I don't feel strongly, though. Convince me it's better to use shadow DOM and slots in this case.

> 
> ::: toolkit/content/widgets/tree.xml
> @@ -907,5 @@
> >      </handlers>
> >    </binding>
> >  
> > -  <binding id="treecols">
> > -    <content orient="horizontal">
> 
> When there's an attr on `<xbl:content>` it copies that onto the host node.
> So in this case it means `<xul:treecols>` will turn into <xul:treecols
> orient="horizontal"> (which then sets `-moz-box-orient: horizontal`
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/orient.
> 
> AFAICT -moz-box-orient:horizontal is the default value so this can safely be
> removed (without either copying this logic into the CE or changing xul.css
> to set -moz-box-orient explicitly).

It'd be good to add this to the XUL migration papercuts document!

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Created attachment 9026314 [details] [diff] [review]
> remove-treecols-2018-11-20-0922.diff
> 
> I searched all our tree usage from the migration document and couldn't find
> any single instance where the three was horizontally scrollable.

Just a heads up that we are planning to remove the .tree-scrollable-columns wrapping hbox [0] so we can do the CE conversion on treecols without using Shadow DOM (and having to redo styles for treecolpicker).

As far as we can tell, this feature isn't used in Firefox, but I do see one place in TB [1] where there are styles set on that element - presumably you are expecting horizontally scrollable columns in that case? If so, you could probably just move that wrapping hbox into the tree's light DOM around the <treecol> elements when it's used in the document. You'll also need to restore the CSS rule in xul.css for that element.

[0]: https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/toolkit/content/widgets/tree.xml#889
[1]: https://searchfox.org/comm-central/search?q=tree-scrollable&path=

Comment 11

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, I filed bug 1509982.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-27-1839.diff

Fixed test as per https://bugzilla.mozilla.org/show_bug.cgi?id=1507704#c13

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 9027955 [details] [diff] [review]
remove-treecols-2018-11-27-1839.diff

Review of attachment 9027955 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/crashtests/crashtests.list
@@ -115,4 @@
>  load 382208-1.xhtml
>  load 382262-1.html
>  load 382396-1.xhtml
> -asserts(1) load 382745-1.xhtml # Bug 758695

you can remove just the `asserts(1)` and keep "load 382745-1.xhtml"

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: remove-treecols-2018-11-27-1851.diff

Comment 15

[Victor Porof [:vporof][:vp]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5c85a06b41c9e54aa85826a27b9a5f053edf862

Comment 16

[Victor Porof [:vporof][:vp]]

Green try.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9f8acdf5f7
Migrate the treecols binding into a custom element. r=bgrins

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4b9f8acdf5f7