Closed Bug 1249253 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 + fixed
firefox47 + fixed
firefox48 + verified
firefox-esr45 --- affected
relnote-firefox --- 46+

People

(Reporter: MarcoZ, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(5 files)

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.
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.
Flags: needinfo?(surkov.alexander)
It'll be helpful to have a reduced test case
Flags: needinfo?(surkov.alexander)
(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?
Flags: needinfo?(slauriat)
This was filed a few weeks ago against NVDA: https://github.com/nvaccess/nvda/issues/5743
Attached file 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.
I'm not sure I caught whether the problem is in focus or in wrong accessible tree?
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.
Ok, I'm curious to see if bug 1248838 helps, when landed.
I don't think so. Issue still exists in the 2016-02-21 build. Unless I've got the timing wrong...
(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.
(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.
Alex, any idea on this? This is still a problem in the latest 48.0a1 nightly build.
Flags: needinfo?(slauriat) → needinfo?(surkov.alexander)
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.
Attached patch patchSplinter Review
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.
Flags: needinfo?(surkov.alexander)
Attachment #8729071 - Flags: review?(yzenevich)
Comment on attachment 8729071 [details] [diff] [review]
patch

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

looks good
Attachment #8729071 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/edfe87fceb4d51e41f16d0fdf66b84d8a85a6c5a
Bug 1249253 - content removal processing can wrongly remove ARIA owned children, r=yzen
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.
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/edfe87fceb4d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Flags: needinfo?(surkov.alexander)
[Tracking Requested - why for this release]:
High-impact regression in the usability of Google Docs for all screen reader users using Firefox on Windows.
Attached patch aurora patchSplinter Review
Flags: needinfo?(surkov.alexander)
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.
Attachment #8730317 - Flags: approval-mozilla-aurora?
(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.
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.
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
Tracking for 46+.
Marco, would you be able to verify the fix on the latest Nightly build? Thanks!
Flags: needinfo?(mzehe)
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+
Attachment #8730317 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(mzehe)
(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.
Status: RESOLVED → VERIFIED
Alexander, do you still intend to create a patch for beta?
Flags: needinfo?(surkov.alexander)
Attached patch beta patchSplinter Review
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
Flags: needinfo?(surkov.alexander)
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?
Flags: needinfo?(mzehe)
The patch or Beta in general doesn't build for me currently. Alex, is the patch complete?
Flags: needinfo?(mzehe) → needinfo?(surkov.alexander)
(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/surkov.alexander@gmail.com-55a9cfc828b886dfdcae868630a65fb6d59f4326/
Flags: needinfo?(surkov.alexander)
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.
Looks promising, can you add the mochitest and request uplift to beta? Thanks!
(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!
Flags: needinfo?(surkov.alexander)
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
Flags: needinfo?(surkov.alexander)
Attachment #8737301 - Flags: approval-mozilla-beta?
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.
Attachment #8737301 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]: Screen reader interaction with Google Docs corrected
Assignee: nobody → surkov.alexander
You need to log in before you can comment on or make changes to this bug.