Migrate the findbar XBL binding to a Custom Element

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(4 attachments, 13 obsolete attachments)

46 bytes, text/x-phabricator-request
Paolo
: review+
Details | Review
46 bytes, text/x-phabricator-request
flod
: review+
Details | Review
46 bytes, text/x-phabricator-request
Paolo
: review+
Details | Review
175.52 KB, image/png
Details
(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
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).
See Also: → bug 843609
(Assignee)

Comment 2

a year ago
Going to spend some time looking into this
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1413418
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8941232 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
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?
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
(Assignee)

Comment 12

a year ago
(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.
(Assignee)

Comment 14

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1442006
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1444285
(Assignee)

Comment 16

a year ago
Renaming to reflect that this may not end up being stringbundle
Summary: Migrate <stringbundle> from a XBL binding to a Custom Element → Migrate the first XBL binding to a Custom Element (stringbundle or deck, maybe)
(Assignee)

Updated

a year ago
Depends on: 1446247
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
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.
(Assignee)

Comment 19

11 months ago
(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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 20

11 months ago
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 22

11 months ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

11 months ago
Kris' suggestion in Comment 21 plus emitting the `document-element-inserted` event from XUL content sink seems to work - clearing needinfo.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 26

11 months ago
(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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

11 months ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
Flags: needinfo?(dtownsend)
Flags: needinfo?(MattN+bmo)

Comment 43

11 months ago
(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.
(Assignee)

Comment 44

11 months ago
(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.
Flags: needinfo?(florian)

Comment 46

11 months ago
(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?

Updated

11 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 47

11 months ago
(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 49

11 months ago
mozreview-review
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?
Attachment #8968635 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 50

11 months ago
(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.
(Assignee)

Comment 51

11 months ago
(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.
(Assignee)

Comment 52

11 months ago
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
(Assignee)

Comment 53

11 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
> it looks like the most obvious place to put this in PrepareToWalk is already guarded

Right.

Comment 58

11 months ago
mozreview-review
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.
Attachment #8968635 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(dtownsend)
My only main comment here is that I expect we'll need this in the content process eventually as well.
(Assignee)

Comment 64

11 months ago
(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).
(Assignee)

Updated

11 months ago
Depends on: 1455680
(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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 67

11 months ago
(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.
(Assignee)

Updated

11 months ago
No longer depends on: 1442006
Duplicate of this bug: 1442006
(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.
(Assignee)

Comment 70

11 months ago
(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?
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8968635 - Attachment is obsolete: true
(Assignee)

Comment 73

11 months ago
(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
(Assignee)

Comment 74

11 months ago
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.
Depends on: 1455649
Summary: Migrate the first XBL binding to a Custom Element (stringbundle or deck, maybe) → Migrate the findbar XBL binding to a Custom Element
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 81

11 months ago
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.
Flags: needinfo?(gandalf)
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 :(
Flags: needinfo?(gandalf)
See Also: → bug 1458378

Comment 83

11 months ago
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.
(Assignee)

Comment 84

11 months ago
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 85

11 months ago
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.
(Assignee)

Comment 86

11 months ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8973021 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 96

11 months ago
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
(Assignee)

Updated

11 months ago
Depends on: 1459245
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 102

11 months ago
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
(Assignee)

Updated

11 months ago
Blocks: 1460982
Brian, please let me know if you need me to look at accessibility (screen reader exposure) for this.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 104

10 months ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8974519 - Attachment is obsolete: true
(Assignee)

Comment 109

10 months ago
(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.
Flags: needinfo?(bgrinstead) → needinfo?(mzehe)
(Assignee)

Updated

10 months ago
Depends on: 1462798
(Assignee)

Comment 110

10 months ago
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 111

10 months ago
mozreview-review
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)
(Assignee)

Comment 112

10 months ago
(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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Flags: needinfo?(mzehe) → needinfo?(bgrinstead)

Comment 120

10 months ago
mozreview-review
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
Attachment #8968674 - Flags: review?(francesco.lodolo) → review+
Posted file bug_1411707_findbar.py (obsolete) —
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8979356 - Attachment is obsolete: true
(Assignee)

Comment 126

10 months ago
(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
(Assignee)

Updated

9 months ago
See Also: → bug 1469339

Updated

8 months ago
Depends on: 1473932
(Assignee)

Updated

7 months ago
Depends on: 1490457
(Assignee)

Comment 127

7 months ago
MozReview-Commit-ID: 9BgKABCy7h3
(Assignee)

Comment 129

7 months ago
This'll make blame easier to follow when reviewing the next changeset

MozReview-Commit-ID: 5BYvzivlH9I
(Assignee)

Comment 130

7 months ago
This will be used when we migrate away from XBL and to a Custom Element
in the following changesets.

MozReview-Commit-ID: 7ScSAAwbgVf
(Assignee)

Updated

7 months ago
Attachment #8922105 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8968674 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8972386 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8973020 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8979499 - Attachment is obsolete: true
Comment on attachment 9008208 [details]
Bug 1411707 - Convert findbar.dtd to findbar.ftl;r=flod

Francesco Lodolo [:flod] has approved the revision.
Attachment #9008208 - Flags: review+
(Assignee)

Comment 133

6 months ago
(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.
Flags: needinfo?(bgrinstead) → needinfo?(mzehe)
(Assignee)

Comment 134

6 months ago
Left is normal m-c, right is with the patches
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1473932
(Assignee)

Updated

6 months ago
Blocks: 957999
Attachment #9008206 - Attachment description: Bug 1411707 - Switch XUL findbar from a XBL binding to a Custom Element; → Bug 1411707 - Switch findbar and findbar-textbox from XBL bindings into a Custom Element;r=paolo
Attachment #9008205 - Attachment is obsolete: true
Attachment #9008948 - Attachment is obsolete: true
Attachment #9008773 - Attachment description: Bug 1411707 - Load findBar.css as a document stylesheet. r=paolo → Bug 1411707 - Load findBar.css as a document stylesheet;r=paolo

Comment 138

6 months ago
Comment on attachment 9008773 [details]
Bug 1411707 - Load findBar.css as a document stylesheet;r=paolo

:Paolo Amadini has approved the revision.
Attachment #9008773 - Flags: review+

Comment 139

6 months ago
Comment on attachment 9008207 [details]
Bug 1411707 - hg copy findbar XBL to findbar.js;

:Paolo Amadini has approved the revision.
Attachment #9008207 - Flags: review+

Comment 140

6 months ago
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.
Attachment #9008206 - Flags: review+
(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/
Attachment #9008207 - Attachment is obsolete: true

Comment 143

6 months ago
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
(Assignee)

Comment 144

6 months ago
(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).

Comment 145

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96b4f5116017
https://hg.mozilla.org/mozilla-central/rev/01d1d7a8b583
https://hg.mozilla.org/mozilla-central/rev/6f58b8caa889
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1491526
Sorry for the delay!

The find bar works now, so the accessibility inspector didn't lie to you. ;)
Flags: needinfo?(mzehe)
(Assignee)

Updated

6 months ago
Depends on: 1491484

Updated

6 months ago
Blocks: 1470830
Depends on: 1519416
You need to log in before you can comment on or make changes to this bug.