After we have Custom Elements in XUL, <stringbundle> would be a simple binding [0] to migrate as a way to figure out what the migration process can look like before attempting more complex bindings. For example, we still need to figure out how to register the Custom Element on the window. Should we register a <script> for every XUL doc [0] that creates a stringbundle? And, should we preprocess all Custom Element definitions into a single file or let documents opt in to only elements they use? [0]: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/stringbundle.xml [1]: http://searchfox.org/mozilla-central/search?q=%3Cstringbundle&path=xul%24

Need to confirm that "XStringBundle" doesn't show up as a directory in the debugger after the change, and remove the relevant debugger code if it's not a problem anymore (see http://searchfox.org/mozilla-central/search?q=XStringBundle and Bug 843609).

Going to spend some time looking into this

Have a try push working around an issue for now by removing disconnectedCallback to avoid widespread leaks (https://bugzilla.mozilla.org/show_bug.cgi?id=1404420#c30). https://treeherder.mozilla.org/#/jobs?repo=try&revision=d178e7b50b0eb456c70c44e631f73f3bab53fe6d. The approach of loading the script with the subscript loader on the "chrome-document-global-created" message seems to work, but will need to do some talos runs.

Smaug, in this patch I'm trying to get the Custom Element registrations loaded in every chrome document. Right now the way I'm doing that is to bind to "chrome-document-global-created" in the parent process and then using the subscript loader to load the JS files that do the registrations. I wonder if we should add (or already have) some mechanism to load scripts inside of each document that has AllowXULXBL set to true. This would allow us to handle the "remote XUL" case in the content process without needing to set up a new observer for chrome documents in that process, and maybe would allow for performance optimization for when/how we load the script. Any ideas on if/how we should do this?

As far as I know, we don't have such thing yet, but probably should add. Or, for now, at least for non-remote case, coulnd't we just always have some <script> element included in XUL documents? The scripts are cached and just cloned for each new instance of the document. But perhaps we should do that implicitly. Each non-overlay XUL document would basically have some default script loaded. I guess easiest would be to reuse the existing XUL prototype stuff. When a new prototype document is created, we'd ensure the default script has been loaded. And when actual XUL document is created, we clone the script and run it. I wonder at which point. Right after creating the document, but before adding document element?

(In reply to Olli Pettay [:smaug] from comment #11) > As far as I know, we don't have such thing yet, but probably should add. > Or, for now, at least for non-remote case, coulnd't we just always have some > <script> element included in XUL documents? The scripts are cached and just > cloned for each new instance of the document. I'd like to avoid explicitly including the script if possible. I started out doing that but quickly realized how many different documents we have in tree (including tests), and I'd like consuming our XUL Custom Elements to be approximately as developer friendly as consuming XBL bindings. > But perhaps we should do that implicitly. Each non-overlay XUL document > would basically have some default script loaded. I guess easiest would be to > reuse the existing XUL prototype stuff. When a new prototype document is > created, we'd ensure the default script has been loaded. And when actual XUL > document is created, we clone the script and run it. Yeah, something like that is what I was thinking of. If we could support a list of scripts that would be better for js debugging compared with preprocessing all of the CE scripts into a mega-file. We could also have a single script that uses the subscript loader for each other file but that may lose potential perf wins from this approach. > I wonder at which point. Right after creating the document, but before adding document element? The current observer notification I'm using ("chrome-document-global-created") is "Sent immediately after a chrome document window has been set up, but before any script code has been executed". I'm not sure if this is the ideal time to do it, but it does seem to work.

Note that stringbundles will go away anyway, see bug 1365426.

(In reply to Dão Gottwald [::dao] from comment #13) > Note that stringbundles will go away anyway, see bug 1365426. Yeah, maybe this should be repurposed to do <deck> (or some other simple binding) instead. I'm hoping to use this bug as a way to figure out how to register the custom element scripts and unblock the rest of the CE work.

Renaming to reflect that this may not end up being stringbundle

Comment on attachment 8922105 [details] Bug 1411707 - Switch XUL findbar from a XBL binding to a Custom Element; https://reviewboard.mozilla.org/r/193106/#review242412 ::: toolkit/content/chromeGlobals.js:14 (Diff revision 6) > + > +// Set up Custom Elements for XUL documents. > +// Wait for DOMContentLoaded since this script gets loaded when the document > +// isn't setup yet (chrome-document-global-created). Note that this is added > +// to the window because it doesn't fire on all the time on `document`. > +window.addEventListener("DOMContentLoaded", () => { https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-upgrades-examples If I understand correctly, waiting for DOMContentLoaded means that all the elements in the document will be upgraded after parsing. Also, any element created programmatically using createElement will be upgraded only when inserted in the document, meaning it may or may not have its custom properties accessible right after createElement is called. If any other code also waits for the same "DOMContentLoaded" event, it may run either before or after the upgrade, making the situation even more uncertain. I'm wondering if it would be better to define the custom elements always before other scripts in the document run, so we get consistent behavior independently from when the code is called? It seems that this may by done by simply including "chromeGlobals.js" in "browser.xul" and other documents manually, but this may not necessarily be practical for the many XUL documents we have in tests. Maybe we need a new platform mechanism? Also, if we put all the "define" calls directly in "chromeGlobals.js", we may be able to lazify the subscript loading. I would call this script something like "xulElementsRegistry.js", because this is basically the role it would have. We may also consider moving the element definitions that are specific to the browser window to a separate "browserElementsRegistry.js" file with a similar structure, still loaded before the other scripts.

(In reply to :Paolo Amadini from comment #18) > Comment on attachment 8922105 [details] > Bug 1411707 - Create 'tmp' custom element to test loading a CE in browser.xul > > https://reviewboard.mozilla.org/r/193106/#review242412 > > ::: toolkit/content/chromeGlobals.js:14 > (Diff revision 6) > > + > > +// Set up Custom Elements for XUL documents. > > +// Wait for DOMContentLoaded since this script gets loaded when the document > > +// isn't setup yet (chrome-document-global-created). Note that this is added > > +// to the window because it doesn't fire on all the time on `document`. > > +window.addEventListener("DOMContentLoaded", () => { > > https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements- > upgrades-examples > > If I understand correctly, waiting for DOMContentLoaded means that all the > elements in the document will be upgraded after parsing. Also, any element > created programmatically using createElement will be upgraded only when > inserted in the document, meaning it may or may not have its custom > properties accessible right after createElement is called. If any other code > also waits for the same "DOMContentLoaded" event, it may run either before > or after the upgrade, making the situation even more uncertain. > > I'm wondering if it would be better to define the custom elements always > before other scripts in the document run, so we get consistent behavior > independently from when the code is called? > > It seems that this may by done by simply including "chromeGlobals.js" in > "browser.xul" and other documents manually, but this may not necessarily be > practical for the many XUL documents we have in tests. Maybe we need a new > platform mechanism? That's what I was hoping for in Bug 1442006, but as per https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c3 this is what chrome-document-global-created is for. So maybe there's something we can do to make the state of things more consistent when that event fires. Kris, do you have any other ideas on how to get the same behavior as if we added a <script> tag as the first thing in each chrome document? The window/document definitely seem to be in some kind of weird state when this chrome-document-global-created event happens - for example: - I have to listen for "DOMContentLoaded" on the window object (if listening on document it doesn't always fire) - document.readyState is inconsistent (I believe it's sometimes even "complete", but usually it isn't) - document.contentType is inconsistent (see https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c15) - The window location is inconsistent Waiting for DOMContentLoaded does seem to work, but as Paolo points out there are likely to be some timing/upgrade issues with Custom Elements if we wait that long.

Also asking :bz if he has an idea on how we could accomplish what we want from Comment 19 - maybe somehow changing the state things are in for chrome-document-global-created, or firing a different event right before the document is parsed and other scripts are run.

The simplest solution for now is probably to use document-element-inserted rather than chrome-document-global-created. It's a bit unfortunate, since that also fires for non-chrome globals, but we shouldn't have very many of those in the parent process, anyway.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #21) > The simplest solution for now is probably to use document-element-inserted > rather than chrome-document-global-created. It's a bit unfortunate, since > that also fires for non-chrome globals, but we shouldn't have very many of > those in the parent process, anyway. Just checked and we don't get notified on this for XUL docs (although we do for HTML docs), I guess because the notification is coming from XML content sink and not XUL content sink. Suppose we could add the notification for XUL docs as well - although we'll need to check to confirm that existing consumers of document-element-inserted will ignore them events.

Kris' suggestion in Comment 21 plus emitting the `document-element-inserted` event from XUL content sink seems to work - clearing needinfo.

(In reply to Brian Grinstead [:bgrins] from comment #25) > Kris' suggestion in Comment 21 plus emitting the `document-element-inserted` > event from XUL content sink seems to work - clearing needinfo. I think the XUL content sink notification is broken when re-opening a window multiple times (as we do for WebIDE via https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/client/framework/devtools-browser.js#320), probably because of prototype caching. As a result with the patches applied the test at `./mach mochitest devtools/client/webide/test/test_autoselect_project.html` fails because the iframe Custom Element doesn't get registered on the second WebIDE window (and thus the .contentDocument property doesn't exist on it).

I'd like some feedback on how the scripts get loaded / Custom Elements get registered from some of the people involved with the discussion in Bug 1442006 before asking for review. The patch in question is here: https://bugzilla.mozilla.org/attachment.cgi?id=8922105. Florian - using document-element-inserted and then filtering on XUL content type should address your concern in https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c12, right? Gijs, Matt, Mossop - what do you think about this general approach? Basically: MainProcessSingleton loads chromeGlobals.js in XUL docs when a document element gets created. Then chromeGlobals loads a set of 'core' / 'toolkit-y' bindings that we want to have available everywhere. Some changes I'd imagine over time are: - chromeGlobals will likely get loaded in HTML docs as well - both to provide global imports like Services, and potentially to expose utilities needed to better support top-level HTML windows as part of a longer term migration towards HTML (see Bug 1453783) - We may want to (carefully) increase what gets imported at the top of the file and thus doesn't need to be imported in every single window (i.e. XPCOMUtils) One potential change I've thought of now is to not load the CE scripts with `this` (the window) as the context but instead to pass only a subset of objects (like `customElements`, `window`, `document`, `Services`, more as needed). This would make it harder to accidentally pollute all XUL windows when importing a script into a CE file. Although I imagine the list of what gets passed in would have to expand quite a bit over time as we migrate more bindings. It would also have the side effect of the classes (i.e. MozIframe) not being part of the window global - I'm not sure if that really matters one way or the other.

(In reply to Brian Grinstead [:bgrins] from comment #42) > One potential change I've thought of now is to not load the CE scripts with > `this` (the window) as the context but instead to pass only a subset of > objects (like `customElements`, `window`, `document`, `Services`, more as > needed). This would make it harder to accidentally pollute all XUL windows > when importing a script into a CE file. Although I imagine the list of what > gets passed in would have to expand quite a bit over time as we migrate more > bindings. It would also have the side effect of the classes (i.e. MozIframe) > not being part of the window global - I'm not sure if that really matters > one way or the other. You mean loading custom elements as ES6 modules? There’s work being done in bug 1308512 to get ES6 modules working with existing Cu.import-based modules, and eventually migrate from Cu.import to ES6 modules entirely.

(In reply to ExE Boss from comment #43) > (In reply to Brian Grinstead [:bgrins] from comment #42) > > One potential change I've thought of now is to not load the CE scripts with > > `this` (the window) as the context but instead to pass only a subset of > > objects (like `customElements`, `window`, `document`, `Services`, more as > > needed). This would make it harder to accidentally pollute all XUL windows > > when importing a script into a CE file. Although I imagine the list of what > > gets passed in would have to expand quite a bit over time as we migrate more > > bindings. It would also have the side effect of the classes (i.e. MozIframe) > > not being part of the window global - I'm not sure if that really matters > > one way or the other. > > You mean loading custom elements as ES6 modules? There’s work being done in > bug 1308512 to get ES6 modules working with existing Cu.import-based > modules, and eventually migrate from Cu.import to ES6 modules entirely. No, I'm referring to passing in a different targetObj to Services.scriptloader.loadSubScript. So instead of `this` we could pass in something like `{ customElements, window, document, Services }`.

(In reply to Brian Grinstead [:bgrins] from comment #42) > Florian - using document-element-inserted and then filtering on XUL content > type should address your concern in > https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c12, right? I think so, but I would prefer to verify with a few dump calls.

(In reply to Brian Grinstead [:bgrins] from comment #42) > I'd like some feedback on how the scripts get loaded / Custom Elements get > registered from some of the people involved with the discussion in Bug > 1442006 before asking for review. > > The patch in question is here: > https://bugzilla.mozilla.org/attachment.cgi?id=8922105. > > Florian - using document-element-inserted and then filtering on XUL content > type should address your concern in > https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c12, right? > > Gijs, Matt, Mossop - what do you think about this general approach? > Basically: MainProcessSingleton loads chromeGlobals.js in XUL docs when a > document element gets created. We could just insert it in documents with system principal. That would (a) avoid nasty surprises with privilege escalation for "remote xul" / "new xbl" or whatever (XBL-in-webcontent was a recurring source of security vulnerabilities until we turned most of it off) and (b) be more compatible with switching to HTML. > One potential change I've thought of now is to not load the CE scripts with > `this` (the window) as the context but instead to pass only a subset of > objects (like `customElements`, `window`, `document`, `Services`, more as > needed). This would make it harder to accidentally pollute all XUL windows > when importing a script into a CE file. Although I imagine the list of what > gets passed in would have to expand quite a bit over time as we migrate more > bindings. It would also have the side effect of the classes (i.e. MozIframe) > not being part of the window global - I'm not sure if that really matters > one way or the other. If I understand correctly, this would mean we create a copy of chromeGlobals.js and its scope object for every window. Can we avoid that? I would assume that essentially, `window` and `document` can be inferred from the bound element to which the CE stuff gets access when instantiated, and Services.* is already a singleton. So perhaps all windows can share a single CE scope, and we just copy references to any global CE constructors from the CE scope to the window in which they might be needed, and invoke the required registration on `customElements` for each new window. Or something. Would that work? Is that a dumb idea? Are we going to have issues with leaks if we do that because this global scope thing might hang on to the element instances ~forever or whatever?

(In reply to :Gijs (he/him) from comment #46) > > One potential change I've thought of now is to not load the CE scripts with > > `this` (the window) as the context but instead to pass only a subset of > > objects (like `customElements`, `window`, `document`, `Services`, more as > > needed). This would make it harder to accidentally pollute all XUL windows > > when importing a script into a CE file. Although I imagine the list of what > > gets passed in would have to expand quite a bit over time as we migrate more > > bindings. It would also have the side effect of the classes (i.e. MozIframe) > > not being part of the window global - I'm not sure if that really matters > > one way or the other. > > If I understand correctly, this would mean we create a copy of > chromeGlobals.js and its scope object for every window. Can we avoid that? I > would assume that essentially, `window` and `document` can be inferred from > the bound element to which the CE stuff gets access when instantiated, and > Services.* is already a singleton. So perhaps all windows can share a single > CE scope, and we just copy references to any global CE constructors from the > CE scope to the window in which they might be needed, and invoke the > required registration on `customElements` for each new window. Or something. > Would that work? Is that a dumb idea? Are we going to have issues with leaks > if we do that because this global scope thing might hang on to the element > instances ~forever or whatever? Yes your understanding is correct. I hadn't thought about moving the Custom Element classes into a JSM or some other way of sharing them across windows in a while - although it is something I considered for tabbrowser. I can't find his original comment, but I believe Mossop said it could actually be faster to just load them in the window if the classes need to interact with the window a lot (I refer to this here https://bugzilla.mozilla.org/show_bug.cgi?id=1392352#c7). If perf is OK I'd generally prefer to have the CE classes live in a window (or window-ish) environment just so it feels more normal (and avoids footguns like leaks). I was thinking of using talos as a proxy for "perf is OK", but could do some more direct testing if needed.

Sorry for the lag here. I'm a bit surprised that the "global created" notification fires before we've set the new document on the enwly-created window (though that's certainly what it sounds like). Might be worth a separate bug on that. In the meantime, using root element insertion seems reasonable...

Comment on attachment 8968635 [details] Bug 1411707 - Notify document-element-inserted in XUL documents; https://reviewboard.mozilla.org/r/237310/#review244048 ::: dom/xul/XULDocument.cpp:532 (Diff revision 6) > > // Add the style overlays from chrome registry, if any. > rv = AddPrototypeSheets(); > if (NS_FAILED(rv)) return rv; > > + nsContentSink::NotifyDocElementCreated(this); Hmm. Have we removed overlays yet? If not, this will get triggered for every overlay that completes. Might be worth conditioning on mCurrentPrototype == mMasterPrototype or mState == eState_Master or something. In any case, notifying at this point is a bit weird, since we won't create the element until PrepareToWalk. And in particular, why not just do this in the block in PrepareToWalk where we actually do the root element creation and insertion?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #49) > Comment on attachment 8968635 [details] > Bug 1411707 - Notify document-element-inserted in XUL documents; > > https://reviewboard.mozilla.org/r/237310/#review244048 > > ::: dom/xul/XULDocument.cpp:532 > (Diff revision 6) > > > > // Add the style overlays from chrome registry, if any. > > rv = AddPrototypeSheets(); > > if (NS_FAILED(rv)) return rv; > > > > + nsContentSink::NotifyDocElementCreated(this); > > Hmm. Have we removed overlays yet? > > If not, this will get triggered for every overlay that completes. > > Might be worth conditioning on mCurrentPrototype == mMasterPrototype or > mState == eState_Master or something. The code hasn't been removed yet, but they are disabled (at least in Firefox) after Bug 1448162. So I'd be inclined to not worry about that case but sounds like it's easy to guard against so am happy to add in a condition if you'd prefer. Are there any other non-overlay / non-xul-document cases where this will run? > In any case, notifying at this point is a bit weird, since we won't create > the element until PrepareToWalk. And in particular, why not just do this in > the block in PrepareToWalk where we actually do the root element creation > and insertion? OK, I'll move it there.

(In reply to Brian Grinstead [:bgrins] from comment #50) > > Hmm. Have we removed overlays yet? > > > > If not, this will get triggered for every overlay that completes. > > > > Might be worth conditioning on mCurrentPrototype == mMasterPrototype or > > mState == eState_Master or something. > > The code hasn't been removed yet, but they are disabled (at least in > Firefox) after Bug 1448162. So I'd be inclined to not worry about that case > but sounds like it's easy to guard against so am happy to add in a condition > if you'd prefer. Just checked and it looks like the most obvious place to put this in PrepareToWalk is already guarded on `if (mState == eState_Master)` anyway.

Unfortunately it looks like picking `iframe` as a way to avoid waiting for Bug 1444285 won't work - I'm still hitting "Assertion failure: !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to backup queue)" on a marionette test, since the test defines iframe in XBL anon content: https://searchfox.org/mozilla-central/source/testing/marionette/chrome/test_anonymous_content.xul#27. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48d722ffb8f7d5fc8426bf7dadbc46dfd722096&selectedJob=174424808

I've asked for help to get bug 1444285 assigned, so hopefully we won't have to wait too long. Or I could look around for a different binding that doesn't get used inside of XBL anon content.

> it looks like the most obvious place to put this in PrepareToWalk is already guarded Right.

Comment on attachment 8968635 [details] Bug 1411707 - Notify document-element-inserted in XUL documents; https://reviewboard.mozilla.org/r/237310/#review244056 ::: dom/xul/XULDocument.cpp:2025 (Diff revision 7) > if (NS_FAILED(rv)) return rv; > > rv = AppendChildTo(root, false); > if (NS_FAILED(rv)) return rv; > > + nsContentSink::NotifyDocElementCreated(this); I'd rather we notify after the BlockOnload call. r=me with that.

(In reply to :Gijs (he/him) from comment #46) > (In reply to Brian Grinstead [:bgrins] from comment #42) > > One potential change I've thought of now is to not load the CE scripts with > > `this` (the window) as the context but instead to pass only a subset of > > objects (like `customElements`, `window`, `document`, `Services`, more as > > needed). This would make it harder to accidentally pollute all XUL windows > > when importing a script into a CE file. Although I imagine the list of what > > gets passed in would have to expand quite a bit over time as we migrate more > > bindings. It would also have the side effect of the classes (i.e. MozIframe) > > not being part of the window global - I'm not sure if that really matters > > one way or the other. > > If I understand correctly, this would mean we create a copy of > chromeGlobals.js and its scope object for every window. Can we avoid that? I > would assume that essentially, `window` and `document` can be inferred from > the bound element to which the CE stuff gets access when instantiated, and > Services.* is already a singleton. So perhaps all windows can share a single > CE scope, and we just copy references to any global CE constructors from the > CE scope to the window in which they might be needed, and invoke the > required registration on `customElements` for each new window. Or something. > Would that work? Is that a dumb idea? Are we going to have issues with leaks > if we do that because this global scope thing might hang on to the element > instances ~forever or whatever? A complication here is that the custom element class must extend XULElement (or whatever element class you want). I don't think that is available on the JSM scope, I'm not sure if that class is per-global or not either.

My only main comment here is that I expect we'll need this in the content process eventually as well.

(In reply to Dave Townsend [:mossop] from comment #63) > My only main comment here is that I expect we'll need this in the content > process eventually as well. For in-content 'about' pages? Figuring out how to share widgets with content priv HTML (and/or convert them to extend HTMLElement instead of XULElement) is a longer term project - although I'm happy to discuss what that should look like! If you are referring to in-content bindings - that plan doesn't require Custom Elements but rather Shadow DOM plus frame scripts (https://bugzilla.mozilla.org/show_bug.cgi?id=1431255#c11).

(In reply to Dave Townsend [:mossop] from comment #62) > (In reply to :Gijs (he/him) from comment #46) > > (In reply to Brian Grinstead [:bgrins] from comment #42) > > > One potential change I've thought of now is to not load the CE scripts with > > > `this` (the window) as the context but instead to pass only a subset of > > > objects (like `customElements`, `window`, `document`, `Services`, more as > > > needed). This would make it harder to accidentally pollute all XUL windows > > > when importing a script into a CE file. Although I imagine the list of what > > > gets passed in would have to expand quite a bit over time as we migrate more > > > bindings. It would also have the side effect of the classes (i.e. MozIframe) > > > not being part of the window global - I'm not sure if that really matters > > > one way or the other. > > > > If I understand correctly, this would mean we create a copy of > > chromeGlobals.js and its scope object for every window. Can we avoid that? I > > would assume that essentially, `window` and `document` can be inferred from > > the bound element to which the CE stuff gets access when instantiated, and > > Services.* is already a singleton. So perhaps all windows can share a single > > CE scope, and we just copy references to any global CE constructors from the > > CE scope to the window in which they might be needed, and invoke the > > required registration on `customElements` for each new window. Or something. > > Would that work? Is that a dumb idea? Are we going to have issues with leaks > > if we do that because this global scope thing might hang on to the element > > instances ~forever or whatever? > > A complication here is that the custom element class must extend XULElement > (or whatever element class you want). I don't think that is available on the > JSM scope, I'm not sure if that class is per-global or not either. There are ways around that. We can make the XULElement constructor available in system scopes, but it wouldn't be from the same document as the elements we're binding to, and I have no idea whether that would work. But we can always do `class Foo extends window.XULElement { ... }` for the windows we're creating elements for. The bigger concern, though, is that that would put the custom element code in a different compartment from the DOM elements they interact with, which would add a lot of extra cross-compartment wrapper overhead (both in terms of performance and in terms of memory). At least, until bug 1357862 is finished it would.

(In reply to Brian Grinstead [:bgrins] from comment #42) > Gijs, Matt, Mossop - what do you think about this general approach? > Basically: MainProcessSingleton loads chromeGlobals.js in XUL docs when a > document element gets created. Then chromeGlobals loads a set of 'core' / > 'toolkit-y' bindings that we want to have available everywhere. My only concerns are that: * things will be put in chromeGlobals.js that don't need to be in every chrome window e.g. should Services.jsm be there or is that just more confusion for contributors and eslint to understand? * the solution may not work seamlessly with all our eslint rules for our various windows. If I have to add a line in the file to teach eslint about a global defined in chromeGlobals.js then that defeats the benefit of not having to include the reference in that file and as I said above, just adds more magic for contributors to learn. Hopefully this can be solved without adding to boilerplate.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #66) > (In reply to Brian Grinstead [:bgrins] from comment #42) > > Gijs, Matt, Mossop - what do you think about this general approach? > > Basically: MainProcessSingleton loads chromeGlobals.js in XUL docs when a > > document element gets created. Then chromeGlobals loads a set of 'core' / > > 'toolkit-y' bindings that we want to have available everywhere. > > My only concerns are that: > * things will be put in chromeGlobals.js that don't need to be in every > chrome window e.g. should Services.jsm be there or is that just more > confusion for contributors and eslint to understand? > * the solution may not work seamlessly with all our eslint rules for our > various windows. If I have to add a line in the file to teach eslint about a > global defined in chromeGlobals.js then that defeats the benefit of not > having to include the reference in that file and as I said above, just adds > more magic for contributors to learn. Hopefully this can be solved without > adding to boilerplate. I was imagining that we would actually want to introduce globals to every chrome document based on examples in https://bugzilla.mozilla.org/show_bug.cgi?id=1442006#c0 (similar to what we have now with Cc/Cu/Ci/Cr). But I can see the argument against it as well. If we want to make that a separate discussion from Custom Element registration, then I could drop chromeGlobals.js entirely and instead load the array of scripts directly in MainProcessSingleton, passing in Services and other globals into the targetObj option of the subscript loader.

(In reply to Brian Grinstead [:bgrins] from comment #67) > If we want to make that a separate discussion from Custom Element > registration, then I could drop chromeGlobals.js entirely and instead load > the array of scripts directly in MainProcessSingleton, passing in Services > and other globals into the targetObj option of the subscript loader. Please don't. See my concerns about cross-compartment overhead in comment 65. If we're concerned about scope pollution, we can keep everything in chromeGlobals.js in an isolated lexical scope.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #69) > (In reply to Brian Grinstead [:bgrins] from comment #67) > > If we want to make that a separate discussion from Custom Element > > registration, then I could drop chromeGlobals.js entirely and instead load > > the array of scripts directly in MainProcessSingleton, passing in Services > > and other globals into the targetObj option of the subscript loader. > > Please don't. See my concerns about cross-compartment overhead > in comment 65. > > If we're concerned about scope pollution, we can keep everything > in chromeGlobals.js in an isolated lexical scope. OK, I can revert that change and continue to use `this` as the targetObj. IIUC then in that case we'd either need to: 1) Wrap the contents of every file that gets loaded by chromeGlobals into an IIFE or similar. And then also re-import Services, XPCOMUtils, and whatever else is used in each file (although, XBL doesn't really provide any special way to deal with imports so we should be able to generally translate whatever bindings are doing in the case where they need to load another module). 2) Wrap the contents of chromeGlobals into an IIFE or similar, and then preprocess the individual files into chromeGlobals rather than using subscriptLoader. Or is there another option that lets us avoid cross-compartment overhead?

(In reply to Brian Grinstead [:bgrins] from comment #70) > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > blocked) from comment #69) > > (In reply to Brian Grinstead [:bgrins] from comment #67) > > > If we want to make that a separate discussion from Custom Element > > > registration, then I could drop chromeGlobals.js entirely and instead load > > > the array of scripts directly in MainProcessSingleton, passing in Services > > > and other globals into the targetObj option of the subscript loader. > > > > Please don't. See my concerns about cross-compartment overhead > > in comment 65. > > > > If we're concerned about scope pollution, we can keep everything > > in chromeGlobals.js in an isolated lexical scope. > > OK, I can revert that change and continue to use `this` as the targetObj. > IIUC then in that case we'd either need to: > 1) Wrap the contents of every file that gets loaded by chromeGlobals into an > IIFE or similar. And then also re-import Services, XPCOMUtils, and whatever > else is used in each file (although, XBL doesn't really provide any special > way to deal with imports so we should be able to generally translate > whatever bindings are doing in the case where they need to load another > module). > 2) Wrap the contents of chromeGlobals into an IIFE or similar, and then > preprocess the individual files into chromeGlobals rather than using > subscriptLoader. > > Or is there another option that lets us avoid cross-compartment overhead? Let me change the question - I just pushed up a version of the patch that passes in a subset of globals from the window as the targetObj for the subscriptLoader into the script that registers the CE. Does that still have the issue you mention in Comment 65, or was that specifically about if we registered the CE in JSM files? See MainProcessSingleton.js and general.js here: https://reviewboard.mozilla.org/r/193106/diff/15#index_header

Going to try for <findbar>. It depends on a bit of Fluent work, but it'll be good to confirm we can make it work with Custom Elements.

Just a couple test failures left, including a red L10N job. Zibi, I see this error: [task 2018-05-01T20:45:42.160Z] 20:45:42 INFO - Error: Missing file: ../../dist/xpi-stage/locale-ach/chrome/ach/locale/ach/global/findbar.dtd [task 2018-05-01T20:45:42.553Z] 20:45:42 ERROR - Traceback (most recent call last): At https://treeherder.mozilla.org/logviewer.html#?job_id=176473414&repo=try&lineNumber=8441 (from this job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9ed2e55a8bbf50ee8e9548ece601f5e4a1245f&selectedJob=176473414). Is that something that will go away when we run the Fluent migration script or si there something else going on here? I don't see any references to findbar.dtd in tree after the patches are applied.

Yeah, so, unfortunately, for some historical reasons we have this test which goes red every time you change the list of DTD/properties files because it tries to run repackage on a m-c against your patch that has different list of files. I think it's very bad for developer culture because the only response is "ignore it" which trains the worst possible habits, but, here we are... so... ignore it :(

I've tried to trigger the right builds, but they're busted 'cause the try build was an artifact build, which already busted at the nightly stage.

Clearing the needinfo - I think loading the subscript with `window` as the targetObj and then wrapping contents of scripts into their own scope seems like a reasonable way to go.

Did some debugging on the remaining test failure. toolkit/content/tests/chrome/test_findbar_entireword.xul fails but only when it runs after test_bug418874.xul and test_bug437844.xul (skipping one or the other makes it work again). I think it has something to do with the RegisterUnregisterChrome.js script which turns off and on the XUL cache (nglayout.debug.disable_xul_cache). I see ASSERTION: writing to the cache file, but the XUL cache is off?: 'cache->IsEnabled()', file /builds/worker/workspace/build/src/dom/xul/nsXULElement.cpp, line 2421 in logs like https://taskcluster-artifacts.net/Y0H1zki2T8qmiLmTu-JtVA/0/public/logs/live_backing.log.

(In reply to Brian Grinstead [:bgrins] from comment #85) > Did some debugging on the remaining test failure. > toolkit/content/tests/chrome/test_findbar_entireword.xul fails but only when > it runs after test_bug418874.xul and test_bug437844.xul (skipping one or the > other makes it work again). I think it has something to do with the > RegisterUnregisterChrome.js script which turns off and on the XUL cache > (nglayout.debug.disable_xul_cache). > > I see ASSERTION: writing to the cache file, but the XUL cache is off?: > 'cache->IsEnabled()', file > /builds/worker/workspace/build/src/dom/xul/nsXULElement.cpp, line 2421 in > logs like > https://taskcluster-artifacts.net/Y0H1zki2T8qmiLmTu-JtVA/0/public/logs/ > live_backing.log. Interesting - I actually see the assertion even without the patches applied when test_bug418874.xul and test_bug437844.xul are run together.

Figured out the problem. The basic idea is that the <textbox> inside the findbar wasn't getting it's XBL binding attached. I haven't figured out why those other two tests need to run together with it to trigger this behavior, but here's what's happening: 1) findbar.xml has `<content hidden=true>`, so the migrated Custom Element implementation sets this.hidden=true in the connectedCallback 2) findBar.css overrides the [hidden=true] to mean `visibility: collapse; display: -moz-box` [0] 3) However, the global.css loading in findbar_entireword_window.xul is async, which means that findBar.css isn't loaded when `<findbar id="FindToolbar" browserid="content"/>` is parsed and connectedCallback is fired on the Custom Element. This means that the findbar is actually hidden (display: none) when the connectedCallback is run. 4) Since the node has a [browserid] attribute, it triggers this line in the connectedCallback: `setTimeout(function(aSelf) { aSelf.browser = aSelf.browser; }, 0, this);`. The `browser` setter then does a call to set `this._findField.value = ...`. However at this time, this._findField is just a plain textbox with no XBL, so `value` gets set as an expando property. 5) Once the stylesheet finishes loading, the findbar is shown and the XBL gets attached to `this._findField`. But since we've already overridden `value`, the XBL <property> for value doesn't get set up. 6) Later on, code looks for `this._findField.value`, and rather than being the actual value of the input, it's whatever was set when the `browser` setter ran. Now: why doesn't the XBL binding get created in (4) when we set this._findField.value? As discussed with :bz [1], this is because the textbox was being created with `document.createElement("textbox")` and so Element::WrapObject is called before it's actually in the document. This both prevents XBL from being attached immediately (as it has no matching CSS selector), and also prevents it from being accessed when you touch the element even once it is in the DOM. So it has to wait for layout to happen before the binding gets attached. The solution [2] is to use a DOMParser to parse the markup, then get the contents of it out as a DocumentFragment using a range, then append that fragment into the host Custom Element. This way, when you access `this._findField` the XBL binding gets eagerly attached even if the element is hidden. [0]: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/toolkit/themes/windows/global/findBar.css#23-26 [1]: https://mozilla.logbot.info/content/20180503#c14706810 [2]: https://hg.mozilla.org/try/rev/b2ee5907e865c7942bc9dc435ac9fe96ca7ff490#l6.19

The last patch in the series makes the implementation of findbar (which is pretty big) get loaded lazily when the first one is constructed. The exact details on how we should do this aren't settled, but it makes a pretty big difference on talos. Without lazy initialization we get a few reds, especially "tp5o responsiveness opt e10s stylo": https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=69440c4386abfa2ce65aa06f8245276fecbcb4e1&newProject=try&newRevision=47c571236d85099b5ed3793aad78ebce563ad9fa&framework=1&showOnlyImportant=1 With lazy initialization we do still have a few yellows, but it looks much better: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=92870fd13fa0795dd2abf846279a86aead8cedf4&newProject=try&newRevision=7b52c95f4de486d129ffd7044d4f5c71afcd55c9&framework=1&showOnlyImportant=1

Brian, please let me know if you need me to look at accessibility (screen reader exposure) for this.

(In reply to Marco Zehe (:MarcoZ) from comment #103) > Brian, please let me know if you need me to look at accessibility (screen > reader exposure) for this. Yes, thank you for the offer! Let me rebase and make a build.

(In reply to Brian Grinstead [:bgrins] from comment #104) > (In reply to Marco Zehe (:MarcoZ) from comment #103) > > Brian, please let me know if you need me to look at accessibility (screen > > reader exposure) for this. > > Yes, thank you for the offer! Let me rebase and make a build. OK, here is the build based on the latest patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1ec78b05f197613085798205e2ed0ff34bb59a6. Let me know if you have any problems getting it running. The way the <findbar> is implemented changes quite a bit as a result of these patches, but the things that seem most relevant to accessibility are: 1) Localization changed from dtd to Fluent. 2) <findbar> inner content is now built as normal DOM children instead of with XBL anonymous content. 3) The input field inside <findbar> is now a normal XUL/XBL <textbox> instead of an extended binding. That said, all the same functions/handlers are still implemented so I don't expect this will change anything.

Seeing some talos regressions still, but hoping they are related to having to include the l10n.js <script> (and the findbar localization <link>) in browser.xul. Bug 1455649 should remove the need for that. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fd605ce88c5721405410a502caabf354afa8b521&newProject=try&newRevision=a1ec78b05f197613085798205e2ed0ff34bb59a6&framework=1&showOnlyImportant=1

Comment on attachment 8968674 [details] Bug 1411707 - Convert findbar.dtd to findbar.ftl; https://reviewboard.mozilla.org/r/237344/#review251216 A couple of notes on the FTL file: * You'll need review from l10n to pass the hg commit hook (either gandalf or me work). * I need to provide you with a Python file to actually migrate the strings from .DTD to .FTL, as soon as the patch is ready for review and stable. ::: toolkit/locales/en-US/toolkit/main-window/findbar.ftl:18 (Diff revision 13) > + .accesskey = l > + .tooltiptext = Highlight all occurrences of the phrase > + > +findbar-case-sensitive = > + .label = Match Case > + .accesskey = c Do you want to highlight the c in Match(c) or (C)ase? I think the latter would be better, so C (uppercase) for .accesskey attribute ::: toolkit/locales/en-US/toolkit/main-window/findbar.ftl:23 (Diff revision 13) > + .accesskey = c > + .tooltiptext = Search with case sensitivity > + > +findbar-entire-word = > + .label = Whole Words > + .accesskey = w W (uppercase)

(In reply to Francesco Lodolo [:flod][PTO May 17-20] from comment #111) > Comment on attachment 8968674 [details] > Bug 1411707 - Convert findbar.dtd to findbar.ftl > > https://reviewboard.mozilla.org/r/237344/#review251216 > > A couple of notes on the FTL file: > * You'll need review from l10n to pass the hg commit hook (either gandalf or > me work). > * I need to provide you with a Python file to actually migrate the strings > from .DTD to .FTL, as soon as the patch is ready for review and stable. OK I'll update the patch queue and request review, since I don't expect this part to change anymore (besides the accesskey suggestions).

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf61980716902412adb1b91d98a6443dd6dab54

Yes, there is breakage unfortunately. The texbox is fine, including the label, but all buttons and checkboxes have lost their labels/accessible names. Moreover, those items that used to be checkboxes like the Highlight one, are now buttons, with a checkable state, but again, no label, and no explanatory text (AKA AccDescription) as they have in the regular Nightly. You should see the difference if you inspect the Accessibility properties with the a11y inspector while inspecting the browser chrome.

Comment on attachment 8968674 [details] Bug 1411707 - Convert findbar.dtd to findbar.ftl; https://reviewboard.mozilla.org/r/237344/#review251792 The FTL file looks good

This is the migration script. They live in https://hg.mozilla.org/mozilla-unified/file/central/python/l10n/fluent_migrations It would be great if you could attach it to the patch that adds the FTL file.

(In reply to Francesco Lodolo [:flod][PTO May 17-20] from comment #121) > Created attachment 8979499 [details] > bug_1411707_findbar.py > > This is the migration script. They live in > https://hg.mozilla.org/mozilla-unified/file/central/python/l10n/ > fluent_migrations > > It would be great if you could attach it to the patch that adds the FTL file. Done

MozReview-Commit-ID: 9BgKABCy7h3

MozReview-Commit-ID: F95i9H9FvYv

This'll make blame easier to follow when reviewing the next changeset MozReview-Commit-ID: 5BYvzivlH9I

This will be used when we migrate away from XBL and to a Custom Element in the following changesets. MozReview-Commit-ID: 7ScSAAwbgVf

Comment on attachment 9008208 [details] Bug 1411707 - Convert findbar.dtd to findbar.ftl;r=flod Francesco Lodolo [:flod] has approved the revision.

(In reply to Marco Zehe (:MarcoZ) from comment #119) > Yes, there is breakage unfortunately. The texbox is fine, including the > label, but all buttons and checkboxes have lost their labels/accessible > names. Moreover, those items that used to be checkboxes like the Highlight > one, are now buttons, with a checkable state, but again, no label, and no > explanatory text (AKA AccDescription) as they have in the regular Nightly. > You should see the difference if you inspect the Accessibility properties > with the a11y inspector while inspecting the browser chrome. With the latest version I see normal-looking accessibility inspector results. Could you please give it another try with this build? https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7f6682514f69663fdb8dde5cb31deb29d19a195.

Left is normal m-c, right is with the patches

Comment on attachment 9008773 [details] Bug 1411707 - Load findBar.css as a document stylesheet;r=paolo :Paolo Amadini has approved the revision.

Comment on attachment 9008207 [details] Bug 1411707 - hg copy findbar XBL to findbar.js; :Paolo Amadini has approved the revision.

Comment on attachment 9008206 [details] Bug 1411707 - Switch findbar and findbar-textbox from XBL bindings into a Custom Element;r=paolo :Paolo Amadini has approved the revision.

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f5860868f023294537509f4e473420612ff9aad

(In reply to Brian Grinstead [:bgrins] from comment #133) > (In reply to Marco Zehe (:MarcoZ) from comment #119) > > Yes, there is breakage unfortunately. The texbox is fine, including the > > label, but all buttons and checkboxes have lost their labels/accessible > > names. Moreover, those items that used to be checkboxes like the Highlight > > one, are now buttons, with a checkable state, but again, no label, and no > > explanatory text (AKA AccDescription) as they have in the regular Nightly. > > You should see the difference if you inspect the Accessibility properties > > with the a11y inspector while inspecting the browser chrome. > > With the latest version I see normal-looking accessibility inspector > results. Could you please give it another try with this build? > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c7f6682514f69663fdb8dde5cb31deb29d19a195. I tested in with NVDA and experience seems pretty much identical. There are some a11y deficiencies, but they present before these patches too: - group size is seems not correct - close findbar button is not reachable with the keyboard - number of matches label is not referenced anywhere, it could be a nice improvement if it was used as a description of the search text area for the/

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/96b4f5116017 Convert findbar.dtd to findbar.ftl;r=flod https://hg.mozilla.org/integration/autoland/rev/01d1d7a8b583 Load findBar.css as a document stylesheet;r=paolo https://hg.mozilla.org/integration/autoland/rev/6f58b8caa889 Switch findbar and findbar-textbox from XBL bindings into a Custom Element;r=paolo

(In reply to Brian Grinstead [:bgrins] from comment #133) > (In reply to Marco Zehe (:MarcoZ) from comment #119) > > Yes, there is breakage unfortunately. The texbox is fine, including the > > label, but all buttons and checkboxes have lost their labels/accessible > > names. Moreover, those items that used to be checkboxes like the Highlight > > one, are now buttons, with a checkable state, but again, no label, and no > > explanatory text (AKA AccDescription) as they have in the regular Nightly. > > You should see the difference if you inspect the Accessibility properties > > with the a11y inspector while inspecting the browser chrome. > > With the latest version I see normal-looking accessibility inspector > results. Could you please give it another try with this build? > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c7f6682514f69663fdb8dde5cb31deb29d19a195. For context: I wanted to get this landed to avoid rebasing again, so I did some testing with the Accessibility Inspector and asked Yura to do some sanity checks to make sure we weren't going to regress anything obvious. I'm going to leave the needinfo open because I'd still like your feedback. We can handle any issues you find in follow up bugs (assuming this sticks).

https://hg.mozilla.org/mozilla-central/rev/96b4f5116017 https://hg.mozilla.org/mozilla-central/rev/01d1d7a8b583 https://hg.mozilla.org/mozilla-central/rev/6f58b8caa889

Sorry for the delay! The find bar works now, so the accessibility inspector didn't lie to you. ;)