Bug 1523957 (deforestation)

Convert trees to custom elements

RESOLVED FIXED in Firefox 67

Status

()

P3
normal
RESOLVED FIXED
2 months ago
10 days ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(13 attachments, 16 obsolete attachments)

1.15 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
27.62 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
702 bytes, patch
bgrins
: review+
Details | Diff | Splinter Review
4.20 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
3.62 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
96.00 KB, patch
bgrins
: review+
mak
: review+
Details | Diff | Splinter Review
2.60 KB, patch
vporof
: review+
standard8
: review+
Details | Diff | Splinter Review
1.98 KB, patch
vporof
: review+
Details | Diff | Splinter Review
1006 bytes, patch
vporof
: review+
Details | Diff | Splinter Review
3.78 KB, patch
mak
: review+
Details | Diff | Splinter Review
7.45 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
968 bytes, patch
bgrins
: review+
standard8
: review+
Details | Diff | Splinter Review
132.28 KB, patch
vporof
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

2 months ago

So this was an adventure.

There's a couple of things going on with this patch.

  1. The XBL binding code was left in place to prevent places-tree from blowing up when attempting to extend the XBL-version of tree.
  2. The XBL binding was noneified for the CE-version of the tree in CSS.
  3. Children elements were replaced with html:slots
  4. Treecols set their slot name when connected to get slotted in the intended place

However things still blow up. It looks like nothing is actually slotted in place. I tried an approach where I'm just using regular <children> nodes inside trees, and nothing is appended in that scenario either.

The immediately obvious consequences are:

  1. Trees are empty.
  2. The "focus" and "blur" events throw errors about "view" and "inputField" being null for the entire tree. This is probably caused by (1)
  3. The in-content preferences page throws an error about engineList.inputField being null for [0]. This looks an input for tree cells which are editable (via overriding the isEditable method). This is likely the same problem as (2)

Quite stuck. Looking for feedback.

[0] https://docs.google.com/document/d/1lrWIAKTGfyO8_FaAScUc9V57jyZdM-odig00MmFh9HA/edit#heading=h.k6pk4f8lm0j2

Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #9040129 - Flags: feedback?(bgrinstead)

I didn't get a chance to apply this today - aiming for tomorrow. In the meantime, what happens if you take Shadow DOM out of the equation and manually "slot" the elements during connection using appendChild, etc? I know there's some downsides with that approach, but I'd be interested if it gets us past these current issues (as a way to narrow down if it's just Shadow DOM causing them).

Comment on attachment 9040129 [details] [diff] [review]
remove-tree-2019-01-30-1705.diff

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

::: toolkit/content/widgets/tree.js
@@ +525,4 @@
>  
>    customElements.define("treecols", MozTreecols);
>  
> +  class MozTree extends MozElementMixin(XULTreeElement) {

This should extend: `BaseControlMixin(MozElementMixin(XULTreeElement))` since tree XBL extends basecontrol. This will let the element QueryInterface to Ci.nsIDOMXULControlElement.
Comment on attachment 9040129 [details] [diff] [review]
remove-tree-2019-01-30-1705.diff

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

::: toolkit/content/widgets/tree.js
@@ +875,5 @@
> +        <html:slot name="treecols"></html:slot>
> +        <stack class="tree-stack" flex="1">
> +          <hbox class="tree-rows" flex="1">
> +            <hbox flex="1" class="tree-bodybox">
> +              <html:slot></html:slot>

html:slot would only work if this was inside of a shadow root (i.e. `this.attachShadow({mode: 'open'})`)

@@ +962,5 @@
> +    }
> +
> +    get inputField() {
> +      if (!this._inputField)
> +        this._inputField = document.getAnonymousElementByAttribute(this, "anonid", "input");

This getter needs to be updated to querySelector to the actual input (either inside of the node's light or shadow DOM depending on how we implement it).

I see errors like chrome://browser/content/aboutSessionRestore.js, line 110: TypeError: tabList.view is null

which I think is ultimately due to FindBodyElement returning null 0, which short circuits XULTreeElement::GetView 1. I'm not familiar with the different iterators, but it looks like the FlattenedChildIterator is intended to support XBL anonymous content, so we may need to update that to something like a ShadowIncludingTreeIterator. I'll push up a WIP with the various fixes I suggested on the patch, so we can ask someone who would know better how to handle this.

Neil pointed out that we are getting the error in Comment 5 because treechildren had display: none from xul.css: https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/toolkit/content/xul.css#64

And this wasn't matching anymore: https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/toolkit/content/xul.css#441.

With the patch I've just pushed to phab I see working trees! Still need to implement attribute inheriting and do more testing, but looks like this should work.

Attachment #9040129 - Flags: feedback?(bgrinstead)

Alright, newest version on phab is working much better. Did some manual testing on session restore and page info UIs.

I'm not sure if the change in XULTreeElement.cpp is necessary - the patch seems to work fine without it. Emilio, when I was chasing down an issue that turned out to be unrelated, I converted the FlattenedChildIterator to a ShadowIncludingTreeIterator because my impression was that FlattenedChildIterator was for XBL anonymous content. Now that I look closer, <treechildren> will be in the light DOM in either the XBL or CE+SD case, so maybe it doesn't matter which iterator we use. Could you have a look at the change in XULTreeElement.cpp at https://phabricator.services.mozilla.com/D18626 and see if it matters one way or another?

Flags: needinfo?(emilio)

I don't understand, FlattenedTreeIterator handles shadow DOM already. ShadowIncludingTreeIterator is something fairly different, and will return, e.g., unassigned nodes. That's not what you want, so I'd revert that part of the patch.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

I don't understand, FlattenedTreeIterator handles shadow DOM already. ShadowIncludingTreeIterator is something fairly different, and will return, e.g., unassigned nodes. That's not what you want, so I'd revert that part of the patch.

Thanks for the heads up, I've reverted the change.

Let's see how orange it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e041349bd81671b09de099d44677fd16094a64.

Of course we need to migrate places-tree as well: https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/PlacesTree.js. I'd be fine to dump that into tree.js (at least for now). You should be able to just extend MozTree and not worry about the mixin.

As for how to register and use it, this is an example of how <richlistitem is="autocomplete-richlistitem"> gets hooked up to a CE: https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/toolkit/content/widgets/autocomplete-richlistitem.js#1020-1022. So https://github.com/bgrins/xbl-analysis/blob/f570ba3af582473f9bc4ce2b1136b3d486623b83/elements/generated/PlacesTree.js#L782 would have to be adjusted. And we'd have to update <tree type="places"> (https://searchfox.org/mozilla-central/search?q=type%3D%22places%22&path=).

(Assignee)

Comment 12

2 months ago

Looks like it's pretty orange.

Having a look now, and migrating places-tree to CE.

(In reply to Victor Porof [:vporof][:vp] from comment #12)

Looks like it's pretty orange.

It's not as bad as I thought it would be - those "mda" failures are unrelated (they are perma orange in artifact builds).

As for the a11y failures ("Can't get accessible for 'tree'"), I wonder if there's some flag or a check in https://searchfox.org/mozilla-central/source/accessible/xul/XULTreeAccessible.cpp that's expecting XBL. Tim pointed out some changes needed for menuitem (changing a flag and setting role=none to children) at https://bugzilla.mozilla.org/show_bug.cgi?id=1519495#c2. Alex, could you help figure out what's going on with these (https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e041349bd81671b09de099d44677fd16094a64&selectedJob=226392597)?

Flags: needinfo?(surkov.alexander)

(In reply to Brian Grinstead [:bgrins] from comment #13)

(In reply to Victor Porof [:vporof][:vp] from comment #12)

Looks like it's pretty orange.

It's not as bad as I thought it would be - those "mda" failures are unrelated (they are perma orange in artifact builds).

As for the a11y failures ("Can't get accessible for 'tree'"), I wonder if there's some flag or a check in https://searchfox.org/mozilla-central/source/accessible/xul/XULTreeAccessible.cpp that's expecting XBL.

here's the logic where we decide if XUL:tree is accessible (https://searchfox.org/mozilla-central/source/accessible/base/XULMap.h#94). It expects treechildren element having nsTreeBodyFrame frame. Is this condition still true?

Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 15

2 months ago

(In reply to Brian Grinstead [:bgrins] from comment #11)

Let's see how orange it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e041349bd81671b09de099d44677fd16094a64.

Of course we need to migrate places-tree as well: https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/PlacesTree.js. I'd be fine to dump that into tree.js (at least for now). You should be able to just extend MozTree and not worry about the mixin.

As for how to register and use it, this is an example of how <richlistitem is="autocomplete-richlistitem"> gets hooked up to a CE: https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/toolkit/content/widgets/autocomplete-richlistitem.js#1020-1022. So https://github.com/bgrins/xbl-analysis/blob/f570ba3af582473f9bc4ce2b1136b3d486623b83/elements/generated/PlacesTree.js#L782 would have to be adjusted. And we'd have to update <tree type="places"> (https://searchfox.org/mozilla-central/search?q=type%3D%22places%22&path=).

Just to be clear here, <tree type="places"> needs to become <places-tree>?

(Assignee)

Updated

2 months ago
Flags: needinfo?(bgrinstead)

(In reply to Victor Porof [:vporof][:vp] from comment #15)

Just to be clear here, <tree type="places"> needs to become <places-tree>?

<tree type="places"> needs to become <tree is="places-tree">. This will make it so the "places-tree" custom element gets attached to it, instead of "tree".

Flags: needinfo?(bgrinstead)
Attachment #9040129 - Attachment is obsolete: true

This patch isn't ready to land, but it's probably worth doing some early testing on TB to make sure it's working for you.

One thing that will probably look wrong is that we're trying to use Shadow DOM, and that means that the anonymous content like .tree-stack, .tree-rows, .tree-bodybox, .tree-input can't be styled from document sheets. Basically anything in here: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.xml#18-45. We need to do some more analysis to decide if we need that feature for m-c, last time I checked we barely touch those styles but I believe c-c does. There's a CSS feature called Shadow Parts that fixes this (tracked for chrome in https://bugzilla.mozilla.org/show_bug.cgi?id=1505489), but it won't land before this does. The alternative way to pass down styles would be to load a SD scoped stylesheet and set CSS variables on the element, then reference the variables in the sheet. Or moving styles onto other elements / dropping them if they aren't necessary.

Flags: needinfo?(mkmelin+mozilla)

I'm not 100% sure if we'll end up using Shadow DOM or light DOM and moving around child nodes. I'd prefer SD if it works.

Pushed up a new version to phab that fixed keydown navigation handlers to skip running the "shift" variations of unless if shift was pressed. Fixes at least accessible/tests/mochitest/events/test_tree.xul for me locally. Although the weird thing is I don't see that "Can't get accessible for 'tree'" error locally when running it - maybe it's not running the same checks on OSX.

Updated the patch to load tree.css in Shadow DOM (a bit redundant with the document sheet loading it, but that allows rules like https://searchfox.org/mozilla-central/rev/490ab7f9b84570573a49d7fa018673ce0d5ddf22/toolkit/themes/osx/global/tree.css#10-14 to apply to the now-anonymous content.

Try looks almost green here. We now need to get places-tree imported (directly into tree.js is fine for testing) and places consumers updated as per Comment 16.

Then before landing we should make a decision on where the places tree implementation should ultimately live and how it should be loaded. I'd be sort of OK to just leave it in tree.js, but most places stuff is in https://searchfox.org/mozilla-central/source/browser/components/places/content, so we could consider putting it there and then loading it explicitly from documents that load places.css: https://searchfox.org/mozilla-central/search?q=places.css&path=. We don't currently have any code running that's the analog of CustomElementsListener but for browser/. That is: a place we can use subscript loader for all documents, but only in browser/. Let's first get a green build and then figure this out then.

Depends on: 1526320

Comment on attachment 9042322 [details]
Bug 1523957 - Convert DevTools XBL test to use a test binding instead of a xul:tree;r=pbro

Revision D19087 was moved to bug 1526320. Setting attachment 9042322 [details] to obsolete.

Attachment #9042322 - Attachment is obsolete: true
(Assignee)

Comment 25

a month ago

(In reply to Brian Grinstead [:bgrins] from comment #23)

Try looks almost green here. We now need to get places-tree imported (directly into tree.js is fine for testing) and places consumers updated as per Comment 16.

Then before landing we should make a decision on where the places tree implementation should ultimately live and how it should be loaded. I'd be sort of OK to just leave it in tree.js, but most places stuff is in https://searchfox.org/mozilla-central/source/browser/components/places/content, so we could consider putting it there and then loading it explicitly from documents that load places.css: https://searchfox.org/mozilla-central/search?q=places.css&path=. We don't currently have any code running that's the analog of CustomElementsListener but for browser/. That is: a place we can use subscript loader for all documents, but only in browser/. Let's first get a green build and then figure this out then.

I also think it's ok landing places-tree inside toolkit/../tree.js for now. If we really need an artificial separation between the two, it should be straight forward.

(Assignee)

Comment 26

a month ago

There's also a few other small things left to do, but looking for some quick feedback.

Attachment #9041343 - Attachment is obsolete: true
Attachment #9042515 - Flags: feedback?(bgrinstead)

(In reply to Victor Porof [:vporof][:vp] from comment #25)

I also think it's ok landing places-tree inside toolkit/../tree.js for now. If we really need an artificial separation between the two, it should be straight forward.

It's not totally artificial (any more than the browser and toolkit separation is). For instance, if we hg mv to a places-tree.js inside of browser/components/places/content it would remain next to the associated styles and JS. The trade off is we'd have to add a new mechanism for loading the script in nsBrowserGlue instead of relying on MainProcessSingleton / customElements.js to load it.

Comment on attachment 9042515 [details] [diff] [review]
remove-tree-latest-2019-02-08-1742.diff

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

The places trees don't seem to work. The Custom Element class is getting constructed fine, but the places UI is empty with the following error:

TypeError: this.currentView is null[Learn More] places.js:1215:5
    get currentPlace chrome://browser/content/places/places.js:1215
    get currentViewOptions chrome://browser/content/places/places.js:1261
    CA__setupView chrome://browser/content/places/places.js:1234
    CA_init chrome://browser/content/places/places.js:1147
    PO_init chrome://browser/content/places/places.js:145
    onload chrome://browser/content/places/places.xul:1
Attachment #9042515 - Flags: feedback?(bgrinstead)

I thought about Comment 23 more, and I'm leaning towards doing hg mv browser/components/places/content/tree.xml browser/components/places/content/places-tree.js, and then anywhere that places.css is loaded (https://searchfox.org/mozilla-central/search?q=places.css&regexp=true&path=) going ahead and loading a script to chrome://browser/content/places/places-tree.js. Then instead of class MozPlacesTree extends MozTree it would be class MozPlacesTree extends customElements.get("tree") (which is the same class).

By explicitly loading scripts it will avoid us having to wire up some new auto-loading mechanism as per Comment 28. The other benefits I'm thinking in comparison vs loading it all in tree.js:

  1. Keep the source next to the rest of the places implementation (better for code ownership and organization)
  2. Keep hg history on the file
(Assignee)

Updated

a month ago
Alias: deforestation
(Assignee)

Comment 33

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

Comment 36

a month ago

I've split things up a bit for sanity, still mowing over failures.

(Assignee)

Comment 37

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

Comment 43

a month ago

Currently failing tests without the places-tree conversion:
1- browser/components/places/tests/browser/browser_bookmarkProperties_newFolder.js | Test timed out
2- toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js | Uncaught exception - Waiting for editing to stop
3a browser/base/content/test/performance/browser_startup.js | nsPlacesExpiration.js is not allowed before handling user events
3b browser/base/content/test/performance/browser_startup.js | PlacesUtils.jsm is not allowed before first paint

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5b43014e466b2453c086a0e7090f1f6576066c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1afa625e224e79f94099ce1079065e5a6a7b2a

Looking to see how things look like with the places-tree conversion. Locally, it looks like (1) is fixed at least.

(Assignee)

Comment 44

a month ago

Currently failing tests with the places-there conversion:

2- toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js | Uncaught exception - Waiting for editing to stop
3a browser/base/content/test/performance/browser_startup.js | nsPlacesExpiration.js is not allowed before handling user events
3b browser/base/content/test/performance/browser_startup.js | PlacesUtils.jsm is not allowed before first paint
4- browser/components/places/tests/browser/browser_bookmarkProperties_no_user_actions.js | this._controller is undefined
5- browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files

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

So indeed, it looks like (1) was automagically fixed, and (4) and (5) newly introduced.

So (5) is probably just a matter of deleting toolkit/content/widgets/tree.xml from disk and any reference from toolkit/content/jar.mn.

My best guess on 3a and 3b is that since we have a places tree in the browser.xul markup, we are attaching the CE to it and loading some modules as a result. I actually tried last week a push to remove it and make it constructed lazily to avoid this problem: https://hg.mozilla.org/try/rev/1496a081f68a33a18a1daaa7957d6d0470f1a7a9. There was one bc test still failing, but maybe try applying that patch on top of yours and see if 3a and 3b go away.

(In reply to Brian Grinstead [:bgrins] from comment #46)

My best guess on 3a and 3b is that since we have a places tree in the browser.xul markup, we are attaching the CE to it and loading some modules as a result. I actually tried last week a push to remove it and make it constructed lazily to avoid this problem: https://hg.mozilla.org/try/rev/1496a081f68a33a18a1daaa7957d6d0470f1a7a9. There was one bc test still failing, but maybe try applying that patch on top of yours and see if 3a and 3b go away.

Presumably this line would have to change to the [is] syntax: https://hg.mozilla.org/try/rev/1496a081f68a33a18a1daaa7957d6d0470f1a7a9#l1.19. You can test locally if it's working by bookmarking a page and then expanding the folder area and make sure the tree shows up.

(Assignee)

Comment 51

a month ago

Still failing with the new try:

2- toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js | Uncaught exception - Waiting for editing to stop
3a browser/base/content/test/performance/browser_startup.js | nsPlacesExpiration.js is not allowed before handling user events
3b browser/base/content/test/performance/browser_startup.js | PlacesUtils.jsm is not allowed before first paint
4- browser/components/places/tests/browser/browser_bookmarkProperties_no_user_actions.js | this._controller is undefined

So (5) was indeed fixed, it was just parma-orange locally.

Eslint issues are all also fixed (see "Part 2.2" which had to add a new mapping).

(Assignee)

Comment 52

a month ago

The patch in comment 46 does indeed fix (3).

Currently failing with all the patches in this bug as well as the patch in comment 46:

2- toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js | Uncaught exception - Waiting for editing to stop
4- browser/components/places/tests/browser/browser_bookmarkProperties_no_user_actions.js | this._controller is undefined

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38edf968fc9f5cc14aec3d4c6a999e3a4d70fc21

(Assignee)

Comment 53

a month ago

This should fix (2). So there's just one more test failure remaining.

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace548ef217487231b2bff9ec860c9ace803fb41

(Assignee)

Updated

a month ago
Attachment #9043551 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043551 - Attachment description: Part 0: Pre → Part 0: Pre, r=bgrins
(Assignee)

Updated

a month ago
Attachment #9043563 - Attachment description: Part 1.1: Naively convert tree to a custom element → Part 1.1: Naively convert tree to a custom element, r=bgrins
Attachment #9043563 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043564 - Attachment description: Part 1.2: Properly mixin BaseControl and extend XULTreeElement → Part 1.2: Properly mixin BaseControl and extend XULTreeElement, r=bgrins
Attachment #9043564 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043565 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043566 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043567 - Attachment description: Part 2.1: Naively convert places-tree to a custom element, preserving history → Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak
Attachment #9043567 - Flags: review?(mak77)
Attachment #9043567 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043632 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043636 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043640 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043803 - Attachment description: Part 2.4: Fix browser_passwordmgr_editing.js, r=bgrins → Part 2.4: Fix browser_passwordmgr_editing.js, r=bgrins, mak
Attachment #9043803 - Flags: review?(mak77)
Attachment #9043803 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043906 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9043907 - Attachment description: Part 2.6: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, r=bgrins → Part 2.6: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, author=bgrins
(Assignee)

Comment 56

a month ago

All failures should be fixed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=665dd2ea26c8ca8a0dd94cc0498352001c99f854

For reviews, please follow the part numbers, they're the intended order for landing and review.

Comment on attachment 9043567 [details] [diff] [review]
Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

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

::: browser/components/places/PlacesUIUtils.jsm
@@ +345,4 @@
>      while (Element.isInstance(node)) {
>        if (node._placesView)
>          return node._placesView;
> +      if (node.localName == "tree" && node.getAttribute("is") == "places-tree")

Something I realized recently in another Custom Element bug that's a bit unfortunate is that when you create an element like:

document.createElement("tree", { is: "places-tree" });

It's a perfectly valid way to instantiate a Custom Element, but it won't put the [is] attribute on the node. So checks like this (and the CSS attr lookups) wouldn't work.

I think it's probably OK to handle it this way for places tree, since we aren't planning to add new ones and all the ones we have are defined in markup (except for possibly the one that we are planning to lazify out of browser.xul). But options we could think about would be:

1) Add a separate attribute in the connectedCallback (i.e. [type=places]) and then rely on that from JS/CSS
2) Set the [is] attribute in the connectedCallback if it's not already set to handle the 'created from js' case. I'm not sure if that's allowed or would cause other issues.

I think I'd prefer (2) if it works. It's possible I'm also not thinking of another option. WDYT?
Posted patch remove-tree-rollup.diff (obsolete) — Splinter Review

Magnus, Jorg, see Comment 17. Here's a rollup patch with the Custom Elementification for <xul:tree>. You probably want to do some preliminary testing before this lands to get a sense for what (if anything) will need to be ported.

Flags: needinfo?(jorgk)

Comment 59

a month ago

Thanks, Magnus and team will take care of it.

Flags: needinfo?(jorgk) → needinfo?(richard.marti)
Comment on attachment 9043567 [details] [diff] [review]
Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

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

::: browser/base/content/browser.xul
@@ +104,4 @@
>    Services.scriptloader.loadSubScript("chrome://browser/content/browser-tabsintitlebar.js", this);
>    Services.scriptloader.loadSubScript("chrome://browser/content/tabbrowser.js", this);
>    Services.scriptloader.loadSubScript("chrome://browser/content/search/searchbar.js", this);
> +  Services.scriptloader.loadSubScript("chrome://browser/content/places/places-tree.js", this);

To limit the impact on startup, let's drop this from here and move it into Part 2.6 right before the tree gets created in editBookmark.js.

I'm doing the port of bug 1527495 so Magnus could do this bug.

Flags: needinfo?(richard.marti)
Comment on attachment 9043907 [details] [diff] [review]
Part 2.6: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, author=bgrins

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

The test failure you are seeing can be fixed by replacing this line https://searchfox.org/mozilla-central/rev/38035ee92463c9e9fbca729ac7e66476ac7eb27a/browser/components/places/tests/browser/browser_bookmarksProperties.js#285 with:

var folderTree = this.window.gEditItemOverlay._folderTree;

::: browser/components/places/content/editBookmark.js
@@ +1078,5 @@
>    onItemVisited() { },
>  };
>  
> +XPCOMUtils.defineLazyGetter(gEditItemOverlay, "_folderTree", () => {
> +  // XXX: Subscript load places-tree.js if customElements.get("places-tree") isn't defined

As per Comment 60, please do:

Services.scriptloader.loadSubScript("chrome://browser/content/places/places-tree.js", window);

Or if we want to be careful:

```
if (!customElements.get("places-tree")) {
  Services.scriptloader.loadSubScript("chrome://browser/content/places/places-tree.js", window);
}
```
Comment on attachment 9043803 [details] [diff] [review]
Part 2.4: Fix browser_passwordmgr_editing.js, r=bgrins, mak

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

r=me if you switch it back to capturing, otherwise, we can discuss.

::: toolkit/content/widgets/tree.js
@@ +973,2 @@
>          this._inputField = this.shadowRoot.querySelector("[anonid=input]");
> +        this._inputField.addEventListener("blur", () => this.stopEditing(true));

This was a capturing event handler before, is there a reason to change it to bubbling?
Attachment #9043803 - Flags: review?(mak77) → review+
Comment on attachment 9043803 [details] [diff] [review]
Part 2.4: Fix browser_passwordmgr_editing.js, r=bgrins, mak

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

MattN's review is enough, clearing my flag
Attachment #9043803 - Flags: review?(bgrinstead)
Comment on attachment 9043906 [details] [diff] [review]
Part 2.5: Fix browser_bookmarkProperties_no_user_actions.js, r=bgrins

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

This is just applying https://hg.mozilla.org/mozilla-central/rev/bb7602ef747f on top of the places Custom Element, right? Please fold this directly into 2.1 (which is where it would go if we had happened to run the converter tool after that changeset had hit m-c).
Attachment #9043906 - Flags: review?(bgrinstead) → review-
Attachment #9043551 - Flags: review?(bgrinstead) → review+
Attachment #9043564 - Flags: review?(bgrinstead) → review+
Comment on attachment 9043565 [details] [diff] [review]
Part 1.3: Use Shadow DOM for slotting children, r=bgrins

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

::: toolkit/content/widgets/tree.js
@@ +995,4 @@
>  
>      get inputField() {
>        if (!this._inputField)
> +        this._inputField = this.shadowRoot.querySelector("[anonid=input]");

While we are here we may as well remove the anonid attribute from this element and `return this.shadowRoot.querySelector(".tree-input")`

::: toolkit/themes/shared/in-content/common.inc.css
@@ -385,4 @@
>    padding-left: 8px;
>  }
>  
> -/* Create a separate rule to unset these styles on .tree-input instead of

We need to do some local testing with trees in docs that load this file (for instance about:preferences). These rules in particular wouldn't have applied anyway (nor would the rules that set these properties which are being unset). So that's all fine. I'm not sure, though, if there are other CSS rules intended to apply to xul textboxes that _are_ important. And if so, how to pass those along to the anon content (via a class and tree.css, CSS variables, etc).

Could you do a bit of checking around to see if we ever render inputs in our known in-content tree consumers, and if so if they look different before/after this patch?
Attachment #9043565 - Flags: review?(bgrinstead)
Attachment #9043566 - Flags: review?(bgrinstead) → review+
Attachment #9043632 - Flags: review?(bgrinstead) → review+
Attachment #9043636 - Flags: review?(bgrinstead) → review+
Comment on attachment 9043640 [details] [diff] [review]
Part 2.3: Remove content/global/bindings/tree.xml and all references, r=bgrins

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

I think I'd cherry pick the tree.xml changes from 2.1 into this patch instead, to make that patch smaller and allow this one to be in charge of removing the file.
Attachment #9043640 - Flags: review?(bgrinstead) → review-
Comment on attachment 9043563 [details] [diff] [review]
Part 1.1: Naively convert tree to a custom element, r=bgrins

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

Looks like a direct import of https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Tree.js, that's fine
Attachment #9043563 - Flags: review?(bgrinstead) → review+
(Assignee)

Updated

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

Comment 69

a month ago

Addressed comments.

(Assignee)

Updated

a month ago
Attachment #9043567 - Attachment is obsolete: true
Attachment #9043567 - Flags: review?(mak77)
Attachment #9043567 - Flags: review?(bgrinstead)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 75

a month ago

(In reply to Brian Grinstead [:bgrins] from comment #66)

While we are here we may as well remove the anonid attribute from this
element and return this.shadowRoot.querySelector(".tree-input")

Sure.

::: toolkit/themes/shared/in-content/common.inc.css
@@ -385,4 @@

padding-left: 8px;
}

Could you do a bit of checking around to see if we ever render inputs in our
known in-content tree consumers, and if so if they look different
before/after this patch?

Will do. Everything else looks ok otherwise?

(Assignee)

Updated

a month ago
Attachment #9044172 - Flags: review?(bgrinstead)
(Assignee)

Updated

a month ago
Attachment #9044172 - Attachment is obsolete: true
Attachment #9044172 - Flags: review?(bgrinstead)
Comment on attachment 9044188 [details] [diff] [review]
Part 2.6: Add eslint globals to places-tree.js, r=bgrins, standard8

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

This seems fine with me, but I want to get Mark's input as well. Mark, do XBL files not get linted for globals? I'm wondering why this is needed now and just want to make sure that https://searchfox.org/mozilla-central/source/browser/components/places/content/tree.xml wasn't somehow being set up into a linting environment that we should be adding this file into.

::: browser/components/places/content/places-tree.js
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global PlacesController */

Nit: it seems pretty common to oneline this - https://searchfox.org/mozilla-central/search?q=%2F*+global&path=js
Attachment #9044188 - Flags: review?(standard8)
Attachment #9044188 - Flags: review?(bgrinstead)
Attachment #9044188 - Flags: review+
Comment on attachment 9044178 [details] [diff] [review]
Part 2.5: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, r=bgrins, mak

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

The tree.js changes are fine (though will become obsolete once Bug 1527680 lands). Forwarding the places/ review to Marco.
Attachment #9044178 - Flags: review?(bgrinstead) → review?(mak77)
(Assignee)

Updated

a month ago
Attachment #9044175 - Flags: superreview?(standard8)
Comment on attachment 9044175 [details] [diff] [review]
Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins, standard8

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

Forwarding review to Mark for the browser-window.js change
Attachment #9044175 - Flags: superreview?(standard8) → review?(standard8)
(Assignee)

Updated

a month ago
Attachment #9044188 - Attachment description: Bug 1523957 - Part 2.6: Add eslint globals to places-tree.js, r=bgrins → Part 2.6: Add eslint globals to places-tree.js, r=bgrins

(In reply to Brian Grinstead [:bgrins] from comment #58)

Created attachment 9043961 [details] [diff] [review]
remove-tree-rollup.diff

Magnus, Jorg, see Comment 17. Here's a rollup patch with the Custom Elementification for <xul:tree>. You probably want to do some preliminary testing before this lands to get a sense for what (if anything) will need to be ported.

I tried the rollup patch with TB and the 3-pane window looked like with the XBL-tree and I saw no to this related errors in the console. I wasn't able to try the calendar views as Lightning is actually broken but I think this works too.

This is very promising.

(Assignee)

Comment 83

a month ago

\o/

See Also: → bug 760974
Comment on attachment 9044173 [details] [diff] [review]
Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

I think the flag for Marco got dropped on the new version
Attachment #9044173 - Flags: review?(mak77)
Comment on attachment 9044175 [details] [diff] [review]
Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins, standard8

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

Yeah, browser-window.js is fine here.
Attachment #9044175 - Flags: review?(standard8) → review+
Comment on attachment 9044188 [details] [diff] [review]
Part 2.6: Add eslint globals to places-tree.js, r=bgrins, standard8

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

::: browser/components/places/content/places-tree.js
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* global PlacesController */

I think slightly nicer here would be to do:

```
/* import-globals-from controller.js */
/* import-globals-from treeView.js */
```
(and drop all the individual global definitions).

That gives people an indication that those two files are required alongside this one (there's more, but they declare what they depend on as well).

r=me with that fixed.
Attachment #9044188 - Flags: review?(standard8) → review+
Comment on attachment 9044178 [details] [diff] [review]
Part 2.5: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, r=bgrins, mak

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

::: browser/components/places/content/editBookmark.js
@@ +1106,3 @@
>                   "locationField", "keywordField",
>                   "tagsField" ]) {
>    let eltScoped = elt;

while here, could you please remove eltScoped? It became unnecessary a couple years ago, when JS was fixed to create a new scope per loop. We can just use elt in the loop.
nit: The space at the end of the array before ] is also unnecessary.
Attachment #9044178 - Flags: review?(mak77) → review+
Priority: -- → P3
Comment on attachment 9044173 [details] [diff] [review]
Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

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

Awesome, it seems to work as expected. There's a ton of simplifications and fixes we can make by merging this and treeview.js (per bug 760974)

I'd suggest an eslint check over this, since it seems to have some code style misses.

::: browser/components/places/content/places-tree.js
@@ +87,4 @@
>            // If the row is not valid we try to insert inside the resultNode.
>            orientation = Ci.nsITreeView.DROP_ON;
>          } else if (PlacesUtils.nodeIsContainer(node) &&
> +          eventY > rowHeight * 0.75) {

were some of the changes made automatically?
These if conditions reindentations around are not great... There are multiple

@@ +123,5 @@
> +      // Force an initial build.
> +      if (this.place)
> +        this.place = this.place;
> +
> +    }

please remove empty newline before braces (I think this may actually fail eslint)

@@ +131,5 @@
> +    }
> +
> +    set disableUserActions(val) {
> +      if (val) this.setAttribute('disableUserActions', 'true');
> +      else this.removeAttribute('disableUserActions');

please no inline if/else bodies

@@ +138,5 @@
> +
> +    get disableUserActions() {
> +      return this.getAttribute('disableUserActions') == 'true';
> +    }
> +    /**

missing newline between methods

@@ +159,5 @@
> +      }
> +    }
> +
> +    get associatedElement() {
> +      return this

missing semicolon

@@ +186,5 @@
> +      } catch (e) {
> +        return null;
> +      }
> +    }
> +    /**

please always add a newline between methods (here and below, please make this consistent across the file), even if they are related to an interface or such.

@@ +378,5 @@
> +    }
> +
> +    get active() {
> +      return this._active
> +    }

move these to the top, closer to connectedCallback, and check missing semicolons around in these getters
Attachment #9044173 - Flags: review?(mak77) → review+

(In reply to Marco Bonardo [::mak] from comment #88)

Comment on attachment 9044173 [details] [diff] [review]
Part 2.1: Naively convert places-tree to a custom element, preserving
history, r=bgrins, mak

Review of attachment 9044173 [details] [diff] [review]:

Awesome, it seems to work as expected. There's a ton of simplifications and
fixes we can make by merging this and treeview.js (per bug 760974)

Yeah, once we get this into JS I expect there's a bunch of cleanup that can be done (we saw this with the tabbrowser XBL -> JS converstion as well).

I'd suggest an eslint check over this, since it seems to have some code
style misses.

::: browser/components/places/content/places-tree.js
@@ +87,4 @@

       // If the row is not valid we try to insert inside the resultNode.
       orientation = Ci.nsITreeView.DROP_ON;
     } else if (PlacesUtils.nodeIsContainer(node) &&
  •      eventY > rowHeight * 0.75) {
    

were some of the changes made automatically?
These if conditions reindentations around are not great... There are multiple

I think all of the lint issues are fixed in part 2.2, although stuff like whitespace alignment that passes eslint but looks wrong were probably an issue with my converter tool (https://github.com/bgrins/xbl-analysis/blob/gh-pages/scripts/custom-element-utils.js, which results in https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/PlacesTree.js).

I wish our eslint configuration (or some other code beautifier) could define and autofix the correct alignment for this type of condition. The current alignment seems a bit off as well, although not as bad (https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/browser/components/places/content/tree.xml#755).

Attachment #9044173 - Flags: review?(bgrinstead) → review+
Comment on attachment 9044182 [details] [diff] [review]
Part 1.3: Use Shadow DOM for slotting children, r=bgrins

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

::: toolkit/content/widgets/tree.js
@@ +878,5 @@
> +        </hbox>
> +      `));
> +    }
> +
> +    static get observedAttributes() {

After https://bugzilla.mozilla.org/show_bug.cgi?id=1527680 we can drop this boilerplate and use the `inheritedAttributes` helper. The only wrinkle is that it currently uses `this.querySelector` to build the map of elements -> attributes, and here we will want it to use `this.shadowRoot.querySelector` instead. I'm OK to spin this out into a followup if you prefer, or you could update the implementation to do the equivalent of `(this.shadowRoot || this).querySelector(selector)` at https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/toolkit/content/customElements.js#121.
Attachment #9044182 - Flags: review?(bgrinstead) → review+
Comment on attachment 9044176 [details] [diff] [review]
Part 2.3: Fix browser_passwordmgr_editing.js, r=MattN

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

This should be r=MattN
(Assignee)

Comment 92

a month ago

(In reply to Mark Banner (:standard8) (afk until Friday) from comment #86)

Comment on attachment 9044188 [details] [diff] [review]
Part 2.6: Add eslint globals to places-tree.js, r=bgrins

Review of attachment 9044188 [details] [diff] [review]:

::: browser/components/places/content/places-tree.js
@@ +2,4 @@

+/* global PlacesController */

I think slightly nicer here would be to do:

/* import-globals-from controller.js */
/* import-globals-from treeView.js */

(and drop all the individual global definitions).

That gives people an indication that those two files are required alongside
this one (there's more, but they declare what they depend on as well).

r=me with that fixed.

TIL. Very nice. Addressed in the rollup patch that I'll upload.

(Assignee)

Comment 93

a month ago

(In reply to Marco Bonardo [::mak] from comment #87)

Comment on attachment 9044178 [details] [diff] [review]
Part 2.5: Make the edit bookmark tree lazily constructed so there aren't any
trees left in the browser window markup, r=bgrins

Review of attachment 9044178 [details] [diff] [review]:

::: browser/components/places/content/editBookmark.js
@@ +1106,3 @@

              "locationField", "keywordField",
              "tagsField" ]) {

let eltScoped = elt;

while here, could you please remove eltScoped? It became unnecessary a
couple years ago, when JS was fixed to create a new scope per loop. We can
just use elt in the loop.
nit: The space at the end of the array before ] is also unnecessary.

Removing eltScoped seems like a perfect followup bug, even a good-first-bug. I'll spinoff one.

Regarding this and all of the other nits, they were fixed in "Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins", which I'll roll up with all of the other patches.

(Assignee)

Updated

a month ago
Attachment #9044176 - Attachment description: Part 2.3: Fix browser_passwordmgr_editing.js, r=bgrins → Part 2.3: Fix browser_passwordmgr_editing.js, r=MattN
(Assignee)

Updated

a month ago
Attachment #9044188 - Attachment description: Part 2.6: Add eslint globals to places-tree.js, r=bgrins → Part 2.6: Add eslint globals to places-tree.js, r=bgrins, standard8
(Assignee)

Updated

a month ago
Attachment #9044178 - Attachment description: Part 2.5: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, r=bgrins → Part 2.5: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, r=bgrins, mak
(Assignee)

Updated

a month ago
Attachment #9044175 - Attachment description: Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins → Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins, standard8
(Assignee)

Comment 94

a month ago

And this is the rollup with all comments addressed.

Attachment #9045639 - Flags: review+
(Assignee)

Updated

a month ago
Attachment #9045639 - Attachment description: rollup:remove-tree@460200-2019-02-21-1619.diff → Bug 1523957 - Convert trees to custom elements, r=bgrins, mak, standard8, MattN

Comment 96

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6606e852767
Convert trees to custom elements, r=bgrins, mak, standard8, MattN

Victor, just a heads up that I noticed during local testing that we aren't doing super.connectedCallback() in https://hg.mozilla.org/integration/mozilla-inbound/rev/a6606e852767#l7.802, which is resulting in the "hidehscroll" attribute not getting set on places trees, which leads to the horizontal scrollbar being visible even when there's no scroll needed. This should be a quick follow-up fix assuming the push to inbound sticks.

Flags: needinfo?(vporof)

(In reply to Brian Grinstead [:bgrins] from comment #97)

Victor, just a heads up that I noticed during local testing that we aren't doing super.connectedCallback() in https://hg.mozilla.org/integration/mozilla-inbound/rev/a6606e852767#l7.802, which is resulting in the "hidehscroll" attribute not getting set on places trees, which leads to the horizontal scrollbar being visible even when there's no scroll needed. This should be a quick follow-up fix assuming the push to inbound sticks.

Let's see what try thinks of a fix for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a4d5d3737a49566477715fa00c758205382d0fd.

Comment 99

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70117d89f29
Fix places-tree horizontal scrollbar by calling super.connectedCallback;r=bgrins

Went ahead and pushed a fix for the horizontal scrollbar issue.

Flags: needinfo?(vporof)
(Assignee)

Comment 101

a month ago

\o/

Comment 102

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1529828
Depends on: 1529829
Depends on: 1529900

Updated

a month ago
Depends on: 1529872
Flags: needinfo?(mkmelin+mozilla)
Depends on: 1531282

Updated

19 days ago
Depends on: 1532863

Updated

12 days ago
Depends on: 1533739
You need to log in before you can comment on or make changes to this bug.