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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Attachment: 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.

Comment 1

[Marco Zehe (:MarcoZ)]

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.

Comment 2

[James Teh [:Jamie]]

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.

Comment 3

[Marco Zehe (:MarcoZ)]

After some more chat on IRC, decided that this should not block the other one, but that work can happen in parallel.

Comment 4

[James Teh [:Jamie]]

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?

Comment 5

[alexander :surkov (:asurkov)]

Trevor, do you have cycles to check this out, it seems to be a regression from bug 1270916.

Comment 6

[Trevor Saunders (:tbsaunde)]

its been in my queue for a while

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: avoid merging text change events for unrelated accessibles

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: don't coalesce non contiguous text changes under the same accessible

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: allow asynchronous checking of text change events

This allows us to check for multiple text change events that could come in any
order.

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: test that unrelated text change events are not coalesced

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 13

[James Teh [:Jamie]]

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.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1322593 patch

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

Comment 15

[Marco Zehe (:MarcoZ)]

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.

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8828641 [details] [diff] [review]
1322593 patch

># HG changeset patch
># User Yura Zenevich <[e-mail]>
>
>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.

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: add test for multiple independent text changes under the same accessible

It was just a bad test copy pasted from the other one.

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0d86f65852e6

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/14a4da09461f
https://hg.mozilla.org/mozilla-central/rev/af4cbb24227d
https://hg.mozilla.org/mozilla-central/rev/8bc21ac8aad1
https://hg.mozilla.org/mozilla-central/rev/37f5660aecf2
https://hg.mozilla.org/mozilla-central/rev/4091c903d5fa

Comment 23

[Marco Zehe (:MarcoZ)]

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.

Comment 24

[Trevor Saunders (:tbsaunde)]

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.

Comment 25

[Julien Cristau [:jcristau]]

Is the leave-open keyword still needed here or has everything needed landed now?

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to Julien Cristau [:jcristau] from comment #25)
> Is the leave-open keyword still needed here or has everything needed landed
> now?

yes

Comment 27

[Gerry Chang [:gchang]]

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+.

Comment 28

[Julien Cristau [:jcristau]]

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

Comment 29

[Carsten Book [:Tomcat]]

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$

Comment 30

[Trevor Saunders (:tbsaunde)]

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.

Comment 31

[Carsten Book [:Tomcat]]

(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 ?

Comment 32

[Julien Cristau [:jcristau]]

The whole patch set should land, per comment 24.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3664b17ad3c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/babb718be60a
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb3fc0d1eace
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c35b1fd511d
https://hg.mozilla.org/releases/mozilla-aurora/rev/08f7fa74c00d

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f72072777689
https://hg.mozilla.org/releases/mozilla-beta/rev/8336a9d40a9a
https://hg.mozilla.org/releases/mozilla-beta/rev/61c451adc3b1
https://hg.mozilla.org/releases/mozilla-beta/rev/d52e2bceab57
https://hg.mozilla.org/releases/mozilla-beta/rev/5ec5c9a7719b
https://hg.mozilla.org/releases/mozilla-beta/rev/d171c36d4848