1474814 - [e10s a11y] Focused state not updated when focus moves

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[James Teh [:Jamie]]

Reported by VFO and causes problems for JAWS.

TL;DR: When focus moves but no other update occurs, the handler cache doesn't get invalidated.

STR (with NVDA):
We use the NVDA python console here to query the buffer. NVDA itself is out-of-process, so doesn't use the handler cache, but the buffer is in-process and does use it. It just so happens that NVDA doesn't care about the focused state reported in the buffer.
1. Open this URl:
data:text/html,<input type="checkbox" id="c"><input type="radio" id="r">
2. Press control+home to move to the check box.
3. Open the NVDA Python console with NVDA+control+z.
4. Paste the following (as a single line) and press enter:
import controlTypes; review.expand("character"); f = review.getTextWithFields(); f[1].field["IAccessible2::attribute_id"], controlTypes.STATE_FOCUSED in f[1].field["states"]
Result: (u'c', False)
5. Alt+tab back to Firefox without closing the console (so the captured review position will still be on the check box).
6. Press space to check the check box.
7. Alt+tab back to the console.
8. Execute the same command again  (press up arrow, enter).
Expected: (u'c', False)
Actual: (u'c', True)
Observation: The state changed, invalidating the cache. However, since we've alt+tabbed out, this doesn't actually have the focus any more, so the focused state shouldn't be present; we should see False, not True.
That alone might be forgivable; someone's not likely to query the focused state when focused in a completely different window. However:
9. Alt+tab back to Firefox without closing the console (so the captured review position will still be on the check box).
10. Press r to move to the radio button.
11. Alt+tab back to the console.
12. Execute the same command again  (press up arrow, enter).
Expected: (u'c', False)
Actual: (u'c', True)
Observation: If anything, the radio button might report focused. The check box should definitely not report focused at this point, but it does.

We don't fire a state change for the focused state, since that's somewhat redundant given we fire a focus event. However, the cache doesn't get invalidated for focus events. :(

Marking as a regression since this worked just fine before e10s.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1474814: Invalidate the AccessibleHandler cache on focus events.

We don't fire a state change for the focused state, since that would be redundant given that we fire a focus event.
However, that also means the handler cache previously didn't get invalidated when there was a focus change without any other update.
That resulted in stale focused states, which could mean an object was exposed as focused when it wasn't.
Invalidating the cache for focus events ensures the focused state is always up to date.

Review commit: https://reviewboard.mozilla.org/r/256184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256184/

Comment 2

[Marco Zehe (:MarcoZ)]

Flagging 62 and 60ESR as affected since this bug is present in both. Does also affect 61, but I don't think this warrants a respin of an 61 dot release. But it is a candidate for a 62 beta uplift, and maybe a 60ESR backport.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 8991223 [details]
Bug 1474814: Invalidate the AccessibleHandler cache on focus events.

https://reviewboard.mozilla.org/r/256184/#review263032

Straight-forward fix. :)

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #0)
> STR (with NVDA):
> We use the NVDA python console here to query the buffer. NVDA itself is
> out-of-process, so doesn't use the handler cache, but the buffer is
> in-process and does use it. It just so happens that NVDA doesn't care about
> the focused state reported in the buffer.
> 1. Open this URl:
> data:text/html,<input type="checkbox" id="c"><input type="radio" id="r">
> 2. Press control+home to move to the check box.
> 3. Open the NVDA Python console with NVDA+control+z.
> 4. Paste the following (as a single line) and press enter:
> import controlTypes; review.expand("character"); f =
> review.getTextWithFields(); f[1].field["IAccessible2::attribute_id"],
> controlTypes.STATE_FOCUSED in f[1].field["states"]
> Result: (u'c', False)

So far, so good.

> 5. Alt+tab back to Firefox without closing the console (so the captured
> review position will still be on the check box).
> 6. Press space to check the check box.
> 7. Alt+tab back to the console.
> 8. Execute the same command again  (press up arrow, enter).
> Expected: (u'c', False)
> Actual: (u'c', True)
> Observation: The state changed, invalidating the cache. However, since we've
> alt+tabbed out, this doesn't actually have the focus any more, so the
> focused state shouldn't be present; we should see False, not True.

This still yields (u'c', True) for me.

> That alone might be forgivable; someone's not likely to query the focused
> state when focused in a completely different window. However:
> 9. Alt+tab back to Firefox without closing the console (so the captured
> review position will still be on the check box).
> 10. Press r to move to the radio button.
> 11. Alt+tab back to the console.
> 12. Execute the same command again  (press up arrow, enter).
> Expected: (u'c', False)
> Actual: (u'c', True)
> Observation: If anything, the radio button might report focused. The check
> box should definitely not report focused at this point, but it does.

This, also, yields (u'c', True) for me. :( I definitely installed the try build, removing any previous Nightly, but it appears the patch doesn't actually cause the cache to be invalidated. Or something else is amiss. I am going to try Brett's example with JAWS next.

Comment 5

[Marco Zehe (:MarcoZ)]

OK, all good with JAWS. The patch does fix the bug, it's just that the NVDA steps aren't reflecting that. This patch can land.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbf89941ccff
Invalidate the AccessibleHandler cache on focus events. r=MarcoZ

Comment 7

[James Teh [:Jamie]]

(In reply to Marco Zehe (:MarcoZ) from comment #5)
> OK, all good with JAWS. The patch does fix the bug, it's just that the NVDA
> steps aren't reflecting that. This patch can land.

That'd be because my steps were bogus. I wanted a more programmatic (and public) way to reproduce this. Unfortunately, I neglected to realise that NVDA doesn't update a node in its buffer just because the focus changes (because it doesn't need to), and even if it did, it fetches a fresh copy of the accessible when it updates, thus avoiding the stale cache. Sorry about that. That's what I get for trying to be clever. :(

Unfortunately, I can't publish the test case from VFO here. The best we can do is verify that the patch fixes the reported issue from VFO, which Marco did in comment 5.

Comment 8

[Raul Gurzau (:RaulG)]

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

Comment 9

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 63.0a1 (20180712220034). Please request uplift. :)

Comment 10

[James Teh [:Jamie]]

Comment on attachment 8991223 [details]
Bug 1474814: Invalidate the AccessibleHandler cache on focus events.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Users of the JAWS screen reader will receive no feedback when navigating to form controls in some cases.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for Windows platform a11y testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: One line patch which invalidates a cache based on an additional event.
[String changes made/needed]: None.

ESR:
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is extremely confusing for users of the JAWS screen reader and is a regression compared to ESR 52.
Fix Landed on Version: 63, but also requesting uplift to 62.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8991223 [details]
Bug 1474814: Invalidate the AccessibleHandler cache on focus events.

Fix for a regression in form controls. Verified in Nightly. Let's uplift for beta 9.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/825a33b82f47

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8991223 [details]
Bug 1474814: Invalidate the AccessibleHandler cache on focus events.

Improves life for JAWS users moving from ESR52 to ESR60. Approved for ESR 60.2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f2e139fd72f6

Comment 15

[roxleitan]

I tested on Firefox Beta 62.0b8 and could not navigate through checkboxes using "x" key with JAWS.
Spoked with Marco and he will help with the verification on the latest Beta and ESR60.

Comment 16

[Marco Zehe (:MarcoZ)]

Verified fixed on 62.0b9. An ESR 60.2.0 is not yet built.

Comment 17

[Marco Zehe (:MarcoZ)]

Verified fixed in Firefox 60.2.2 ESR.

Comment 18

[Brindusa Tot, Desktop QA]

The bug was verified on all fixed FF versions. Removing the qe-verify+ flag.