Migrate the treecols binding into a custom element

RESOLVED FIXED in Firefox 65

Status

()

task
P5
normal
RESOLVED FIXED
8 months ago
14 days ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Depends on 1 bug, Blocks 1 bug, Regressed 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Assignee

Description

8 months ago
No description provided.
Assignee

Updated

8 months ago
Depends on: 1499423

Updated

8 months ago
Priority: -- → P5
Assignee

Comment 1

7 months ago
Attachment #9024775 - Flags: feedback?(bgrinstead)
Assignee

Updated

7 months ago
Attachment #9024775 - Attachment is obsolete: true
Attachment #9024775 - Flags: feedback?(bgrinstead)
Assignee

Comment 2

7 months ago
Hacky way to work around <children>.
Attachment #9025565 - Flags: review?(bgrinstead)
Assignee

Comment 3

7 months ago
In addition to comment 2, looks like delayConnectedCallback makes the tree blank so I removed that.
Attachment #9025573 - Flags: review?(bgrinstead)
Assignee

Updated

7 months ago
Attachment #9025565 - Attachment is obsolete: true
Attachment #9025565 - Flags: review?(bgrinstead)
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?
ni? for Comment 4
Flags: needinfo?(vporof)
Assignee

Comment 6

7 months ago
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.
Attachment #9025573 - Attachment is obsolete: true
Attachment #9025573 - Flags: review?(bgrinstead)
Flags: needinfo?(vporof)
Attachment #9026314 - Flags: review?(bgrinstead)
Assignee

Comment 7

7 months ago
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.
Attachment #9026314 - Attachment is obsolete: true
Attachment #9026314 - Flags: review?(bgrinstead)
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).
Assignee

Comment 9

7 months ago
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!
Attachment #9026915 - Flags: review?(bgrinstead)
Assignee

Updated

7 months ago
Attachment #9026329 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Blocks: 1507704
(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=
Flags: needinfo?(jorgk)
Attachment #9026915 - Flags: review?(bgrinstead) → review+

Updated

7 months ago
Depends on: 1509982

Comment 11

7 months ago
Thanks for the heads-up, I filed bug 1509982.
Flags: needinfo?(jorgk)
Assignee

Comment 12

7 months ago
Fixed test as per https://bugzilla.mozilla.org/show_bug.cgi?id=1507704#c13
Attachment #9026915 - Attachment is obsolete: true
Attachment #9027955 - Flags: review+
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"
Assignee

Updated

7 months ago
Attachment #9027955 - Attachment is obsolete: true
Attachment #9027955 - Flags: review+
Assignee

Comment 16

7 months ago
Green try.
Keywords: checkin-needed

Comment 17

7 months ago
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9f8acdf5f7
Migrate the treecols binding into a custom element. r=bgrins
Keywords: checkin-needed

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b9f8acdf5f7
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65

Updated

7 months ago
Depends on: 1510616

Updated

3 months ago
Depends on: 1535852

Updated

2 months ago
Regressions: 1543036

Updated

15 days ago
Type: enhancement → task
Regressions: 1557285
You need to log in before you can comment on or make changes to this bug.