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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: Jamie, Assigned: Jamie)

Tracking

unspecified
mozilla61
All
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox60 verified, firefox61 verified)

Details

Attachments

(3 attachments)

Posted 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
https://hg.mozilla.org/mozilla-central/rev/702ffbc7ccd5
https://hg.mozilla.org/mozilla-central/rev/a61e917f9406
Status: NEW → RESOLVED
Closed: Last year
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.