Remove the treerows binding

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P5
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 months ago
Component: View Source → XUL Widgets

Updated

6 months ago
Priority: -- → P5
(Assignee)

Comment 1

5 months ago
Brian, I can't hack my way around <children> here like in bug 1503824. How are these situations handled usually? (slots?)
Attachment #9025566 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 2

5 months ago
This sorta works but all a11y is lost.
Attachment #9025575 - Flags: feedback?(bgrinstead)
(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.
Attachment #9025566 - Flags: feedback?(bgrinstead)
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
Attachment #9025575 - Flags: feedback?(bgrinstead)
(Assignee)

Updated

5 months ago
Depends on: 1507704
(Assignee)

Comment 5

5 months ago
(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.
(Assignee)

Updated

3 months ago
Summary: Migrate the treerows binding into a custom element → Remove the treerows binding
(Assignee)

Updated

3 months ago
Attachment #9025566 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #9025575 - Attachment is obsolete: true
(Assignee)

Comment 6

3 months ago

Poof!

Attachment #9037166 - Flags: review?(bgrinstead)
(Assignee)

Comment 8

3 months ago

Choppy but green.

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.
Attachment #9037166 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 10

3 months ago

Makes sense! New patch incoming.

(Assignee)

Updated

3 months ago
Keywords: checkin-needed
(Assignee)

Updated

3 months ago
Status: NEW → ASSIGNED

Comment 12

3 months ago
Keywords: checkin-needed

Comment 13

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.