1522440 - Port browser_test_focus_urlbar.js to the new address bar implementation

Status: RESOLVED FIXED

(Firefox :: Disability Access, task, P2)

Description

[Mark Banner (:standard8)]

I spotted these tests are failing with QuantumBar enabled:

TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_test_focus_urlbar.js | Uncaught exception - Waiting for search to complete - timed out after 50 tries
TEST-UNEXPECTED-FAIL | accessible/tests/browser/general/browser_test_urlbar.js | Test timed out -
TEST-UNEXPECTED-TIMEOUT | accessible/tests/browser/general/browser_test_urlbar.js | application timed out after 370 seconds with no output

This either needs porting or skipping (and later removal) for Quantumbar.

I think we might want to skip it, because we're no longer using richlistbox in the address bar results.

Comment 1

[Marco Zehe (:MarcoZ)]

Skipping and removing is not an option because this test specifically tests the interaction of the keyboard with the autocomplete stuff and makes sure the experience for screen reader users remains consistent and pleasant. So whichever the new interaction model is it must be accessible and working with the keyboard. So, porting is the only way forward.

Comment 2

[Marco Bonardo [:mak]]

I'd probably suggest to wait a few days before trying to move them around, we should first fix the urlbar tests so we are sure things are in a workable state.

Comment 3

[Mark Banner (:standard8)]

Bug 1522489 and dependencies will be adding helper functions for tests for the URLBar, hence we'll want to wait until at least that before fixing this, so adding it as a dependency.

When I mentioned about skipping/removing I was looking at browser_test_urlbar.js which seems to be just checking the popup has a richlistbox which gets the role ROLE_COMBOBOX_LIST.

browser_test_focus_urlbar.js will definitely need porting.

Comment 4

[James Teh [:Jamie]]

(In reply to Mark Banner (:standard8) from comment #3)

When I mentioned about skipping/removing I was looking at browser_test_urlbar.js which seems to be just checking the popup has a richlistbox which gets the role ROLE_COMBOBOX_LIST.

The fact that it is a richlistbox is an implementation detail and will need porting. The fact that it gets ROLE_COMBOBOX_LIST is really what we're testing here. Unfortunately, I have a nasty suspicion there's no way to reproduce that role from HTML. IIUC, the reason that role exists is because it needs to be exposed differently on different platforms: a list on Windows, a menu on other platforms.

browser_test_focus_urlbar.js will definitely need porting.

I wrote that test knowing there would be a new URL bar implementation soon. There are details that will need tweaking (e.g. waitForSearchFinish relies on implementation details that probably changed). However, the bulk of the test (the actual event checking) should not change. If it has to be changed, the implementation is almost certainly wrong in terms of the events it is firing. As I understand it, the intent is to keep the behaviour from the user's perspective mostly the same between the two implementations. That should also apply to a11y.

Comment 5

[alexander :surkov (:asurkov)]

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

(In reply to Mark Banner (:standard8) from comment #3)

When I mentioned about skipping/removing I was looking at browser_test_urlbar.js which seems to be just checking the popup has a richlistbox which gets the role ROLE_COMBOBOX_LIST.

The fact that it is a richlistbox is an implementation detail and will need porting. The fact that it gets ROLE_COMBOBOX_LIST is really what we're testing here. Unfortunately, I have a nasty suspicion there's no way to reproduce that role from HTML.

besides HTML:select, role="listbox" inside role="combobox" does the trick.

Comment 6

[Mark Banner (:standard8)]

I'm splitting out the browser_test_urlbar.js fix into bug 1534661 as I happened on a fix for it whilst looking at bug 1524564.

Comment 7

[Mark Banner (:standard8)]

Since this might help with knowing where to start fixing browser_test_focus_urlbar.js, MarcoZ has pointed out that the current implementation of QuantumBar auto-focuses the first item as soon as someone starts typing in the urlbar. It should only auto-focus after the user has pressed the down arrow (or whatever to get to the results).

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mark Banner (:standard8) from comment #7)

Since this might help with knowing where to start fixing browser_test_focus_urlbar.js, MarcoZ has pointed out that the current implementation of QuantumBar auto-focuses the first item as soon as someone starts typing in the urlbar. It should only auto-focus after the user has pressed the down arrow (or whatever to get to the results).

Yes, I'm aware. The suppressMenuItemEvent stuff in urlbarBindings.xml implements that.

Comment 9

[Dão Gottwald [:dao]]

Regarding this failure:

FAIL Focussed [DOM node id: urlbarView-row-1, role: combobox option, name: 'test visit for http://example2.com/—example2.com', address: [xpconnect wrapped nsIAccessible]] must be focusable!got '0', expected 'focusable'

For options to be focusable, I would expect nsGkAtoms::listitem to have eFocusableUntilDisabled (as listbox does):

https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/accessible/base/ARIAMap.cpp#755-778

Is the test wrong or is nsGkAtoms::listitem wrong or am I missing something else?

Comment 10

[alexander :surkov (:asurkov)]

listitem (which might appear confusing) is an item for list role (matches HTML:ul), not listbox role (HTML:select) which btw is expected to contain option roles (HTML:option), see https://www.w3.org/WAI/PF/aria/roles#listitem. So listitem shouldn't be focusable in general.

I'm not 100% sure what problem you deal with, but if you have listitem roles under listbox role then you should change those to option role.

Comment 11

[Dão Gottwald [:dao]]

(In reply to alexander :surkov (:asurkov) from comment #10)

listitem (which might appear confusing) is an item for list role (matches HTML:ul), not listbox role (HTML:select) which btw is expected to contain option roles (HTML:option), see https://www.w3.org/WAI/PF/aria/roles#listitem. So listitem shouldn't be focusable in general.

I'm not 100% sure what problem you deal with, but if you have listitem roles under listbox role then you should change those to option role.

We use role="option" and that doesn't have eFocusableUntilDisabled either so I guess I still have the same question:

https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/accessible/base/ARIAMap.cpp#906-917

Comment 12

[James Teh [:Jamie]]

Can you provide the full stack trace for the failure in question?

It's true that role="option" isn't focusable unconditionally. That makes sense, since you can't programmatically focus it. However, once you apply aria-activedescendant, it's supposed to become focusable:
https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#1262

Unfortunately, that isn't working here because the target of aria-activedescendant isn't actually a descendant. That's technically an ARIA spec violation, but it's a useful hack which makes life a lot easier in cases like these. I didn't realise this would cause problems for the focusable state, though.

The intuitive solution here is to make the list box "owned" by the input combo box; i.e. add aria-owns="urlbarView-results" to #urlbar. Unfortunately, that causes all sorts of other breakage I haven't tracked down yet.

You could perhaps hack around this by setting tabindex="-1" on the list box options. That would allow you to move forward until we can figure out how to fix this properly in the a11y module. However, I'm not sure if that causes other problems for you or whether it's something you can live with.

Comment 13

[James Teh [:Jamie]]

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

Unfortunately, that isn't working here because the target of aria-activedescendant isn't actually a descendant. That's technically an ARIA spec violation, but it's a useful hack which makes life a lot easier in cases like these. I didn't realise this would cause problems for the focusable state, though.

I filed bug 1543561 for this and have a patch which should fix it.

Comment 14

[Dão Gottwald [:dao]]

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

The intuitive solution here is to make the list box "owned" by the input combo box; i.e. add aria-owns="urlbarView-results" to #urlbar. Unfortunately, that causes all sorts of other breakage I haven't tracked down yet.

We set it on the <input>:

https://searchfox.org/mozilla-central/rev/69ace9da347adcc4a33c6fa3d8e074759b91068c/browser/base/content/urlbarBindings.xml#38

The textbox is going to be removed and the input is going to become non-anonymous in bug 1513337.

Comment 15

[James Teh [:Jamie]]

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

We set it on the <input>:

https://searchfox.org/mozilla-central/rev/69ace9da347adcc4a33c6fa3d8e074759b91068c/browser/base/content/urlbarBindings.xml#38

Oh, I didn't realise that. Despite being intuitive, that's going to cause some undesirable side effects... if it worked. Right now, it isn't working at all (it's a no-op), nor is aria-controls. I think that's because the input is anonymous, so the ids are in a different namespace.

The textbox is going to be removed and the input is going to become non-anonymous in bug 1513337.

That would probably solve some problems here, but it looks like that's a way off, so I guess we'll need to come up with an interim solution here.

Comment 16

[alexander :surkov (:asurkov)]

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

The textbox is going to be removed and the input is going to become non-anonymous in bug 1513337.

That would probably solve some problems here, but it looks like that's a way off, so I guess we'll need to come up with an interim solution here.

It's already a second issue (the first one was bug 1534661) that bug 1513337 could resolve. I guess I could give it a shot if :dao is ok with that.

Comment 17

[Dão Gottwald [:dao]]

I'd prefer lifting the anon->non-anon reference restriction as I also mentioned here: https://phabricator.services.mozilla.com/D27021#799198

Comment 18

[alexander :surkov (:asurkov)]

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

I'd prefer lifting the anon->non-anon reference restriction as I also mentioned here: https://phabricator.services.mozilla.com/D27021#799198

I would avoid doing this for number of reasons, I left a comment in phabricator.

Comment 19

[Dão Gottwald [:dao]]

With bug 1543561 fixed the test now times out here: https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/accessible/tests/browser/events/browser_test_focus_urlbar.js#155

Comment 20

[Dão Gottwald [:dao]]

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

With bug 1543561 fixed the test now times out here: https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/accessible/tests/browser/events/browser_test_focus_urlbar.js#155

The reason seems to be that we incorrectly autofill after backspace, and then the result list is too short, and instead of selecting a result the test selects a one-off search button.

Comment 21

[Drew Willcoxon :adw]

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

The reason seems to be that we incorrectly autofill after backspace, and
then the result list is too short, and instead of selecting a result the
test selects a one-off search button.

I think this is wrong. See bug 1545731.

Comment 22

[Drew Willcoxon :adw]

Attachment: awesomebar.png

Following up on bug 1545731 comment 5, I'll attach a couple of screenshots of what the test looks like on quantumbar vs. awesomebar, when this line here is reached: https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/accessible/tests/browser/events/browser_test_focus_urlbar.js#130

The difference between the two starts when the test synthesizes left and right arrow keys before that line. On awesomebar, that closes the popup, but the popup remains open on quantumbar. Then when the test synthesizes the down arrow key, on awesomebar the popup opens and the first result is selected, but on quantumbar, since the popup remains open, the third result ends up being selected.

Then when the test presses down and backspace, on awesomebar the value in the input is example2.com, which doesn't get autofilled after the test backspaces. On quantumbar, the value is example example2.com/blah, which does get autofilled on backspace. (Awesomebar will autofill this too, but the test doesn't hit this case.)

I tried manually closing the popup before the test presses the down arrow key. That reveals another difference between awesomebar and quantumbar: when the popup opens, on awesomebar, it includes an "example2.com" search suggestion, but it does not on quantumbar.

Comment 23

[Drew Willcoxon :adw]

Attachment: quantumbar.png

Comment 24

[James Teh [:Jamie]]

FWIW, if the behaviour in Quantumbar is intentionally different with respect to when the popup opens/closes/autofills for backspace + left/right arrows, it makes sense to just adjust the a11y test accordingly. The goals for the test are to ensure that:

1. When typing, a11y focus should always be on the input. That is, typing should never move a11y focus to a suggestion, and if a suggestion has a11y focus, a11y focus should be moved back to the input.
2. Down arrow/up arrow (and other keystrokes on mac) should move a11y focus to the selected suggestion (if any).
3. Backspace, as well as left arrow and right arrow (shifted or not), must restore a11y focus to the input if a11y focus was on a suggestion. Whether the autocomplete is expanded or collapsed at that point is irrelevant; that's just an aspect of the current Awesomebar we're testing against.

Comment 25

[Drew Willcoxon :adw]

Yeah, we usually just adjust tests so they work in quantumbar, or both, as necessary. I'll leave it to Dao to decide what to do here, and it may be the case that some of the quantumbar behavior this tests uncovered isn't intentional/desireable. Sometimes behavior is different between quantumbar and awesomebar and it's no big deal, or the awesomebar behavior might even be wrong but unless it's a big problem we won't spend time fixing it. Regardless, IMO we should keep awesomebar coverage -- i.e., modify the test as necessary so that it works and runs under both quantumbar and awesomebar.

Comment 26

[Dão Gottwald [:dao]]

Attachment: Bug 1522440 - Explicitly close the view since KEY_ArrowLeft won't automatically do that in the quantumbar. r=adw

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db10a6b3d728
Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw

Comment 28

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/db10a6b3d728

Comment 29

[Dão Gottwald [:dao]]

<Standard8> dao: I just tried another QuantumBar run, and browser_test_focus_urlbar.js is still timing out. Shall I file a new bug for that? https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b7c6a07564cb51e0576be49e62aaf0ca4947df&selectedJob=243545782
<dao> Standard8: I'll reopen. I think I screwed up running the test locally, was probably with the quantumbar disabled

Comment 30

[Dão Gottwald [:dao]]

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

3. Backspace, as well as left arrow and right arrow (shifted or not), must restore a11y focus to the input if a11y focus was on a suggestion. Whether the autocomplete is expanded or collapsed at that point is irrelevant; that's just an aspect of the current Awesomebar we're testing against.

The test now times out here:
https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/accessible/tests/browser/events/browser_test_focus_urlbar.js#149

Calling gURLBar.view.removeAccessibleFocus doesn't seem to help, the test still times out there. I'm not quite sure why we wait for the focus event and how this works in the legacy urlbar -- the textbox input never really loses DOM focus, so gURLBar.input.focus() would be a no-op (and I expect that FocusManager wouldn't dispatch the focus event). The a11y focus event we're waiting for here depends on the DOM event, right?

Comment 31

[James Teh [:Jamie]]

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

The test now times out here:
https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/accessible/tests/browser/events/browser_test_focus_urlbar.js#149

Curious. Testing locally, pressing backspace while a suggestion has a11y focus does seem to push a11y focus back to the input, which is what that test is supposed to be testing. I'm not in a position to be able to run this test locally just now... :(

I'm not quite sure why we wait for the focus event and how this works in the legacy urlbar -- the textbox input never really loses DOM focus, so gURLBar.input.focus() would be a no-op (and I expect that FocusManager wouldn't dispatch the focus event). The a11y focus event we're waiting for here depends on the DOM event, right?

No. The input never loses focus, as you say. However, setting/clearing aria-activedescendant moves a11y focus and causes a11y focus events to be fired. When a suggestion is selected using the up/down arrow keys, we set aria-activedescendant and thus that suggestion gets a11y focus. When the user types some text, presses left arrow, right arrow, backspace, etc., we clear aria-activedescendant and thus a11y focus returns to the input. I would have thought that at the very least, calling gURLBar.view.removeAccessibleFocus before waiting for the event would make this pass, but you tried that already.

Comment 32

[Mike de Boer [:mikedeboer]]

We're enabling QuantumBar now, with this test disabled temporarily. This bug will be responsible to re-enable it, once a solution has been found.

Comment 33

[Dão Gottwald [:dao]]

Attachment: Bug 1522440 - Remove a11y focus from UrlbarView on backspace.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 9062190 [details]
Bug 1522440 - Remove a11y focus from UrlbarView on backspace.

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

Here's what I tried specifically.

Comment 35

[Marco Bonardo [:mak]]

Note, Mike temporarily disabled this test in bug 1548031, you must re-enable it in browser.ini

Comment 36

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] (Away 22 -28 Apr) from comment #35)

Note, Mike temporarily disabled this test in bug 1548031, you must re-enable it in browser.ini

I just integrated this into my patch now.

Comment 37

[Phabricator Automation]

Comment on attachment 9062190 [details]
Bug 1522440 - Remove a11y focus from UrlbarView on backspace.

Revision D29664 was moved to bug 1548860. Setting attachment 9062190 [details] to obsolete.

Comment 38

[James Teh [:Jamie]]

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

Curious. Testing locally, pressing backspace while a suggestion has a11y focus does seem to push a11y focus back to the input,

Not sure what I was doing differently (maybe I accidentally disabled Quantumbar or something? :( ), but this now isn't working for me. If I press down arrow then backspace, accessible focus stays in the autocomplete. That's why the test is failing. That's also a nasty regression as far as screen reader users are concerned, which is why we test it.

This little dirty patch to the test demonstrates the problem:

diff --git a/accessible/tests/browser/events/browser_test_focus_urlbar.js b/accessible/tests/browser/events/browser_test_focus_urlbar.js
index f1197e2c4df1..c63434a6f627 100644
--- a/accessible/tests/browser/events/browser_test_focus_urlbar.js
+++ b/accessible/tests/browser/events/browser_test_focus_urlbar.js
@@ -148,7 +148,11 @@ async function runTests() {
 
   info("Ensuring text box focus on backspace");
   focused = waitForEvent(EVENT_FOCUS, textBox);
+  info("jtd sending backspace");
   EventUtils.synthesizeKey("KEY_Backspace");
+  await new Promise(resolve => setTimeout(resolve, 2000));
+  info("jtd aria-activedescendant is " + gURLBar.inputField.getAttribute("aria-activedescendant"));
+  info("jtd waiting for focus on input");
   await focused;
   testStates(textBox, STATE_FOCUSED);
 

After sending backspace, we wait 2 seconds to avoid timing issues. Then, we check aria-activedescendant. Here's what I get:

 0:10.99 INFO Ensuring text box focus on backspace
 0:10.99 INFO jtd sending backspace
 0:12.99 INFO jtd aria-activedescendant is urlbarView-row-0
 0:12.99 INFO jtd waiting for focus on input

aria-activedescendant should be empty at this point, but it isn't. My guess is that it's being cleared, but then something causes it to be set again.

Comment 39

[Dão Gottwald [:dao]]

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

After sending backspace, we wait 2 seconds to avoid timing issues. Then, we check aria-activedescendant. Here's what I get:
[...]
aria-activedescendant should be empty at this point, but it isn't. My guess is that it's being cleared, but then something causes it to be set again.

That's a good guess. Also not sure why I didn't see this when tracing calls to _setAccessibleFocus. Since bug 1548860 we rely on UrlbarController::_userSelectionBehavior being up to date which it doesn't seem to be here.

Comment 40

[Dão Gottwald [:dao]]

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

Comment 41

[Dão Gottwald [:dao]]

Attachment: Bug 1522440 - Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd51101014c0
Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw

Comment 43

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/bd51101014c0