Migrate the treebody binding into a custom element

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks: 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 months ago
Attachment #9021781 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

5 months ago
I forgot how hg treats renames. Unsmartly.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6423f6dfc7b0ca9c5c1b344ef13934d21c2ca989
Attachment #9021782 - Attachment is obsolete: true
Attachment #9021782 - Flags: review?(bgrinstead)
Attachment #9021783 - Flags: review?(bgrinstead)
Comment on attachment 9021783 [details] [diff] [review]
remove-treebody-2018-11-01-1218.diff

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

::: toolkit/content/customElements.js
@@ +296,4 @@
>      "chrome://global/content/elements/radio.js",
>      "chrome://global/content/elements/textbox.js",
>      "chrome://global/content/elements/tabbox.js",
> +    "chrome://global/content/elements/treecol.js",

As we discussed, I have a slight preference to keep all the tree stuff in a single js file (analogous to tree.xml). I don't feel super strongly about it one way or another - let's follow up on this later and make a decision.

::: toolkit/content/widgets/treechildren.js
@@ +123,5 @@
> +
> +      /**
> +       * double-click
> +       */
> +      this.addEventListener("dblclick", (event) => {

Are you sure you don't want to implement [clickcount] instead :)?

@@ +153,5 @@
> +
> +    connectedCallback() {
> +      this._lastSelectedRow = -1;
> +
> +      if ("_ensureColumnOrder" in this.parentNode)

I suspect we may want to skip this if isRunningDelayedConnectedCallback is true to avoid constructing XBL on the (potentially) hidden tree parent. Although unlike in treecol from first glance it seems like the parent binding ("tree") doesn't handle this case on it's own and is relying on the treechildren to do it even on first construction.

We'll have to do some testing with this, but I think we could have a call to `_ensureColumnOrder` in the tree <constructor>, and then either never call it here, or only call it here when !isRunningDelayedConnectedCallback, depending on if we want to support dynamically added treechildren.
Attachment #9021783 - Flags: review?(bgrinstead) → feedback+

Updated

4 months ago
Priority: -- → P3
(Assignee)

Comment 5

4 months ago
Attachment #9021783 - Attachment is obsolete: true
Attachment #9025564 - Flags: review?(bgrinstead)
Comment on attachment 9025564 [details] [diff] [review]
remove-treebody-2018-11-16-0719.diff

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

::: toolkit/content/widgets/tree.js
@@ +156,5 @@
> +      }
> +
> +      this._lastSelectedRow = -1;
> +
> +      if ("_ensureColumnOrder" in this.parentNode)

I was thinking that this would cause a perf regression (by accessing this.parentNode which is a tree it will force-construct the XBL binding). I'm not really seeing any regressions though, which is good news for future tree conversions (this implies we aren't getting hit too hard on the XBL construction portion of the tree which will typically run eagerly with Custom Elements).
Attachment #9025564 - Flags: review?(bgrinstead) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 8

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

Comment 9

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7a9ceb9becb
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.