1503826 - Remove the treerows binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Comment 1

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

Attachment: remove-treerows-2018-11-16-0923.diff

Brian, I can't hack my way around <children> here like in bug 1503824. How are these situations handled usually? (slots?)

Comment 2

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

Attachment: remove-treerows-2018-11-16-0959.diff

This sorta works but all a11y is lost.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #1)
> Created attachment 9025566 [details] [diff] [review]
> remove-treerows-2018-11-16-0923.diff
> 
> Brian, I can't hack my way around <children> here like in bug 1503824. How
> are these situations handled usually? (slots?)

Yes, in theory we can use slots although we haven't fleshed out exactly how that will work fully yet (both in terms of functionality and in terms of styling the anonymous content from the document, if necessary). I left an outline of how to do this in https://bugzilla.mozilla.org/show_bug.cgi?id=1503824#c8. If you can give it a shot that would help us figure out what we want to do with that.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 9025575 [details] [diff] [review]
remove-treerows-2018-11-16-0959.diff

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

::: toolkit/content/widgets/tree.js
@@ +461,5 @@
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }
> +
> +      this.textContent = "";

We shouldn't wipe out DOM here, right? If the goal is to not duplicate scrollbar appending then we can check for its existence before appending it again.

If you want to try emulating the current behavior without Shadow DOM, I think I'd try to copy the XBL structure, but with light DOM. So:

<hbox.tree-bodybox>
  <whatever-children-are-here-during-connectedCallback>
</hbox>
<scrollbar>

Depending on how JS consumers work (i.e. do they read or write to treerows.childNodes from JS?), we could either:

1) update them to append directly into the tree-bodybox
2) overwrite the common / used DOM APIs on this CE (i.e. make a custom appendChild that appends into the right place)
3) wire up a MutationObserver that detects childNodes being added into this node and "slots" them into the right place. This would likely need to be combined with 1 or 2 for consumers that read childNodes after appending

Comment 5

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Comment on attachment 9025575 [details] [diff] [review]
> remove-treerows-2018-11-16-0959.diff
> 
> If you want to try emulating the current behavior without Shadow DOM, I
> think I'd try to copy the XBL structure, but with light DOM. So:
> 

I think this is the perfect place to do this properly and use Shadow DOM, as opposed to bug 1503824.

Comment 6

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

Attachment: remove-treerows-2019-01-17-1056.diff

Poof!

Comment 7

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

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

Comment 8

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

Choppy but green.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 9037166 [details] [diff] [review]
remove-treerows-2019-01-17-1056.diff

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

::: toolkit/content/widgets/tree.xml
@@ +625,5 @@
> +          // Scrollport event orientation
> +          // 0: vertical
> +          // 1: horizontal
> +          // 2: both (not used)
> +          if (event.detail == 1)

I've done some testing with this (in Places UI by resizing the window to cause overflow on history items, for example), and it seems to work fine.

My only concern is if there's a case where "underflow" and "overflow" events would fire on non-treechildren elements within the tree markup (either anonymous nodes or inside the `<children includes="treecols"/>`), which wouldn't have been observed before, when these handlers were only added for <treerows>. If that happened, we'd be incorrectly showing and hiding the scrollbars for the tree as a whole.

Even though I haven't been able to reproduce this locally, I'm thinking we should add an `if (event.target.tagName != "treechildren") { return; }` check in these handlers to be sure we aren't changing the tree scrollbars based on some other element overflowing. Can you add that and double check the scrollbar still gets added/removed as expected? r=me with that change.

Comment 10

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

Makes sense! New patch incoming.

Comment 11

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

Attachment: remove-treerows-2019-01-18-1242.diff

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e7e2dac7c9
Remove the treerows binding, r=bgrins

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/47e7e2dac7c9

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/47e7e2dac7c9