Closed Bug 1448091 Opened 7 years ago Closed 7 years ago

[e10s a11y] Node removals aren't reflected in JAWS virtual buffer

Categories

(Core :: Disability Access APIs, defect)

All
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

Details

Attachments

(3 files)

Attached file Test case.
STR (with JAWS): 1. Open the attached test case. 2. Press the Go button. 3. Read through the text of the document using the arrow keys. Expected: The text "This paragraph will be removed." should not be present. Actual: It is. If you disable the COM handler (by setting the pref accessibility.handler.enabled to false and restarting), this does not occur. So, this is somehow related to AccessibleHandler. However, this does not occur in either case with NVDA.
Comment on attachment 8961841 [details] Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. https://reviewboard.mozilla.org/r/230666/#review236146 ::: accessible/ipc/win/handler/AccessibleHandler.cpp:223 (Diff revision 1) > - return mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData); > + HRESULT hr = mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData); > + if (SUCCEEDED(hr)) { > + // We just updated the cache, so update this object's cache generation > + // so we only update the cache again after the next change. > + mCacheGen = gen; > + } if you failed to update the cache, then do you still need to keep old version or should you instead to nullify it?
Some notes on these patches: Part 1 is the fix I expected to work for this bug. Curiously, it does fix this test case, but it sometimes doesn't work for the same case in larger documents. I don't really understand why. Part 2 seems to fix this even in larger documents. Again, I don't really understand why. I suspect there is some kind of timing issue here, but I can't work out exactly what it is. Part 2 also fixes extreme slowness when refreshing documents with JAWSKey+escape. Even though there are some unknowns here, both of these patches correct behaviour which is very clearly incorrect. I'm inclined to accept that "correctness" deals with this problem rather than spending a huge amount of time on figuring out the exact interactions in a complex system.
Comment on attachment 8961840 [details] Bug 1448091 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails. https://reviewboard.mozilla.org/r/230660/#review236154
Attachment #8961840 - Flags: review?(mzehe) → review+
Comment on attachment 8961841 [details] Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. https://reviewboard.mozilla.org/r/230666/#review236156
Attachment #8961841 - Flags: review?(mzehe) → review+
Pushed by mzehe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/702ffbc7ccd5 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails. r=MarcoZ https://hg.mozilla.org/integration/autoland/rev/a61e917f9406 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. r=MarcoZ
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Verified fixed in Firefox 61.0a1 (20180324100127).
Comment on attachment 8961840 [details] Bug 1448091 part 1: Accessible HandlerProvider: Return an error if refreshing the cache fails. Approval Request Comment [Feature/Bug causing the regression]: Bug 1303060/e10s Windows a11y. [User impact if declined]: Incorrect content presented to screen reader users when nodes are removed from the document. [Is this code covered by automated tests?]: No; no mechanism to test platform a11y. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: Other patch from this bug. [Is the change risky?]: No. [Why is the change risky/not risky?]: Straightforward correctness patch. [String changes made/needed]: None.
Attachment #8961840 - Flags: approval-mozilla-beta?
(In reply to James Teh [:Jamie] from comment #10) > [Needs manual test from QE? If yes, steps to reproduce]: Sorry. The answer to this is "no".
Comment on attachment 8961841 [details] Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. Approval Request Comment [Feature/Bug causing the regression]: Bug 1303060/e10s Windows a11y. [User impact if declined]: Incorrect content presented to screen reader users when nodes are removed from the document. [Is this code covered by automated tests?]: No; no mechanism to test platform a11y. [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]: Other patch from this bug. [Is the change risky?]: No. [Why is the change risky/not risky?]: Straightforward correctness patch. [String changes made/needed]: None.
Attachment #8961841 - Flags: approval-mozilla-beta?
Comment on attachment 8961841 [details] Bug 1448091 part 2: AccessibleHandler: Don't repeatedly and unnecessarily refresh the cache after the first change. a11y fix for beta60. should be in 60.0b8.
Attachment #8961841 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8961840 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → jteh
Verified fixed in Firefox 60.0b8.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: