Closed Bug 569356 Opened 14 years ago Closed 11 years ago

Coalescence of state change events is bad

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: davidb, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
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).
For 1, we might consider always initializing the node member variable. We need to consider the life cycle of nodes and accessibles (and debug).
Attached patch patchSplinter Review
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)
Wow thanks for getting to this bug!
(nit: don't forget to recomment gA11yEventDumpToConsole)
(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.
(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.
(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+
(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+
(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).
(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'...
(I should mention it really wasn't a 'nit' in case that matters later)
thanks!
https://hg.mozilla.org/mozilla-central/rev/696264e2414c
https://hg.mozilla.org/mozilla-central/rev/da88cd4bc6db
Status: ASSIGNED → RESOLVED
Closed: 11 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 → ---
landing by parts:
part 1 (support the event scenarios in mochitests): http://hg.mozilla.org/integration/mozilla-inbound/rev/3bf0fc40df43
Whiteboard: [leave open]
2nd part: http://hg.mozilla.org/integration/mozilla-inbound/rev/d00f2767e57c, logging is enabled in case of failures
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/d00f2767e57c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.