Closed Bug 1459556 Opened 6 years ago Closed 6 years ago

Move richlistbox#handlersView off of XBL

Categories

(Toolkit :: UI Widgets, task, P1)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug 1459556 - Part 1 - Remove the implementation from the "handler" binding.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1459556 - Part 2 - Remove the "handlers" binding.
59 bytes, text/x-review-board-request
bgrins
: review+
bzbarsky
: review+
Details 
+++ This bug was initially created as a clone of Bug #1457027 +++

THe richlistbox in main.xul is very complicated and cannot be easily refactored to work with Fluent.

This makes it a blocker for bug 1435915.

There's a plan to extend Fluent to support the required feature [0] but it is scheduled for post Fluent 1.0, so I'd like to try to fix it without dynamic references.

If we could move richlistbox away from XBL, or at least flatten the XBL it uses, I believe we could migrate a major remaining chunk of .properties in Preferences to Fluent.


[0] https://github.com/projectfluent/fluent/issues/80
Attachment #8973644 - Attachment is obsolete: true
Attachment #8973644 - Flags: review?(bgrinstead)
Comment on attachment 8973655 [details]
Bug 1459556 - Part 3 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242020/#review247870


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/main.js:2455
(Diff revision 1)
> +  const d = new DOMParser();
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2456
(Diff revision 1)
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2457
(Diff revision 1)
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);
> +  //return range.extractContents();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8973655 [details]
Bug 1459556 - Part 3 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242020/#review247872


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/main.js:2455
(Diff revision 2)
> +  const d = new DOMParser();
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2456
(Diff revision 2)
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2457
(Diff revision 2)
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);
> +  //return range.extractContents();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

Something is not working quite right with this patch. On Mac the menulist is not rendered correctly, and only on Windows it doesn't appear at all, and I get this error after selecting a few items:

TypeError: menuPopup is undefined

rebuildActionsMenu chrome://browser/content/preferences/in-content/main.js:1616:12
_loadData/< chrome://browser/content/preferences/in-content/main.js:1379:9
_fireOnSelect chrome://global/content/bindings/richlistbox.xml:78:13
selectItem chrome://global/content/bindings/listbox.xml:237:11
onxblmousedown chrome://global/content/bindings/listbox.xml:995:13

A number of these errors appear after a while, not immediately, in the console. The inspector doesn't show anything useful regarding the menulist elements.

Any idea what's going on?

Also, I'm using "doc.firstChild.firstElementChild" in parseDOM. Any reason to use a range in bug 1411707? Anyways, it makes no difference for the issue above.
Flags: needinfo?(bgrinstead)
Attachment #8973657 - Flags: review?(bgrinstead)
I also took a look at the CSS and I didn't see anything that would be affected when the children are made not anonymous, but I may have missed something...
(In reply to :Paolo Amadini from comment #8)
> Also, I'm using "doc.firstChild.firstElementChild" in parseDOM. Any reason
> to use a range in bug 1411707? Anyways, it makes no difference for the issue
> above.

The reason the patch for <findbar> uses a range is so that we don't access the DOM node from JS before it's appended into the DOM (to make sure XBL constructors always run). It's likely that returning doc.firstChild.firstElementChild will have the same effect in this case (since it looks like firstElementChild is an hbox without a XBL binding).

Will have to do further testing to confirm, but the concern would be if firstElementChild was an element with a binding attached, then accessing it will bypass XBL construction.
Comment on attachment 8973655 [details]
Bug 1459556 - Part 3 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242020/#review247920

::: browser/components/preferences/in-content/main.js:2497
(Diff revision 2)
>  
> +    this.node.querySelector('[class="actionsMenu"]').addEventListener("command",
> +      event => gMainPane.onSelectAction(event.originalTarget));
> +
>      this.node.setAttribute("type", this.handlerInfoWrapper.type);
> -    this.node.setAttribute("typeDescription",
> +    this.node.querySelector('[inherits="value=typeDescription"]')

I think I'd prefer to change the markup to remove [inherits] altogether and querySelector the relevant classes / other attributes to set the attribute.

Aside: are there cases where the attribute on the host element actually changes dynamically that we are relying on [inherits] to send it down to children, or is this all just set up one time here? (In other similar bindings it turned out to just be done once so stripping out [inherits] clarified things quite a bit).
(In reply to :Paolo Amadini from comment #8)
> Comment on attachment 8973657 [details]
> Bug 1459556 - Part 2 - Copy the content of the "handler" binding to
> "main.js".
> 
> Something is not working quite right with this patch. On Mac the menulist is
> not rendered correctly, and only on Windows it doesn't appear at all, and I
> get this error after selecting a few items:
> 
> TypeError: menuPopup is undefined
> 
> rebuildActionsMenu
> chrome://browser/content/preferences/in-content/main.js:1616:12
> _loadData/< chrome://browser/content/preferences/in-content/main.js:1379:9
> _fireOnSelect chrome://global/content/bindings/richlistbox.xml:78:13
> selectItem chrome://global/content/bindings/listbox.xml:237:11
> onxblmousedown chrome://global/content/bindings/listbox.xml:995:13
> 
> A number of these errors appear after a while, not immediately, in the
> console. The inspector doesn't show anything useful regarding the menulist
> elements.
> 
> Any idea what's going on?

Wait, this happens with just when this patch (part 2) is applied? All this patch does is add a comment into the XBL anonymous content and a JS file, so that would be... strange.

I will take a look, but the patches don't apply cleanly (assuming they rely on patches from Bug 1457027). Are you planning to land Bug 1457027 soon?
Flags: needinfo?(bgrinstead) → needinfo?(paolo.mozmail)
(In reply to Brian Grinstead [:bgrins] from comment #11)
> I think I'd prefer to change the markup to remove [inherits] altogether and
> querySelector the relevant classes / other attributes to set the attribute.

Ah yes, I should've mentioned that I'll try to simplify this. I'm not sure we'll even need the parseDOM approach, the code may not suffer from the issues in the other bug and it may be easier to create the elements and set the attributes with parameters, but I'd still like to test if it works.

> Aside: are there cases where the attribute on the host element actually
> changes dynamically that we are relying on [inherits] to send it down to
> children, or is this all just set up one time here? (In other similar
> bindings it turned out to just be done once so stripping out [inherits]
> clarified things quite a bit).

In the original binding yes, but I've refactored the parts that change dynamically to a separate method, so no change is needed in the callers now.

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Wait, this happens with just when this patch (part 2) is applied? All this
> patch does is add a comment into the XBL anonymous content and a JS file, so
> that would be... strange.

Silly me... I clicked on the third attachment in the bug without noticing this is Part 2, not Part 3 as intended :-)

> I will take a look, but the patches don't apply cleanly (assuming they rely
> on patches from Bug 1457027). Are you planning to land Bug 1457027 soon?

I'll land this now that soft code freeze is lifted. You can also pull from MozReview, but of course you'd have to rebase and then prune 11 changesets afterwards.
Flags: needinfo?(paolo.mozmail)
Attachment #8973655 - Flags: review?(bgrinstead)
Now rebased.
Flags: needinfo?(bgrinstead)
Comment on attachment 8973655 [details]
Bug 1459556 - Part 3 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242020/#review248192


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/main.js:2455
(Diff revision 3)
> +  const d = new DOMParser();
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2456
(Diff revision 3)
> +  d.forceEnableXULXBL();
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/components/preferences/in-content/main.js:2457
(Diff revision 3)
> +  const doc = d.parseFromString(
> +    `<xul:box xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">${str}</xul:box>`,
> +    "application/xml");
> +  //const range = doc.createRange();
> +  //range.selectNodeContents(doc.firstChild);
> +  //return range.extractContents();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
I'm still debugging the cause of it, but it seems that the menulist is intermittently not getting a XBL binding attached (that's what happens when the label is visible but the menu popup doesn't work and you get the JS error).

My initial suspicion is this is because the result of parseDOM gets appended into a richlistitem that itself isn't in the DOM. Specifically, this code:

this.node = document.createElement("richlistitem");
this.node.appendChild(parseDOM(...));

Looking to see how to fix it.
Attached patch xbl-fixes.patch (obsolete) — — Splinter Review 
This fixes both problems. The menulist not getting a binding attached is fixed by including the richlistitem in the parseDOM call instead of creating it with document.createElement. And the disappearing label on selection is fixed by appending the menupopup after the fact (haven't narrowed down why the anon content isn't being created otherwise).
Flags: needinfo?(bgrinstead)
Thanks, that's very helpful! Interesting bugs...

I'll work on an updated patch tomorrow. I'm also wondering if creating everything with createElement instead of using parseDOM would work in this case, or if it would incur in other issues?
(In reply to :Paolo Amadini from comment #21)
> Thanks, that's very helpful! Interesting bugs...
> 
> I'll work on an updated patch tomorrow. I'm also wondering if creating
> everything with createElement instead of using parseDOM would work in this
> case, or if it would incur in other issues?

Please do double check, but I don't think createElement will work (even just using it on the host richlistitem was causing these bugs). I think the rule of thumb is that if you are creating an element that will ultimately have a XBL binding, you can't access it directly from JS until it is inside the DOM and thus will have the CSS binding rule apply. Even `parent.appendChild(document.createElement('richlistitem'))` will trigger the bad behavior.
Right.  XBL bindings get attached when:

1)  The node gets a frame constructed.

or

2)  The node gets its JS reflector created, if it's in the document at that point.

whichever happens first.  If you createElement, you create a JS reflector, but you're not in the document, so you don't get XBL attached.  After that point, even if you insert into a document you won't get XBL attached until either the frame is constructed or the JS reflector is GCed and then you touch the element again.  This last is obviously not a reliable way to get XBL attached...
(In reply to Brian Grinstead [:bgrins] from comment #20)
> And the disappearing label on selection is
> fixed by appending the menupopup after the fact (haven't narrowed down why
> the anon content isn't being created otherwise).

So this one is weird. If I leave no whitespace in the child element, like...

  "<xul:menulist><xul:menupopup/></xul:menulist>"

...it works, while this doesn't:

  "<xul:menulist> <xul:menupopup/> </xul:menulist>"

Is there anything special in the XBL parsing that the DOMParser does not do?
Flags: needinfo?(bzbarsky)
Attachment #8974138 - Attachment is obsolete: true
>   "<xul:menulist><xul:menupopup/></xul:menulist>"
> 
> ...it works, while this doesn't:
> 
>   "<xul:menulist> <xul:menupopup/> </xul:menulist>"
> 
> Is there anything special in the XBL parsing that the DOMParser does not do?

The XUL/XBL parser is set to ignore all-whitespace nodes, whereas (X)HTML does not do this. Most XUL code assumes that the whitespace has been stripped out.
What comment 26 says.  Basically, the XUL/XBL parser kinda violates the XML spec, as a workaround for Gecko layout "issues" around the XUL box model that may or may not still exist.

One thing we could try is have parseDOM call 

  var iter = doc.createNodeIterator(doc, NodeFilter.SHOW_TEXT);

and then walk iter along, collecting up textnodes that are whitespace-only into an array and then call remove() on them all.  That will let you avoid touching the element nodes from script while finding the whitespace bits.

You might be able to put the whitespace-only check into a NodeFilter you pass in; I don't know how the performance would compare.  You might also be able to remove as you iterate, but I'd have to check carefully to tell how NodeIterator responds to removals during iteration.
Flags: needinfo?(bzbarsky)
For our purposes, I'm thinking it may even be sufficient to use a simple string replace. As for performance, hopefully we need to do the parsing only once per binding. I tried using document.importNode to reuse the fragment for multiple instances, and for the moment this seems to work...
Attachment #8973655 - Attachment is obsolete: true
This is ready for review. I didn't copy the markup in a separate changeset because it changed significantly after cleaning up "inherits", and I also removed the namespace. I used some of the comments in this bug to make a hopefully correct and detailed comment for the parseXULToFragment helper function, and I used importNode so we can reuse the result of the parsing.
Attachment #8973656 - Flags: review?(bgrinstead) → review?(jaws)
I can review part 1 if :jaws is busy, but I think he's a better reviewer for that part.
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242024/#review250386

::: browser/components/preferences/in-content/main.js:2453
(Diff revision 3)
>  
>  function isFeedType(t) {
>    return t == TYPE_MAYBE_FEED || t == TYPE_MAYBE_VIDEO_FEED || t == TYPE_MAYBE_AUDIO_FEED;
>  }
>  
> +var gXULDOMParser = new DOMParser();

We are going to want to re-use this very soon (like in the `<findbar>` conversion). I'd be generally OK to land it here for now, but can you think of a place we could put this that would be shareable with other CE implementations?

My initial ideas:

1. Some JSM in toolkit/modules (either an existing one or new one)
2. Tack it onto `window.DOMParser` or `window.customElements` or make a new window property in MainProcessSingleton when we load CE scripts (possibly making a new JS file called customElements.js that both defines utility functions and takes care of subscript loading for CE definitions).

(1) is probably the most straightforward for now. Although as we want to share other capabilities for CE that rely on accessing document/window properties (like a xbl:inherits-style helper) a JSM probably won't be the right place. But we can cross that bridge when we get there.

::: browser/components/preferences/in-content/main.js:2485
(Diff revision 3)
> +  // The XUL/XBL parser is set to ignore all-whitespace nodes, whereas (X)HTML
> +  // does not do this. Most XUL code assumes that the whitespace has been
> +  // stripped out, so we do this manually before using the parser.
> +  let doc = gXULDOMParser.parseFromString(`
> +    <box xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +      ${str.replace(/>\s+</g, "><")}

Unless if there are measurable performance issues with it, I'd prefer to do something like this instead of relying on a regexp:

```
const iter = doc.createNodeIterator(doc, NodeFilter.SHOW_TEXT);
const pars = [];
let currentNode;
while (currentNode = iter.nextNode()) {
  pars.push(currentNode);
}
pars.forEach(p=>p.remove());
```

::: browser/components/preferences/in-content/main.js:2548
(Diff revision 3)
> +
> +    this.node.querySelector(".actionsMenu").addEventListener("command",
> +      event => gMainPane.onSelectAction(event.originalTarget));
> +
> +    let typeDescription = this.handlerInfoWrapper.typeDescription;
> +    this.setOrRemoveAttributes([

I think the API would be simplified here by handling only a single node/attribute at a time. So instead of an array of arrays for as a single argument it would take 3 separate args like:

```
this.setOrRemoveAttribute(null, "type", this.handlerInfoWrapper.type);
this.setOrRemoveAttribute(".typeContainer", tooltiptext", typeDescription);
...
```

I'll leave it up to you, but I think it's more guessable what to pass into the function this way.
Attachment #8973657 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #36)
> 1. Some JSM in toolkit/modules (either an existing one or new one)

I'd avoid a JSM in this case because the code may be performance sensitive and may want to use global window properties and constructors frequently. This would require cross-compartment wrappers if the code is in a JSM.

> 2. Tack it onto `window.DOMParser` or `window.customElements` or make a new
> window property in MainProcessSingleton when we load CE scripts (possibly
> making a new JS file called customElements.js that both defines utility
> functions and takes care of subscript loading for CE definitions).

I'd make one or more new window properties. Since this is only loaded in privileged scopes, I'm not concerned about having multiple properties for class-like constructors and objects, for example we recently added XULPopupElement to the properties accessible in the window scope.

We may define something like this before loading the elements:

var mozElementMixin = Base => class extends Base {
  static parseXULToFragment() {}
  setOrRemoveAttribute() {}
};
class MozXULElement extends mozElementMixin(XULElement) {}
class MozXULPopupElement extends mozElementMixin(XULPopupElement) {}

The mixin is only necessary because we have more than one class implemented in C++, and we only need to use it once for each  class that we implement in C++. Most custom elements would inherit from MozXULElement directly. For example:

class MozDeckElement extends MozXULElement {}

Note that for the elements where we don't use lazy loading, this allows us to use "instanceof" checks exactly like all other element types:

if (element instanceof MozDeckElement) {}

This can't be done for lazy loaded elements, but we can just check the tag name in that case.

if (element.localName == "moz-deck") {}

Static helpers would be accessible elsewhere from the MozXULElement interface:

MozXULElement.parseXULToFragment()

We can also consider having a separate interface for the static helpers, if we don't need any form of overriding when they are called on a derived class.

> Unless if there are measurable performance issues with it, I'd prefer to do
> something like this instead of relying on a regexp:

Okay. Looks like the nodes can also be removed directly during the iteration.

> I think the API would be simplified here by handling only a single
> node/attribute at a time. So instead of an array of arrays for as a single
> argument it would take 3 separate args like:

I implemented it this way to reduce repetition in the common use case. For example, we do this for various APIs that we use for defining module and service getters. We may want to add a "singular" version of the same API later if needed, like we have for the module getters.
Defining MozXULElement with a static parseXULToFragment function makes sense to me. We could add a new customElements.js file that gets loaded from MainProcessSingleton, defines the base class(es) and then sets up lazy loading for the actual Custom Elements.
(In reply to Brian Grinstead [:bgrins] from comment #40)
> Defining MozXULElement with a static parseXULToFragment function makes sense
> to me. We could add a new customElements.js file that gets loaded from
> MainProcessSingleton, defines the base class(es) and then sets up lazy
> loading for the actual Custom Elements.

I think there's a test that will fail if we tack a new global onto windows (I had a try push where I defined MozStringbundle outside of the block scope it's currently in and the test complained about it). But we should be able to whitelist `MozXULElement` for that test. Let's move this work out into a new bug depending on this one.
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

Boris, can you please review the code/comment for parseXULToFragment in this patch?
Attachment #8973657 - Flags: review?(bzbarsky)
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242024/#review250820

r=me on the parseXULToFragment bits.
Attachment #8973657 - Flags: review?(bzbarsky) → review+
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242024/#review250852

::: browser/components/preferences/in-content/main.js:1550
(Diff revision 4)
>      // If the user is filtering the list, then only show matching types.
>      if (this._filter.value)
>        visibleTypes = visibleTypes.filter(this._matchesFilter, this);
>  
>      for (let visibleType of visibleTypes) {
>        let item = new HandlerListItem(visibleType);

Could we pass `this._list` into the constructor instead of having a new `connectAndAppendToList` function that's expected to be called first thing after being constructed? Or is there some case where we would want to construct one and not append it to the list immediately?

::: browser/components/preferences/in-content/main.js:2564
(Diff revision 4)
>      this.refreshAction();
> +    this.showActionsMenu = false;
>    }
>  
>    refreshAction() {
> -    this.node.setAttribute("actionDescription",
> +    let actionIconClass = this.handlerInfoWrapper.actionIconClass;

Nit:

```
let {actionIconClass,actionDescription} = this.handlerInfoWrapper;
```
Attachment #8973657 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #44)
> Could we pass `this._list` into the constructor instead of having a new
> `connectAndAppendToList` function that's expected to be called first thing
> after being constructed?

The reason I didn't do that is that the method name makes it clear that the element is appended to the list, whereas in the constructor it would need a comment in the caller saying what the behavior is. Constructor parameters generally provide a backreference to the container, but don't add the item to it.

This also resembles how Custom Elements are only connected when they are inserted into the document, but can be constructed earlier, even though in this case we can't use the node before it's connected because of the XBL limitations.
No longer blocks: 1462798
Depends on: 1462798
Attachment #8973656 - Flags: review?(jaws) → review?(bgrinstead)
Bouncing this back to bgrins as I'm pretty busy right now and he's already reviewed part 2 of the patch series here.
Comment on attachment 8973657 [details]
Bug 1459556 - Part 2 - Remove the "handlers" binding.

https://reviewboard.mozilla.org/r/242024/#review251614

Please update this patch to use MozXULElement.parseXULToFragment now that we landed it as a shared helper
Comment on attachment 8973656 [details]
Bug 1459556 - Part 1 - Remove the implementation from the "handler" binding.

https://reviewboard.mozilla.org/r/242022/#review251616

::: browser/components/preferences/in-content/main.js:1456
(Diff revision 4)
>  
> +  selectedHandlerListItem: null,
> +
> +  _initListEventHandlers() {
> +    this._list.addEventListener("select", event => {
> +      if (event.target != this._list) {

When would this happen?
Attachment #8973656 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #49)
> > +    this._list.addEventListener("select", event => {
> > +      if (event.target != this._list) {
> 
> When would this happen?

When elements from the menulist are selected.
ESLint says that MozXULElement is not defined in the "main.js" file, see the test result in comment 50.

MozXULElement is a window property defined in all windows where one of our Custom Elements can be used, I'm not totally sure if this is all privileged contexts or not, but Brian may be able to specify this better.

Mark, what is the correct way to whitelist this window property? We could do this locally in "main.js", but I assume that this class will be used in other cases as well.
Flags: needinfo?(standard8)
Flags: needinfo?(bgrinstead)
It's available in all chrome privileged XUL documents in the parent process: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/toolkit/components/processsingleton/MainProcessSingleton.js#84-93.

It was added to the browser-window eslint environment here: https://hg.mozilla.org/mozilla-central/rev/7c4c86f3913f#l10.12. Maybe it needs to be added to an additional environment?
Flags: needinfo?(bgrinstead)
Hm, I'll patch this locally this until we figure out how to apply this to all in-content privileged windows.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b309d5725fe
Part 1 - Remove the implementation from the "handler" binding. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/4106f2f28462
Part 2 - Remove the "handler" binding. r=bgrins
It might be simplest for now to add it as a global to recommended.js.

Unfortunately, we have lots of slightly different environments across the tree and determining which applies to which file is hard. This sounds like it'd want a "xul privileged" environment, which is probably almost the same as "chrome privileged" but we don't really have either at the moment, and working out which file / sub-sections applies where is going to be hard.
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/6b309d5725fe
https://hg.mozilla.org/mozilla-central/rev/4106f2f28462
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Mark Banner (:standard8) from comment #56)
> It might be simplest for now to add it as a global to recommended.js.
> 
> Unfortunately, we have lots of slightly different environments across the
> tree and determining which applies to which file is hard. This sounds like
> it'd want a "xul privileged" environment, which is probably almost the same
> as "chrome privileged" but we don't really have either at the moment, and
> working out which file / sub-sections applies where is going to be hard.

I'm now wondering if we should be loading customElements.js in all chrome privileged documents, and not just XUL (so we could use XUL CE in an HTML context). Would also simplify the number of lint environments.
Blocks: 1381706
Type: enhancement → task
See Also: → 1585837
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: