Coalescence of state change events is bad

RESOLVED FIXED in mozilla21

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: davidb, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Spin off from bug 564471 research and discussion with Alexander.

Looking over our current implementation I don't see how we avoid mistakenly removing 'duplicate' EVENT_STATE_CHANGE events (which can be different, e.g. EXT_STATE_FOO EXT_STATE_BAR).
(Reporter)

Comment 1

7 years ago
So we need to consider two related things:

1. What to do when the delayed event has an initialized accessible but not an initialzed node.
2. What to do about false hits for removing duplicates.

For 1, we can compare accessibles as well, instead of just nodes.

For 2, we can consider comparing the state. This could be a general member of the base class (perhaps called mSubType or something) which could give us a general solution if we need this for other event types. For state changes we would just mSubType(aState).
(Reporter)

Comment 2

7 years ago
For 1, we might consider always initializing the node member variable. We need to consider the life cycle of nodes and accessibles (and debug).
(Assignee)

Comment 3

4 years ago
Created attachment 693350 [details] [diff] [review]
patch

1) make state change events coalescence (Trevor)
2) change eventQueue to handle alternatives (named as scenarios), otherwise we'll regress testing from bug 446469 (Marco).
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #693350 - Flags: review?(trev.saunders)
Attachment #693350 - Flags: review?(marco.zehe)
(Reporter)

Comment 4

4 years ago
Wow thanks for getting to this bug!
(nit: don't forget to recomment gA11yEventDumpToConsole)
(Assignee)

Comment 5

4 years ago
(In reply to David Bolter [:davidb] from comment #4)
> Wow thanks for getting to this bug!

why is it important for you?

> (nit: don't forget to recomment gA11yEventDumpToConsole)

yeah, thanks, usually I skim the patch after I uploaded it so I fix some things after that.
(Reporter)

Comment 6

4 years ago
(In reply to alexander :surkov from comment #5)
> (In reply to David Bolter [:davidb] from comment #4)
> > Wow thanks for getting to this bug!
> 
> why is it important for you?

Just feeling badly that it sat for so long.
(Assignee)

Comment 7

4 years ago
(In reply to David Bolter [:davidb] from comment #6)

> Just feeling badly that it sat for so long.

yeah, you must be feeling bad permanently ;)
Comment on attachment 693350 [details] [diff] [review]
patch

>+          if (thisSCEvent->mState == tailSCEvent->mState) {
>+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
>+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
>+              tailEvent->mEventRule = AccEvent::eDoNotEmit;

nit, assert the mIsEnabled are different?

r=me for accessible/src/ (I didn't read the tests)
Attachment #693350 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 9

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 693350 [details] [diff] [review]
> patch
> 
> >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> 
> nit, assert the mIsEnabled are different?

why?

> r=me for accessible/src/ (I didn't read the tests)

those are really two different but cycle dependent bugs.
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Comment on attachment 693350 [details] [diff] [review]
> > patch
> > 
> > >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> > >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> > >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> > >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> > 
> > nit, assert the mIsEnabled are different?
> 
> why?
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > Comment on attachment 693350 [details] [diff] [review]
> > > patch
> > > 
> > > >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> > > >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> > > >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> > > >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> > > 
> > > nit, assert the mIsEnabled are different?
> > 
> > why?

because I forgoforget it, I forgot we might have already coalesced an intermediate state change event away.  What I wanted to assert was that we never fire state foo enabled event then another state foo enabled event witout disabling state foo in between.
Comment on attachment 693350 [details] [diff] [review]
patch

>+    gA11yEventDumpToConsole = true; // debug stuff

Nit: Remove this before checkin, which will obsolete any diffs to actions/anchors.html.

>+ *     // [optional] if true then event sequances are ignored (no failure if

Nit: sequences, not sequances.

>+                ok(false,
>+                   "We have a matched scendario at index " + matchIdx + " already.");

Nit: "...scenario".

>+        if (!checker.unexptected && checker.unique &&

This looks like a typo which may have borked your tests in some way. Please check that you've not missed anything.

r=me with those nits fixed and the last point checked.
Attachment #693350 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 13

4 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #12)

> >+        if (!checker.unexptected && checker.unique &&
> 
> This looks like a typo which may have borked your tests in some way. Please
> check that you've not missed anything.

comment was updated to:

// Report an error if we hanlded not expected event of unique type
// (i.e. event types are matched, targets differs).
(Assignee)

Comment 14

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/696264e2414c
Flags: in-testsuite+
(Reporter)

Comment 15

4 years ago
(In reply to alexander :surkov from comment #14)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/696264e2414c

Looks like you accidentally missed Marco's nit about 'unexptected'...
(Reporter)

Comment 16

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/da88cd4bc6db
(Reporter)

Comment 17

4 years ago
(I should mention it really wasn't a 'nit' in case that matters later)
(Assignee)

Comment 18

4 years ago
thanks!
https://hg.mozilla.org/mozilla-central/rev/696264e2414c
https://hg.mozilla.org/mozilla-central/rev/da88cd4bc6db
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Backed out for intermittent, but pretty frequent test_doc_busy.html timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18616115&tree=Firefox

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0794537c6d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla21 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/4a0794537c6d
(Assignee)

Comment 22

4 years ago
landing by parts:
part 1 (support the event scenarios in mochitests): http://hg.mozilla.org/integration/mozilla-inbound/rev/3bf0fc40df43
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/3bf0fc40df43
(Assignee)

Comment 24

4 years ago
2nd part: http://hg.mozilla.org/integration/mozilla-inbound/rev/d00f2767e57c, logging is enabled in case of failures
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/d00f2767e57c
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.