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)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: Jamie, Assigned: Jamie)
Details
Attachments
(3 files)
401 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
MarcoZ
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
MarcoZ
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-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/#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?
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
mozreview-review |
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 6•7 years ago
|
||
mozreview-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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/702ffbc7ccd5
https://hg.mozilla.org/mozilla-central/rev/a61e917f9406
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 9•7 years ago
|
||
Verified fixed in Firefox 61.0a1 (20180324100127).
status-firefox60:
--- → affected
Assignee | ||
Comment 10•7 years ago
|
||
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?
Assignee | ||
Comment 11•7 years ago
|
||
(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".
Assignee | ||
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8961840 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Assignee: nobody → jteh
Comment 14•7 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•