1523957 - (deforestation) Convert trees to custom elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: remove-tree-2019-01-30-1705.diff

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

Comment 2

[Brian Grinstead [:bgrins]]

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 3

[Brian Grinstead [:bgrins]]

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 4

[Brian Grinstead [:bgrins]]

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).

Comment 5

[Brian Grinstead [:bgrins]]

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.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1523957 - WIP - Convert <xul:tree> to a Custom Element

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

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?

Comment 9

[Emilio Cobos Álvarez (: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.

Comment 10

[Brian Grinstead [:bgrins]]

(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.

Comment 11

[Brian Grinstead [:bgrins]]

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=).

Comment 12

[Victor Porof [:vporof][:vp]]

Looks like it's pretty orange.

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

Comment 13

[Brian Grinstead [:bgrins]]

(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)?

Comment 14

[alexander :surkov (:asurkov)]

(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?

Comment 15

[Victor Porof [:vporof][:vp]]

(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>?

Comment 16

[Brian Grinstead [:bgrins]]

(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".

Comment 17

[Brian Grinstead [:bgrins]]

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.

Comment 18

[Brian Grinstead [:bgrins]]

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.

Comment 19

[Brian Grinstead [:bgrins]]

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.

Comment 20

[Brian Grinstead [:bgrins]]

Latest version looks much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baa2f003bcfa62f5c15e16c1c1ecab43aceb0574

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: Bug 1523957 - Convert DevTools XBL test to use a test binding instead of a xul:tree;r=pbro

This will unblock removing XBL for tree.

Comment 22

[Brian Grinstead [:bgrins]]

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.

Comment 23

[Brian Grinstead [:bgrins]]

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.

Comment 24

[Phabricator Automation]

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.

Comment 25

[Victor Porof [:vporof][:vp]]

(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.

Comment 26

[Victor Porof [:vporof][:vp]]

Attachment: remove-tree-latest-2019-02-08-1742.diff

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

Comment 27

[Victor Porof [:vporof][:vp]]

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

Comment 28

[Brian Grinstead [:bgrins]]

(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 29

[Brian Grinstead [:bgrins]]

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

Comment 30

[Brian Grinstead [:bgrins]]

Heh, I guess this explains it: https://searchfox.org/mozilla-central/rev/03ebbdab952409640c6857d835d3040bf6f9e2db/browser/components/places/PlacesUIUtils.jsm#348.

Comment 31

[Brian Grinstead [:bgrins]]

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

Comment 32

[Victor Porof [:vporof][:vp]]

New try with the library window fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf740f2677d806fe8ce648047eba8735bc9f7b9

Comment 33

[Victor Porof [:vporof][:vp]]

Attachment: Part 0: Pre, r=bgrins

Comment 34

[Victor Porof [:vporof][:vp]]

Attachment: Part 1: Convert tree to a custom element, r=bgrins

Comment 35

[Victor Porof [:vporof][:vp]]

Attachment: Part 2: Convert places-tree to a custom element, r=bgrins

Comment 36

[Victor Porof [:vporof][:vp]]

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

Comment 37

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.1: Naively convert tree to a custom element

Comment 38

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.1: Naively convert tree to a custom element, r=bgrins

Comment 39

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.2: Properly mixin BaseControl and extend XULTreeElement, r=bgrins

Comment 40

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.3: Use Shadow DOM for slotting children, r=bgrins

Comment 41

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.4: Properly handle key events in combination with shift modifiers, r=bgrins

Comment 42

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

Comment 43

[Victor Porof [:vporof][:vp]]

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.

Comment 44

[Victor Porof [:vporof][:vp]]

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.

Comment 45

[Brian Grinstead [:bgrins]]

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

Comment 46

[Brian Grinstead [:bgrins]]

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.

Comment 47

[Brian Grinstead [:bgrins]]

(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.

Comment 48

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.5: Lint for the lint gods in tree.js, r=bgrins

Comment 49

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins

Comment 50

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.3: Remove content/global/bindings/tree.xml and all references, r=bgrins

Removed for good measure, but this does't fix (5) locally.

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

Comment 51

[Victor Porof [:vporof][:vp]]

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).

Comment 52

[Victor Porof [:vporof][:vp]]

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

Comment 53

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.4: Fix browser_passwordmgr_editing.js, r=bgrins, mak

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

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

Comment 54

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.5: Fix browser_bookmarkProperties_no_user_actions.js, r=bgrins

Comment 55

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.6: Make the edit bookmark tree lazily constructed so there aren't any trees left in the browser window markup, author=bgrins

Comment 56

[Victor Porof [:vporof][:vp]]

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 57

[Brian Grinstead [:bgrins]]

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?

Comment 58

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 59

[Jorg K (CEST = GMT+2)]

Thanks, Magnus and team will take care of it.

Comment 60

[Brian Grinstead [:bgrins]]

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.

Comment 61

[Richard Marti (:Paenglab)]

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

Comment 62

[Brian Grinstead [:bgrins]]

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 63

[Matthew N. [:MattN]]

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?

Comment 64

[Brian Grinstead [:bgrins]]

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

Comment 65

[Brian Grinstead [:bgrins]]

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).

Comment 66

[Brian Grinstead [:bgrins]]

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?

Comment 67

[Brian Grinstead [:bgrins]]

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.

Comment 68

[Brian Grinstead [:bgrins]]

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

Comment 69

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.3: Use Shadow DOM for slotting children, r=bgrins

Addressed comments.

Comment 70

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.1: Naively convert places-tree to a custom element, preserving history, r=bgrins, mak

Comment 71

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.2: Lint for the lint gods in places-tree.js, r=bgrins, standard8

Comment 72

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.3: Fix browser_passwordmgr_editing.js, r=MattN

Comment 73

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.4: Fix browser_bookmarksProperties.js, r=bgrins

Comment 74

[Victor Porof [:vporof][:vp]]

Attachment: 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

Comment 75

[Victor Porof [:vporof][:vp]]

(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?

Comment 76

[Victor Porof [:vporof][:vp]]

Attachment: Part 1.3: Use Shadow DOM for slotting children, r=bgrins

Comment 77

[Victor Porof [:vporof][:vp]]

Attachment: Part 2.6: Add eslint globals to places-tree.js, r=bgrins, standard8

Comment 78

[Victor Porof [:vporof][:vp]]

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

Comment 79

[Brian Grinstead [:bgrins]]

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

Comment 80

[Brian Grinstead [:bgrins]]

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.

Comment 81

[Brian Grinstead [:bgrins]]

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

Comment 82

[Richard Marti (:Paenglab)]

(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.

Comment 83

[Victor Porof [:vporof][:vp]]

\o/

Comment 84

[Brian Grinstead [:bgrins]]

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

Comment 85

[Mark Banner (: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]:
-----------------------------------------------------------------

Yeah, browser-window.js is fine here.

Comment 86

[Mark Banner (:standard8)]

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.

Comment 87

[Marco Bonardo [:mak]]

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.

Comment 88

[Marco Bonardo [:mak]]

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

Comment 89

[Brian Grinstead [:bgrins]]

(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).

Comment 90

[Brian Grinstead [:bgrins]]

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.

Comment 91

[Matthew N. [:MattN]]

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

Comment 92

[Victor Porof [:vporof][:vp]]

(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 @@

• 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.

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

Comment 93

[Victor Porof [:vporof][:vp]]

(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.

Comment 94

[Victor Porof [:vporof][:vp]]

Attachment: Bug 1523957 - Convert trees to custom elements, r=bgrins, mak, standard8, MattN

And this is the rollup with all comments addressed.

Comment 95

[Victor Porof [:vporof][:vp]]

Here's a try with the rollup: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b9fc003d10189cc0c85212e50092f0303b2122

Here's a talos with the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0fe555fa22ed51ad4f557f6279b696f2eeaff5b
...and without: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74ebc4b9eb766b51cc2b0393f1886c3199c6debc

Comment 96

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6606e852767
Convert trees to custom elements, r=bgrins, mak, standard8, MattN

Comment 97

[Brian Grinstead [:bgrins]]

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.

Comment 98

[Brian Grinstead [:bgrins]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70117d89f29
Fix places-tree horizontal scrollbar by calling super.connectedCallback;r=bgrins

Comment 100

[Brian Grinstead [:bgrins]]

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

Comment 101

[Victor Porof [:vporof][:vp]]

\o/

Comment 102

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a6606e852767
https://hg.mozilla.org/mozilla-central/rev/e70117d89f29