Migrate the treebody binding into a custom element

RESOLVED FIXED in Firefox 65

Status

()

task
P3
normal
RESOLVED FIXED
9 months ago
2 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)

No description provided.
Attachment #9021781 - Flags: review?(bgrinstead)
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+
Priority: -- → P3
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c7a9ceb9becb
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.