Closed Bug 1322593 Opened 6 years ago Closed 6 years ago

Text inserted/removed events intermittently missing when multiple elements updated at once

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: Jamie, Assigned: tbsaunde)

References

Details

(Keywords: regression)

Attachments

(7 files)

Test case.
344 bytes, text/html
Details
avoid merging text change events for unrelated accessibles
2.12 KB, patch
davidb
: review+
Details | Diff | Splinter Review
don't coalesce non contiguous text changes under the same accessible
2.63 KB, patch
davidb
: review+
Details | Diff | Splinter Review
allow asynchronous checking of text change events
1.30 KB, patch
yzen
: review+
Details | Diff | Splinter Review
test that unrelated text change events are not coalesced
3.32 KB, patch
yzen
: review+
Details | Diff | Splinter Review
1322593 patch
1.45 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
add test for multiple independent text changes under the same accessible
3.48 KB, patch
yzen
: review+
gchang
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file Test case. — 
STR:
1. Open the attached test case.
2. Press the button.
Expected: both textRemoved and textInserted events for both live and nonlive div accessibles
Actual: Some of these events are missing.

The specific events sent change intermittently, but are consistent for the life of the document. That is, if you keep pressing the button, the events sent remain the same, but if you refresh the page and press the button, the events sent might be different.

I see two cases here:

Case 1:
textRemoved on live
reorder on live
reorder on nonlive
textInserted on nonlive
This causes the live region to fail in NVDA because we never receive a textInserted on live.

Case 2:
textRemoved on live
reorder on nonlive
reorder on live
textInserted on live
The live region works in NVDA because we do get a textInserted on live. It's still odd that some expected events are missing, though.

Impact: Causes some live regions to intermittently fail for NVDA users.
Tested with: Firefox 53.0a1 (2016-12-08) (64-bit)
Occurs both with and without e10s.
I suspect this is a regression, but I'm not yet certain.
This bug was actually found while examining bug 1322532. This one needs to be fixed before we can determine a course forward (if any) on bug 1322532.
Blocks: 1322532
It's worth noting that while we can't *prove* bug 1322532 without a fix for this one, Trev and I both agree that bug 1322532 is definitely going to happen at some point. It's just a question of how often it breaks things practically speaking.
After some more chat on IRC, decided that this should not block the other one, but that work can happen in parallel.
No longer blocks: 1322532
Confirmed that this is a regression.

Last good revision: 791cc18c90be2f6ecb12de64a344feb86b34f191
First bad revision: 387d3acae9e99bdc140a65fd367ecbaa6238f3a3
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=791cc18c90be2f6ecb12de64a344feb86b34f191&tochange=387d3acae9e99bdc140a65fd367ecbaa6238f3a3
Possibly bug 1270916?
Keywords: regression
Blocks: 1270916
Blocks: 1325493
Trevor, do you have cycles to check this out, it seems to be a regression from bug 1270916.
Flags: needinfo?(tbsaunde+mozbugs)
its been in my queue for a while
Comment on attachment 8828520 [details] [diff] [review]
avoid merging text change events for unrelated accessibles

Review of attachment 8828520 [details] [diff] [review]:
-----------------------------------------------------------------

That seems reasonable.
Attachment #8828520 - Flags: review?(dbolter) → review+
This allows us to check for multiple text change events that could come in any
order.
Attachment #8828524 - Flags: review?(yzenevich)
Comment on attachment 8828523 [details] [diff] [review]
don't coalesce non contiguous text changes under the same accessible

Review of attachment 8828523 [details] [diff] [review]:
-----------------------------------------------------------------

Oh! Yeah this seems more correct.
Attachment #8828523 - Flags: review?(dbolter) → review+
I don't quite know what version bug 1270916 landed in. However, I'd strongly suggest that this should be uplifted to that version. It intermittently breaks messages in quite a lot of commonly used apps including Gmail, Google Docs and Google Sheets. In Sheets, it's very nasty because the core of the app is implemented using live region messages, so it is *completely unusable* with this bug.
Attachment #8828524 - Flags: review?(yzenevich) → review+
Attachment #8828525 - Flags: review?(yzenevich) → review+
Attached patch 1322593 patch — — Splinter Review 
Hi Trevor, this is a patch that adds an additional match function for a text change event checker. However one of the test assertions still fails (the last one for b being inserted). It looks like the start position is not right there:

Event type: text inserted, start: 7, length: 1, inserted text: b. Target: ['div@id="container" node', address: [object HTMLDivElement], role: section, address: 0x1087b9aa0]. Listeners count: 1
18 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_bug1322593.2.html | Wrong start offset for  'container'  - got 7, expected +0
Attachment #8828641 - Flags: review?(tbsaunde+mozbugs)
This bug also affects Aurora, and it also affects non-e10s builds, so needs uplift to 52 once it lands. Note that 52 will move to beta early next week.
Assignee: nobody → tbsaunde+mozbugs
Status: NEW → ASSIGNED
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → affected
status-firefox53: --- → affected
Comment on attachment 8828641 [details] [diff] [review]
1322593 patch

># HG changeset patch
># User Yura Zenevich <yzenevich@mozilla.com>
>
>Bug 1322593 - add match function for text change checker. r=tbsaunde

would be good to explain why your doing this.

>+  this.match = function stextChangeChecker_match(aEvent)
>+  {

I guess you could move most of the checking stuff here and leave bad events unmatched, that would allow multiple events with the same text at different locations, but we don't need that so whatever.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8828641 - Flags: review?(tbsaunde+mozbugs) → review+
It was just a bad test copy pasted from the other one.
Attachment #8828961 - Flags: review?(yzenevich)
Comment on attachment 8828961 [details] [diff] [review]
add test for multiple independent text changes under the same accessible

Review of attachment 8828961 [details] [diff] [review]:
-----------------------------------------------------------------

Tests pass, all good.
Attachment #8828961 - Flags: review?(yzenevich) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d86f65852e6
add match function for text change checker to stricten match based on modified text. r=tbsaunde
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a4da09461f
avoid merging text change events for unrelated accessibles r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/af4cbb24227d
don't coalesce non contiguous text changes under the same accessible r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc21ac8aad1
allow asynchronous checking of text change events r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f5660aecf2
test that unrelated text change events are not coalesced r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4091c903d5fa
add test for multiple independent text changes under the same accessible r=yzen
Trevor, can this one be closed? As far as I can see and also reports indicate, the bug has been fixed. It needs uplift to Aurora (53) and Beta (52) as well.
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8828961 [details] [diff] [review]
add test for multiple independent text changes under the same accessible

Approval Request Comment
[Feature/Bug causing the regression]:1270916
[User impact if declined]:google sheets is aparently pretty broken.
[Is this code covered by automated tests?]:yes
[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]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:very simple fixes
[String changes made/needed]:none

Note to be clear I want to uplift all the patches in this bug not just this one.
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8828961 - Flags: approval-mozilla-beta?
Attachment #8828961 - Flags: approval-mozilla-aurora?
Is the leave-open keyword still needed here or has everything needed landed now?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Julien Cristau [:jcristau] from comment #25)
> Is the leave-open keyword still needed here or has everything needed landed
> now?

yes
Flags: needinfo?(tbsaunde+mozbugs)
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8828961 [details] [diff] [review]
add test for multiple independent text changes under the same accessible

Fix a broken google sheet issue. Aurora53+.
Attachment #8828961 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8828961 [details] [diff] [review]
add test for multiple independent text changes under the same accessible

let's get this set of a11y fixes in 52.0b3
Attachment #8828961 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
seems this need rebasing for aurora like:

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 6
adding 1322593 to series file
renamed 1322593 -> bug-1322593---add-test-for-multiple-independent-te.patch
applying bug-1322593---add-test-for-multiple-independent-te.patch
patching file accessible/tests/mochitest/events/a11y.ini
Hunk #1 FAILED at 7
1 out of 1 hunks FAILED -- saving rejects to file accessible/tests/mochitest/events/a11y.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1322593---add-test-for-multiple-independent-te.patch
Tomcats-MacBook-Pro-300:mozilla-aurora Tomcat$
Flags: needinfo?(tbsaunde+mozbugs)
Did you apply all the patches here, or just the one?  I'd think dealing with those conflicts would be trivial that file is basically just a list of tests.  If you want I can deal with it though.
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> Did you apply all the patches here, or just the one?  I'd think dealing with
> those conflicts would be trivial that file is basically just a list of
> tests.  If you want I can deal with it though.

just that one - since its the only one with approval or does the others also need to land on aurora/beta ?
The whole patch set should land, per comment 24.
You need to log in before you can comment on or make changes to this bug.