Remove the treerows binding

RESOLVED FIXED in Firefox 66

Status

()

P5
normal
RESOLVED FIXED
4 months ago
16 days ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 months ago
Component: View Source → XUL Widgets

Updated

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

Comment 1

3 months ago
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?)
Attachment #9025566 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 2

3 months ago
Created attachment 9025575 [details] [diff] [review]
remove-treerows-2018-11-16-0959.diff

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

3 months ago
Depends on: 1507704
(Assignee)

Comment 5

3 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

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

Updated

a month ago
Attachment #9025566 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #9025575 - Attachment is obsolete: true
(Assignee)

Comment 6

a month ago

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

Poof!

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

Comment 8

a month 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

29 days ago

Makes sense! New patch incoming.

(Assignee)

Updated

29 days ago
Keywords: checkin-needed
(Assignee)

Updated

29 days ago
Status: NEW → ASSIGNED

Comment 12

29 days ago
Keywords: checkin-needed

Comment 13

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