It would be nice to not make it extend textbox and only port the needed properties/methods to unblock the normal textboxes from changing to html inputs.

Hi Neil,

I'm converting the autocomplete binding to a customized input element. The current patch pretty much works if the this.mController.input == this.nsIAutocompleteInput checks are removed. The reason the check is failing is that this.mController.input = this.nsIAutocompleteInput; in attachController() is changing this.nsIAutocompleteInput from a Proxy to a XPCWrappedNative_NoHelper, which means the this.mController.input == this.nsIAutocompleteInput checks are never true.

I tried doing:

 this.mController.input = this.nsIAutocompleteInput;
 this.nsIAutocompleteInput = this.mController.input;

in attachController(), which successfully transforms this.nsIAutocompleteInput into a XPCWrappedNative_NoHelper and makes the checks true, but it causes a NS_ERROR_FAILURE in autocomplete-popup.js.

For reference, this.nsIAutocompleteInput is set to this.getCustomInterfaceCallback(Ci.nsIAutoCompleteInput) in this patch, I also tried using QI, but that made this.mController.input = this.nsIAutocompleteInput; unhappy.

So far I've checked current usages of MozXULElement.implementCustomInterface, and none of them seem to do equality checks like the current patch does.

What I could do is keep a second field which I set to this.mController.input in attachController and I could do the equality check on that field instead, but it does sound rather ugly IMO.

For completeness, the autocomplete binding just does the following check: this.mController.input == this, where this is the autocomplete binding.

Is there a way we could make that check work in a CE world? or is there an alternative check that could be done ?

Just to give an update of this bug:

• This depends on the removal of legacy-urlbar (the current patch breaks the old urlbar) and possibly autocomplete-rich-result-popup (not sure it's really needed but it will get removed together with legacy-urlbar anyway)
• Tests/reftests need to be ported with the new markup (<html:input is="autocomplete-input"/> instead of <textbox type="autocomplete"/>) and possibly new selectors

There are also some XXX(ntim) comments in the patch:

• One regarding comment 5
• One regarding bug 1556566 (although just wrapping the searchbar.js usage with <moz-input-box> would also do the job without needing to fix bug 1556566)
• clickSelectsAll needs to ported, this is pretty easy, see bug 1558574. Bug 1528614 is getting rid of clickSelectsAll

Tim, we would like to remove the usage of |eval()/new function()| from autocomplete.xml over within Bug 1498560. It seems its a time consuming task and we where wondering if you are planning to remove autocomplete.xml entirely. If so, then obviously then it doesn't make sense for us to invest any time in fixing it.

If you are planning to remove autocomplete.xml, what's the timeline for it? Thanks!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)

Tim, we would like to remove the usage of |eval()/new function()| from autocomplete.xml over within Bug 1498560. It seems its a time consuming task and we where wondering if you are planning to remove autocomplete.xml entirely. If so, then obviously then it doesn't make sense for us to invest any time in fixing it.

If you are planning to remove autocomplete.xml, what's the timeline for it? Thanks!

Converting the binding to a custom element would likely just move that code; you can see this in ntim's unfinished patch. So bug 1498560 would still apply. It could be fixed before or after this bug, doesn't really matter.

I've got the issue from comment 5 resolved locally, still working through other test failures.

Neil, this is still in progress but getting close to green. However, I'm stumped on this failure:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257065456&repo=try&lineNumber=6320

From what I can gather, this is testing this code:
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/toolkit/components/autocomplete/nsAutoCompleteController.cpp#493-510
and it appears to be doing the right thing, but the test is still receiving unwanted keypress events.

I'm a bit over my head on debugging this. Do you have time to take a look at the attached patch and see what's going wrong in this test?

Re-reading I realize this could be more detailed. My understanding is that the nsAutoCompleteController code linked in comment 10 should cause keypress events to be suppressed here:
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/toolkit/content/widgets/autocomplete.xml#482-485

And this test creates a listener here and asserts that it doesn't get called:
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul#54-56

The converted handler is here:
https://hg.mozilla.org/try/file/6ba2bea28ac277aa13f19aa773dd4b7caaea68c0/toolkit/content/widgets/autocomplete-input.js#l18

I've manually verified that this handler is calling stopPropagation() in this case, but the listener added by the test is still invoked. Is there any sort of special treatment of event handlers in XBL bindings that would be different with the custom element that would explain why this is behaving differently?

Unrelated to comment 10 and comment 11, I'm not sure how we should be handling accessibility for converted elements.
For the old <textbox type="autocomplete"> widgets, the accessibility tree looks like this:
https://searchfox.org/mozilla-central/rev/96403eac4ff6f96f89a0b120b29bd46540d5705e/accessible/tests/mochitest/tree/test_txtctrl.xul#95-121

However, now we're getting rid of the outer <textbox> instance and just using <html:input> directly. It would be simple enough to make input elements that have the autocomplete binding attached have role AUTOCOMPLETE but I don't know enough about how the accessibility data is used to know if that actually makes sense. I would ask Alex but he's on PTO for a few weeks. Yura, is this a question you can help with? Or if not can you help me find somebody who can?

I think this link has some good best practices of authoring auto-completes with roles/properties and interactions: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html#rps_label . The pattern there at least suggests to use a containing element for both input and the list of autocomplete values with the role combobox. I'm also including Marco here, I'm sure he will have good suggestions too.

There are two issues here:

1. You are now adding the autocomplete keypress listener to the <input> whereas before it was being added to the parent textbox. This means that stopPropagation won't stop the listener added by the test to the same element. (but stopImmediatePropagation will do this) The fix here is to just change to test to listen to the parent element or window or something that comes afterwards in the event bubbling order.
2. SpecialPowers.addSystemEventListener takes a boolean 'capture' flag as its fourth argument but is being passed { once: true } instead (which will get converted into a true value). To fix (1) above you will need to pass false here, then remove the listener afterwards.

Re comment 14, thanks Neil!

Re comment 13, I'm now more confused than before. The page linked in that comment is about the combox role which is a standard part of aria. But XUL <textbox type="autocomplete"> elements use this (non-standard?) role:
https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/accessible/base/Role.h#629

After this bug, these elements will be turned into something else (ideally HTML <input>, but it could be another container for the purpose of the a11y tree if that's desirable). But I don't know what the desired final state is, Marco can you give some guidance?

Adding :Jamie for additional input on comment 15.

In case it's not clear, the autocomplete textbox that's mentioned here is used in the following places:

• browser searchbar
• homepage change preference (when you choose to pick custom URLs)
• typing tags in the "New bookmark" popup/bookmarks window

On Windows, ROLE_AUTOCOMPLETE maps to ROLE_SYSTEM_COMBOBOX. However, ATK (Linux) seems to have a special ATK_ROLE_AUTOCOMPLETE.

Joanie, I'm a little confused about the expected hierarchy for ATK_ROLE_AUTOCOMPLETE. Would you expect this to be a container or could it also be on the editable text element itself? Would you expect the autocomplete results list to be a child of ATK_ROLE_AUTOCOMPLETE? And what role would you expect the results list to be (listbox or menu)?

Andrew: One problem I see for a11y with eliminating textbox is that now, the results list ends up as a child of the text input, which in turn causes a11y to see this as part of the text of the input. Ideally, I think we'd want something wrapping the input which could contain the input and the popup. The wrapper would have role combobox (or autocomplete, pending Joanie's feedback). Failing that, we'd need some way to make the popup a sibling of the input instead of a child (in the a11y hierarchy).

(In reply to James Teh [:Jamie] from comment #18)

Andrew: One problem I see for a11y with eliminating textbox is that now, the results list ends up as a child of the text input, which in turn causes a11y to see this as part of the text of the input.

I'm curious, where is the code that currently implements the behaviour making the results list the child of the textbox in the a11y tree ? I'm asking this, since XUL/XBL markup-wise, the autocomplete popup isn't a child of the autocomplete textbox in most cases where we use them.

One example is the browser searchbar, where #PopupSearchAutoComplete is outside <searchbar> (which contains the <textbox type="autocomplete">, which itself contains the input). Both are linked using the autocompletepopup attribute. This is also the case for the bookmark usages.

Ideally, I think we'd want something wrapping the input which could contain the input and the popup. The wrapper would have role combobox (or autocomplete, pending Joanie's feedback). Failing that, we'd need some way to make the popup a sibling of the input instead of a child (in the a11y hierarchy).

Can this be implemented in the a11y tree without having to force this on the XUL markup ? I'm assuming it's probably possible since the a11y tree of the browser searchbar is already decoupled from its actual XUL markup. It would really be nice not having to maintain the textbox wrapper that we currently have.

(In reply to Tim Nguyen :ntim from comment #19)

I'm curious, where is the code that currently implements the behaviour making the results list the child of the textbox in the a11y tree ? I'm asking this, since XUL/XBL markup-wise, the autocomplete popup isn't a child of the autocomplete textbox in most cases where we use them.

Yeah, I was trying to figure this out myself. I finally found it:
https://searchfox.org/mozilla-central/source/browser/components/search/content/searchbar.js#715
It's done with aria-owns. Removing that would stop it from being a child. However, it would also completely disassociate the results list from the input as far as a11y is concerned.

Can this be implemented in the a11y tree without having to force this on the XUL markup ? I'm assuming it's probably possible since the a11y tree of the browser searchbar is already decoupled from its actual XUL markup.

I guess so, but I'd really prefer we didn't make super heavy XUL specific exceptions in the a11y C++ code, especially since one goal of de-XBL is to standardise on web technologies. In contrast, aria-owns is standard web tech. The problem, though, is that we can't aria-owns the list as a sibling; aria-owns only works for adopting children.

It would really be nice not having to maintain the textbox wrapper that we currently have.

Can you explain the reasons for this? I'm lacking a lot of context here and understanding more details here might help me come up with a better solution for a11y.

For example, is there any reason consumers couldn't see it as an input tag, but internally (inside its shadow root), it has a wrapper with an input inside that wrapper?

(In reply to James Teh [:Jamie] from comment #20)

For example, is there any reason consumers couldn't see it as an input tag, but internally (inside its shadow root), it has a wrapper with an input inside that wrapper?

I guess that'd mean you can't extend HTMLInputElement, which means consumers can't just use the normal input attributes, properties and methods (unless you forward them all). Is that also the motivation for killing the textbox wrapper?

(In reply to James Teh [:Jamie] from comment #20)

(In reply to Tim Nguyen :ntim from comment #19)

I'm curious, where is the code that currently implements the behaviour making the results list the child of the textbox in the a11y tree ? I'm asking this, since XUL/XBL markup-wise, the autocomplete popup isn't a child of the autocomplete textbox in most cases where we use them.

Yeah, I was trying to figure this out myself. I finally found it:
https://searchfox.org/mozilla-central/source/browser/components/search/content/searchbar.js#715
It's done with aria-owns. Removing that would stop it from being a child. However, it would also completely disassociate the results list from the input as far as a11y is concerned.

Ah nice, thanks for figuring this out!

It would really be nice not having to maintain the textbox wrapper that we currently have.

Can you explain the reasons for this? I'm lacking a lot of context here and understanding more details here might help me come up with a better solution for a11y.

There are 2 main reasons I want to get rid of the textbox wrapper:

• Having the attribute/property/event/method call forwarding from the container to the input around means this stuff needs to be covered by tests.
• There's a lot of CSS that simply forwards the styling from the input to the container that could be removed. There are also a bunch of reftests that just check that an unstyled XUL textbox looks like an unstyled HTML input to make sure the default CSS works correctly.

It would be nice to be able to clean up most of this stuff (if not all) once all textbox usages are gone (see bug 1513325).

For example, is there any reason consumers couldn't see it as an input tag, but internally (inside its shadow root), it has a wrapper with an input inside that wrapper?

This would still have the problems that I've mentioned above unfortunately, since you'd have to implement attribute/property/event/method call forwarding, and potentially styling too.

But from what you mentioned, I think we can simply add those wrappers manually (i.e. without any custom element/shadow dom involved) exclusively for a11y like this:

For home.inc.xul/editBookmarkPanel.inc.xul:

<box aria-owns="PopupAutocomplete">
  <html:input id="..." is="autocomplete-input" [...] />
</box>

For searchbar.js, we don't even need to add an extra wrapper, since <searchbar> is already a wrapper itself that only contains elements relevant to it, meaning we can probably just change this.textbox.setAttribute("aria-owns", this.textbox.popup.id); to this.setAttribute("aria-owns", this.textbox.popup.id); and preserve the same tree.

What do you think ?

(In reply to Tim Nguyen :ntim from comment #22)

There are 2 main reasons I want to get rid of the textbox wrapper:
...

Thanks for explaining. I suspected something like that and started to use my brain a bit more in comment 21. :)

But from what you mentioned, I think we can simply add those wrappers manually (i.e. without any custom element/shadow dom involved) exclusively for a11y like this:

Yes, we can. It's a bit sad that we have to rely on the consumer to know they need a11y specific stuff, though, rather than that being encapsulated by the widget. Still, I don't really see an alternative.

<box aria-owns="PopupAutocomplete">
  <html:input id="..." is="autocomplete-input" [...] />
</box>

We'd probably want a role="combobox" on that box (and same for any other cases of this; e.g. searchbar).

Note that we're still pending Joanie's input re the autocomplete role (vs combobox role). If we do need the autocomplete role, that's something we're going to somehow need to deal with in the a11y code, since that's not a web standard thing.

Nothing to add from my end so far.

Thanks Tim and Jamie, it sounds like we've settled on a plan here. I'll see about putting some logic into the autocomplete-input to either set up or verify that we have set up the a11y structure properly. There's some tension here because our testing it set up based on the idea that autocomplete is a resuable widget that can be unit tested in one place, but given that we only have 3 instances in Firefox and one (the search bar) is heavily customized, it just doesn't make sense to test it that way.

On that note, do we have existing tests to verify the a11y structure of browser chrome? Or in other words, if we encode the a11y structure directly in the frontend as described in comment 23, where should i put a test to catch regressions that break that? I did a quick search and couldn't find any, I'm not sure if I'm just not looking in the right place or if we just don't have any such tests.

(In reply to Andrew Swan [:aswan] from comment #25)

On that note, do we have existing tests to verify the a11y structure of browser chrome? Or in other words, if we encode the a11y structure directly in the frontend as described in comment 23, where should i put a test to catch regressions that break that? I did a quick search and couldn't find any, I'm not sure if I'm just not looking in the right place or if we just don't have any such tests.

While we do have tests which verify the a11y tree resulting from specific snippets of markup, I don't think we have any that verify the tree for specific pieces of browser chrome. (I did add a11y tests for the URL bar last year, but they're more about a11y events than tree structure.) That said, we could add tests in accessible/tests/browser/tree. We would use the testAccessibleTree function similar to the place you referenced in comment 12 for the old <textbox> implementation:
https://searchfox.org/mozilla-central/rev/96403eac4ff6f96f89a0b120b29bd46540d5705e/accessible/tests/mochitest/tree/test_txtctrl.xul#95-121
However, we'd run this on the actual browser search bar. Since you don't really have any good examples to follow as to how to do this, I'd be happy to help here.

:Jamie, does the test in the patch here seem reasonable for testing the searchbar a11y tree layout? If it does, I'll feel quite a bit more confident about making the changes we talked about earlier.

The autocomplete binding was also used in the edit bookmarks panel (for entering tags) and in the section of about:preferences where the homepage can be set. Should I add similar tests for those?

We may at some point stop using toolkit autocomplete and a tree for the search bar to make it work more like the address bar. If your intent is to test autocomplete + tree accessibility, better not use the search bar for that.

(In reply to Dão Gottwald [::dao] from comment #28)

We may at some point stop using toolkit autocomplete and a tree for the search bar to make it work more like the address bar. If your intent is to test autocomplete + tree accessibility, better not use the search bar for that.

This is about making sure the search bar doesn't regress, regardless of how it is implemented. This is especially necessary because some of the a11y semantics have to be done outside of the XUL widget (by the consumer), since we need the parent role="combobox" container.

(In reply to Dão Gottwald [::dao] from comment #28)

We may at some point stop using toolkit autocomplete and a tree for the search bar to make it work more like the address bar. If your intent is to test autocomplete + tree accessibility, better not use the search bar for that.

The intention here isn't to test the autocomplete CE itself. This patch moves the a11y tree setup out of the CE (see comments around 22/23) for more detail, so we can't rely on a test that just creates a new autocomplete widget and checks its a11y tree.

Somewhat related, I tried what was described in comment 22 but the <panel> linked with aria-owns isn't showing up in the a11y tree. This shows up as a failure in accessible/tests/browser/browser_searchbar.js, it can also be seen with the Accessibility panel in the browser toolbox. Alex, when you're back can you take a look? I think this is the last remaining big issue before this will be ready for review.

(In reply to Andrew Swan [:aswan] from comment #32)

Somewhat related, I tried what was described in comment 22 but the <panel> linked with aria-owns isn't showing up in the a11y tree. This shows up as a failure in accessible/tests/browser/browser_searchbar.js, it can also be seen with the Accessibility panel in the browser toolbox. Alex, when you're back can you take a look? I think this is the last remaining big issue before this will be ready for review.

The #PopupSearchAutoComplete panel does show up in the a11y tree for me as a child of #searchbar with the patches applied, it's just that it appears as "group" as role instead of "combobox-list". It sounds like this was intended since role="group" is set on the popup.

(In reply to Tim Nguyen :ntim from comment #33)

The #PopupSearchAutoComplete panel does show up in the a11y tree for me as a child of #searchbar with the patches applied, it's just that it appears as "group" as role instead of "combobox-list". It sounds like this was intended since role="group" is set on the popup.

Huh, strange. What platform? And you see it in the accessibility panel of the browser toolbox?

It looks like you have to get the popup to show at least once (by typing a search query for example) before it shows up in the a11y tree.

(In reply to Andrew Swan [:aswan] from comment #32)

Somewhat related, I tried what was described in comment 22 but the <panel> linked with aria-owns isn't showing up in the a11y tree. This shows up as a failure in accessible/tests/browser/browser_searchbar.js, it can also be seen with the Accessibility panel in the browser toolbox. Alex, when you're back can you take a look? I think this is the last remaining big issue before this will be ready for review.

it could be because <panel> is not yet rendered (if no associated layout frame object, then no accessible object). Previous test used to call popup.initialize() to trigger <panel> accessible object creation, you might need to make a similar thing in the new test. At least this is the first thing to check I'd say.

There's a whole lot of orange here but I think it is all known intermittents:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7d0b99b38f27d9be13615f0ed97b469edeb9e78

Tim, with aswan leaving is this something you might have time to drive over the finish line?

Dão, Is the review something you could get to this week ? We could ask someone else if not.

See also: https://phabricator.services.mozilla.com/D33250#1350686

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

Backed out changeset e859a5aebb5b for causing failures at test_bug437844.xul

Backout link: https://hg.mozilla.org/integration/autoland/rev/17e8e912e185c5c3543e6bc248c78ec5e2401c03

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C7%2Cdebug%2Cmochitests%2Cwithout%2Ce10s%2Ctest-windows7-32%2Fdebug-mochitest-chrome-1proc-3%2Cm-1proc%28c3%29&tochange=17e8e912e185c5c3543e6bc248c78ec5e2401c03&fromchange=0fe71f31e0450543ab885bde8d172e629aec9898&selectedJob=266969328

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266969328&repo=autoland&lineNumber=26409

[task 2019-09-17T03:48:43.516Z] 03:48:43 INFO - GECKO(1292) | MEMORY STAT | vsize 882MB | vsizeMaxContiguous 340MB | residentFast 333MB | heapAllocated 122MB
[task 2019-09-17T03:48:43.516Z] 03:48:43 INFO - GECKO(1292) | JavaScript error: , line 0: uncaught exception: [fluent][resolver] errors in en-US/file-dd: ReferenceError: Unknown variable: $pluginLibraries.
[task 2019-09-17T03:48:43.516Z] 03:48:43 INFO - TEST-OK | toolkit/content/tests/chrome/test_bug437844.xul | took 2077ms
[task 2019-09-17T03:48:43.516Z] 03:48:43 INFO - GECKO(1292) | ++DOMWINDOW == 87 (1A0EF000) [pid = 3292] [serial = 121] [outer = 1DE2BDC0]
[task 2019-09-17T03:48:43.516Z] 03:48:43 INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug437844.xul | assertion count 55 is more than expected 14 to 54 assertions
[task 2019-09-17T03:48:43.517Z] 03:48:43 INFO - TEST-START | toolkit/content/tests/chrome/test_bug451540.xul

https://hg.mozilla.org/mozilla-central/rev/9ec431469e37