Closed Bug 1570848 Opened 1 year ago Closed 1 year ago

Extension popups sometimes inaccessible on Windows

Categories

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

Desktop
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr68 69+ verified
firefox69 --- verified
firefox70 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

Details

Attachments

(2 files)

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.

Summary: Extension popups sometimes inaccessible → Extension popups sometimes inaccessible on Windows

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.

Pushed by mzehe@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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?

Status: RESOLVED → VERIFIED
Flags: needinfo?(jteh)

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.
Flags: needinfo?(jteh)
Attachment #9082515 - Flags: approval-mozilla-esr68?
Attachment #9082515 - Flags: approval-mozilla-beta?

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.

Attachment #9082515 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

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

Flags: needinfo?(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.

Flags: needinfo?(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).

windows a11y fix for 68.1esr

Attachment #9082515 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Please provide a patch which applies without conflicts on ESR68.

Flags: needinfo?(jteh)

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

Flags: needinfo?(jteh)

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.

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

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.