1570848 - Extension popups sometimes inaccessible on Windows

Status: VERIFIED FIXED

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

Description

[James Teh [:Jamie]]

Spun off bug 1523034 comment 20.

Intermittent STR (with the NVDA screen reader):

1. Install the Tampermonkey extension.
2. Navigate to the Tampermonkey button on the toolbar.
3. Press space.
   • Expected: NVDA should read the content of the extension popup.
   • Actual: NVDA just says the title of the window.

Note that this is intermittent. If it doesn't fail, press escape, then repeat step 3. If you keep doing this (speed is irrelevant), it will probably fail eventually.

I'm filing a new bug for this because the manifestation on Windows is specific to Windows code. I'm not sure why this is failing on Linux. We'll leave that to bug 1523034.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1570848: When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any).

Normally, the OuterDocAccessible is created first and the DocAccessibleParent for a remote document is created after that.
So, we get the OuterDocAccessible and call DocAccessibleParent::SendParentCOMProxy when the DocAccessibleParent is constructed (BrowserParent::RecvPDocAccessibleConstructor).
However, sometimes, the OuterDocAccessible is created after the DocAccessibleParent.
This sometimes happens for extension popups, for example.
In that case, we previously never sent the parent COM proxy.
Aside from leaving the remote document with a null parent, this also meant we never sent any events for the document, since events are buffered for remote documents until the parent COM proxy is received.
This effectively left the remote document (e.g. extension popup) inaccessible.

Now, we also call SendParentCOMProxy in the OuterDocAccessible constructor.
Note that this doesn't result in duplicates because if the OuterDocAccessible was created first, there won't be a DocAccessibleParent for the remote document yet, so this code won't run.

That said, if the OuterDocAccessible is recreated (e.g. due to frame reconstruction), we may call SendParentCOMProxy again.
This should be okay, but it required an assertion in DocAccessibleChild::RecvParentCOMProxy to be tweaked.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f980c4db1343
When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any). r=MarcoZ

Comment 3

[Alexandru Michis [:malexandru]]

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

Comment 4

[Marco Zehe (:MarcoZ)]

This is now fixed in Nightly. This was indeed the cause.

Jamie, should we uplift this to 69 beta, and 68ESR, given that the new keyboard nav is fairly new and a lot of users might run into this issue?

Comment 5

[James Teh [:Jamie]]

Comment on attachment 9082515 [details]
Bug 1570848: When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any).

Beta/Release Uplift Approval Request

• User impact if declined: Screen reader users (and potentially users of other accessibility tools) will intermittently be unable to access extension popups.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple patch only impacting accessibility. At worst, this might result in a message being sent to a content process twice, and I've verified that doing this is harmless.
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Prevents screen reader users from using extensions.
• User impact if declined: Screen reader users (and potentially users of other accessibility tools) will intermittently be unable to access extension popups.
• Fix Landed on Version: 70, also requesting uplift to 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple patch only impacting accessibility. At worst, this might result in a message being sent to a content process twice, and I've verified that doing this is harmless.
• String or UUID changes made by this patch: None.

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9082515 [details]
Bug 1570848: When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any).

a11y only patch, fix on nightly verified by the developer and the reviewer, looks low risk to me, approved for 69 beta 11, thanks.

Comment 7

[Oana Pop-Rus]

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

Comment 8

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0
Build ID: 20190805120428

Verified as fixed on Windows 10 using the latest Firefox 69 beta 11 and the latest Nightly 70.0a1.

Comment 9

[Simona Badau, Desktop QA]

Pascal, can you please tell me if this fix will be uplifted on the esr68 branch? Thanks!

Comment 10

[Pascal Chevrel:pascalc]

(In reply to Simona Badau from comment #9)

Pascal, can you please tell me if this fix will be uplifted on the esr68 branch? Thanks!

This is likely to be uplifted to ESR if there is no problem on beta.

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9082515 [details]
Bug 1570848: When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any).

windows a11y fix for 68.1esr

Comment 12

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

Please provide a patch which applies without conflicts on ESR68.

Comment 13

[James Teh [:Jamie]]

Sorry. I neglected to realise this depends on some refactor I did for Fission in bug 1543313. I've prepared a patch for ESR with just the parts necessary for this fix, minus the Fission specific bits. I guess this might need to be re-assessed for ESR approval?

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf92f177cb186737b4fe1d006be55811616cf6fe

Comment 14

[James Teh [:Jamie]]

Attachment: Bug 1570848: When an OuterDocAccessible is constructed, send it as the parent COM proxy for its remote document (if any). ESR68 patch.

Normally, the OuterDocAccessible is created first and the DocAccessibleParent for a remote document is created after that.
So, we get the OuterDocAccessible and call DocAccessibleParent::SendParentCOMProxy when the DocAccessibleParent is constructed (BrowserParent::RecvPDocAccessibleConstructor).
However, sometimes, the OuterDocAccessible is created after the DocAccessibleParent.
This sometimes happens for extension popups, for example.
Previously, we called SendParentCOMProxy in the OuterDocAccessible constructor to deal with this case.
Unfortunately, this was a no-op because SendParentCOMProxy called DocAccessible::GetAccessible for the frame element, which would have returned null because the accessible isn't bound to the document until after it is constructed.
That meant we never actually sent the COM proxy for this case.
Aside from leaving the remote document with a null parent, this also meant we never sent any events for the document, since events are buffered for remote documents until the parent COM proxy is received.
This effectively left the remote document (e.g. extension popup) inaccessible.

Now, SendParentCOMProxy takes an OuterDocAccessible and the OuterDocAccessible constructor passes itself to SendParentCOMProxy.
Note that this doesn't result in duplicates because if the OuterDocAccessible was created first, there won't be a DocAccessibleParent for the remote document yet, so this code won't run.

That said, if the OuterDocAccessible is recreated (e.g. due to frame reconstruction), we may call SendParentCOMProxy again.
This should be okay, but it required an assertion in DocAccessibleChild::RecvParentCOMProxy to be tweaked.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/67fba999b1f2

Comment 16

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Firefox/68.0
Build ID: 20190812183651

Verified as fixed also on Firefox 68.1.0esr - tested on Windows 10 x64-bit with NVDA version 2019.1.1