1249253 - After bug 1133213, in Google Docs, screen readers started to say "blank" before characters, lines, and when navigating

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

1. With NVDA and Firefox in Windows, go to https://drive.google.com and log in.
2. Create a new Docs document.
3. Make sure the Braille support for screen readers is turned *off*. This is normally the default, but better to check.
4. Type your name.
5. Arrow over the characters you just typed.

Result: NVDA will only announce the letters.

6. Select your last name with Shift+Left and Right Arrows.
7. Type your last name again, overwriting the selection.
8. Arrow over the typed characters again.

Expected: Should only speak characters as before.
Actual: Says "blank" before each character.

Note that you have to type over the selection. Just selecting and deselecting is not enough to trigger the bug.

Bisection traced this down to the checkins around bug 1133213, our aria-owns support:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d90bfbc8688ad3d6d846b55e594cd423c8dd16fc&tochange=a252f3d0a8c970231c8db43638d0f68e82b66ac3

This was reported to me by Roger from the Google Docs team, and I've confirmed it in both Firefox 44 release and latest nightly builds.

Comment 1

[Marco Zehe (:MarcoZ)]

Alex, since you're the author of the aria-owns support, the ball is in your court now for this regression. This affects all Google Docs users with screen readers, and it affects primarily those who don't use the braille support. The majority won't use the braille support, leaving the talking to Docs and its output for the most part. So the impact is rather big.

Comment 2

[alexander :surkov (:asurkov)]

It'll be helpful to have a reduced test case

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #2)
> It'll be helpful to have a reduced test case

Shawn, do you have/can you provide that?

Comment 4

[James Teh [:Jamie]]

This was filed a few weeks ago against NVDA: https://github.com/nvaccess/nvda/issues/5743

Comment 5

[James Teh [:Jamie]]

Attachment: Minimal test case.

Here's what I understand about what's going on here.

Google Docs puts DOM focus on a contentEditable to handle text entry, selection, etc. However, most of the time (when braille mode is disabled), that contentEditable doesn't contain any real text. To stop screen readers from trying to handle the focus as editable text (and thus saying "blank" when pressing the arrow keys, etc.), Docs "fakes" focus on a "group" accessible. It does this by using aria-owns and aria-activedescendant on a div with role="group". This had the desired effect before Firefox implemented aria-owns... and it still does, until you change the selection twice. Once you do that, the "group" (owned) accessible simply disappears from the tree. The DOM node still exists, but the accessible is just gone. If you remove aria-owns *before* this disappearance, this doesn't occur.

When you select in Docs, it replaces the content of the contentEditable with a span and selects it. Replacing the content of the contentEditable twice seems to break the owned tree.

If you open the attached test case, you'll see this occur 2 seconds after it loads. The accessible for id="fake" disappears from the tree.

Comment 6

[alexander :surkov (:asurkov)]

I'm not sure I caught whether the problem is in focus or in wrong accessible tree?

Comment 7

[James Teh [:Jamie]]

Both. My guess is the node disappears, Gecko realises it can't be an active descendant and thus throws focus back to the real focus.

Comment 8

[alexander :surkov (:asurkov)]

Ok, I'm curious to see if bug 1248838 helps, when landed.

Comment 9

[James Teh [:Jamie]]

I don't think so. Issue still exists in the 2016-02-21 build. Unless I've got the timing wrong...

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #8)
> Ok, I'm curious to see if bug 1248838 helps, when landed.

Unfortunately it does not. I can still reproduce the bug in Docs with the 2016-02-21 build, which contains the fix for bug 1248838.

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #9)
> I don't think so. Issue still exists in the 2016-02-21 build. Unless I've
> got the timing wrong...

No, you did not get the timing wrong, the 2016-02-21 build does contain the fix for bug 1248838.

Comment 12

[Marco Zehe (:MarcoZ)]

Alex, any idea on this? This is still a problem in the latest 48.0a1 nightly build.

Comment 13

[Marco Zehe (:MarcoZ)]

Oh and with the bug now being in the second release (Firefox 45), people who need to use Docs productively, are starting to notice it more and more. Reports are increasing. So it'd be good to have a fix we can at least uplift to 46.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

basically it's backing out the patch of bug 852150, maybe something how we update the tree or notifications has been changed, but mochitest passes. Also, if we were able to find a container for a removed element by searching its accessible child by means of TreeWalker in nsAccessibilityService::ContentRemoved(), then we should be able to find all accessible children in it in UpdateTreeOnRemoval, since it's basically same operation.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8729071 [details] [diff] [review]
patch

Review of attachment 8729071 [details] [diff] [review]:
-----------------------------------------------------------------

looks good

Comment 16

[alexander :surkov (:asurkov)]

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

Comment 17

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edfe87fceb4d51e41f16d0fdf66b84d8a85a6c5a
Bug 1249253 - content removal processing can wrongly remove ARIA owned children, r=yzen

Comment 18

[Marco Zehe (:MarcoZ)]

Thanks for getting on this! Can this be backported to 47 (Aurora) and 46 (Beta) easily? I mean with all the other changes that recently went on in TreeWalker etc.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #18)
> Thanks for getting on this! Can this be backported to 47 (Aurora) and 46
> (Beta) easily? I mean with all the other changes that recently went on in
> TreeWalker etc.

not sure, it has to be same approach, but it it will be likely a different patch, that should be tested separately.

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Marco Zehe (:MarcoZ)]

Fix confirmed in 48 nightly builds. Alex, can you create a patch for 47 and 46? I am not sure we'll get this into 45.0.x as a ride-along or 45 ESR, but it would be good to quickly have this uplifted to 47 and 46 at least. This is affecting all Google Docs users who use a screen reader.

Comment 22

[Marco Zehe (:MarcoZ)]

[Tracking Requested - why for this release]:
High-impact regression in the usability of Google Docs for all screen reader users using Firefox on Windows.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: aurora patch

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 8730317 [details] [diff] [review]
aurora patch

Approval Request Comment
[Feature/regressing bug #]: bug 1133213
[User impact if declined]: After manipulating the first selection in Google Docs, screen reader users will hear the word "blank" prepended to every utterance the screen reader speaks in Docs.
[Describe test coverage new/current, TreeHerder]: Has mochitest, landed on Central, tested Aurora-specific patch in a local build and confirmed fix.
[Risks and why]: Low risk, partial backout, see comment #20.
[String/UUID change made/needed]: None.

Comment 25

[Marco Zehe (:MarcoZ)]

(In reply to Marco Zehe (:MarcoZ) from comment #24)
> Comment on attachment 8730317 [details] [diff] [review]
> aurora patch
> 
> Approval Request Comment
...
> [Risks and why]: Low risk, partial backout, see comment #20.

Sorry, make that comment #14.

Comment 26

[Marco Zehe (:MarcoZ)]

Alex, are you planning on creating a patch for Beta, too? The Aurora patch doesn't apply to Beta due to a lot of changes in the tree walker code.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #26)
> Alex, are you planning on creating a patch for Beta, too? The Aurora patch
> doesn't apply to Beta due to a lot of changes in the tree walker code.

I can try of course. I wouldn't say it's a low risk though, the builds needs some good testing.

Comment 28

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

Tracking for 46+.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Marco, would you be able to verify the fix on the latest Nightly build? Thanks!

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730317 [details] [diff] [review]
aurora patch

Based on the comments in the bug this seems to be severely impacting affected gdoc users in Firefox. Aurora47+

Comment 31

[Marco Zehe (:MarcoZ)]

(In reply to Ritu Kothari (:ritu) from comment #29)
> Marco, would you be able to verify the fix on the latest Nightly build?

It's already verified fixed in Nightly 48, see comment #21. Once this lands on Aurora, I will verify there, too. Already verified that the patch works by building Aurora with the patch locally, so I expect it to work in a regular Aurora build, too. Furthermore, it has a test that will run as soon as it lands.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2fd77fb72e36

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Marco Zehe (:MarcoZ) from comment #31)
> (In reply to Ritu Kothari (:ritu) from comment #29)
> > Marco, would you be able to verify the fix on the latest Nightly build?
> 
> It's already verified fixed in Nightly 48, see comment #21. Once this lands
> on Aurora, I will verify there, too. Already verified that the patch works
> by building Aurora with the patch locally, so I expect it to work in a
> regular Aurora build, too. Furthermore, it has a test that will run as soon
> as it lands.

Great! Thank you for the due diligence.

Comment 34

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

Alexander, do you still intend to create a patch for beta?

Comment 35

[alexander :surkov (:asurkov)]

Attachment: beta patch

here's a beta patch, tests pass, no data if it regresses anything, and if it does then it's worse than this bug. not sure who's in charge to make a decision about backporting it, and what else should be done to help with this decision

Comment 36

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

Marco, is this something you could verify on a beta build with Alexander's patch in comment 35?  Or, should we land it for beta 7 and test there?

Comment 37

[Marco Zehe (:MarcoZ)]

The patch or Beta in general doesn't build for me currently. Alex, is the patch complete?

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #37)
> The patch or Beta in general doesn't build for me currently. Alex, is the
> patch complete?

it should be, a try build was completed ok:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=55a9cfc828b8
 https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-55a9cfc828b886dfdcae868630a65fb6d59f4326/

Comment 39

[Marco Zehe (:MarcoZ)]

OK, the try server build fixes the bug as well. Alex, if it is possible, I would like you to add the mochitest so we can make sure the testcase is covered on beta and release as well.

Comment 40

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

Looks promising, can you add the mochitest and request uplift to beta? Thanks!

Comment 41

[Marco Zehe (:MarcoZ)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Looks promising, can you add the mochitest and request uplift to beta?
Alex, can you do that, please? Including the asking for approval? Thanks!

Comment 42

[alexander :surkov (:asurkov)]

Attachment: beta patch with mochitest

Approval Request Comment
[Feature/regressing bug #]: bug 1133213
[User impact if declined]: screen readers are unable to work with GoogleDocs
[Describe test coverage new/current, TreeHerder]: mochitests + manual testing, running on nightly + aurora
[Risks and why]: moderate, possible regressions are not necessary worse than  GoogleDocs issue.
[String/UUID change made/needed]:no

Comment 43

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

Comment on attachment 8737301 [details] [diff] [review]
beta patch with mochitest

Fix for screen reader google docs issue. Please uplift to beta. This should make it into either beta 8 or beta 9.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c83ddaf42140

Comment 45

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

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]: Screen reader interaction with Google Docs corrected