Closed Bug 1522440 Opened 8 months ago Closed 4 months ago

Port browser_test_focus_urlbar.js to the new address bar implementation

Categories

(Firefox :: Disability Access, task, P2)

task
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 68
Iteration:
68.3 - Apr 15 - 28
Tracking Status
firefox68 --- fixed

People

(Reporter: standard8, Assigned: dao)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxsearch])

Attachments

(4 files, 1 obsolete file)

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.

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.

Flags: needinfo?(jteh)

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.

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.

Depends on: 1522489

(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.

Flags: needinfo?(jteh)

(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.

Depends on: 1524564
Depends on: 1534661

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.

No longer blocks: quantumbar-release

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).

(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.

Summary: Port accessibility test to the new address bar implementation or skip the test for QuantumBar → Port accessibility test to the new address bar implementation
Depends on: 1536753
Depends on: 1537716
Summary: Port accessibility test to the new address bar implementation → Port browser_test_focus_urlbar.js to the new address bar implementation
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Points: --- → 2
Type: enhancement → task
Depends on: 1542099

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?

Flags: needinfo?(surkov.alexander)

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.

Flags: needinfo?(surkov.alexander)

(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

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh)
Depends on: 1543561

(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.

(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.

Iteration: --- → 68.3 - Apr 15 - 28

(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.

(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.

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

(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.

(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.

Depends on: 1545731

(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.

Attached image 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.

Attached image quantumbar.png

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.

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.

No longer depends on: 1545731
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db10a6b3d728
Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

<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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

  1. 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?

Flags: needinfo?(jteh)
Blocks: 1548031

(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.

Flags: needinfo?(jteh)

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 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.

Attachment #9062190 - Flags: feedback?(jteh)

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

Attachment #9061220 - Flags: checkin+

(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.

Flags: needinfo?(jteh)
Depends on: 1548860

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.

Attachment #9062190 - Attachment is obsolete: true
Attachment #9062190 - Flags: feedback?(jteh)

(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.

Flags: needinfo?(jteh)
Attachment #9061220 - Attachment description: Bug 1522440 - Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw → Bug 1522440 - Explicitly close the view since KEY_ArrowLeft won't automatically do that in the quantumbar. r=adw

(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.

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd51101014c0
Finish porting browser_test_focus_urlbar.js to quantumbar. r=adw
Status: REOPENED → RESOLVED
Closed: 5 months ago4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.