Closed Bug 788389 Opened 7 years ago Closed 6 years ago

fire statechange event whenever checked state is changed not depending on focused state

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

We were asked to fire statechange event whenever checked state is changed, for example, if the author does checkbox.checked = true. Historically we fired the event when the widget was focused. We started to do exceptions on this way, for example, for ARIA attributes like bug 467143.
Fwiw, NVDA needs this too. I just never thought of it before. :)
(In reply to James Teh [:Jamie] from comment #1)
> Fwiw, NVDA needs this too. I just never thought of it before. :)

ok, cool :)
Instead of listening the CheckboxStateChange DOM event we need to listen content state change for HTML and checked attribute change for XUL.

Consequences
1) this approach allows to fix bug 477997
2) we can start to keep cached accessible states for example mStates (too fat for one cached state though)
3) we can provide HasNativeState(aState) method optimized to work on mFlags like
a) if (mStates & aState) return true; if (states::FOCUSABLE & aState) return NativeInteractiveState() & aState; etc.

Trev, does it look good?
(In reply to alexander :surkov from comment #3)
> Instead of listening the CheckboxStateChange DOM event we need to listen
> content state change for HTML and checked attribute change for XUL.

Well, I'm basically always for getting rid of usage of DOM events :)

> Consequences
> 1) this approach allows to fix bug 477997
> 2) we can start to keep cached accessible states for example mStates (too
> fat for one cached state though)

I'm not sure that needs to be part of this, but if we were to do it I think we could probably cache a couple other already, but that would need to be checked.

> 3) we can provide HasNativeState(aState) method optimized to work on mFlags
> like
> a) if (mStates & aState) return true; if (states::FOCUSABLE & aState) return
> NativeInteractiveState() & aState; etc.

That could be nice, but if the state is cached as off we don't take advantage of the cache which seems like something we should do.

> Trev, does it look good?

I'm not thrilled by the thought of making every Accessible 8 bytes larger, but its probably worth it, and maybe we can be clever and not increase size that much.
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > 2) we can start to keep cached accessible states for example mStates (too
> > fat for one cached state though)
> 
> I'm not sure that needs to be part of this, but if we were to do it I think
> we could probably cache a couple other already, but that would need to be
> checked.

I think the idea was to keep checkable as part of mStates (we could do that in mFlags though) because we need to know at least for XUL when checked is valid attribute and when it's not.

> I'm not thrilled by the thought of making every Accessible 8 bytes larger,
> but its probably worth it, and maybe we can be clever and not increase size
> that much.

for example?
I've been asked again about this (so adding dependency on bug 2013q3a11y). I think to fix the bug HTML checkbox and radios only (leaving the idea from comment #3 out of board for now). So XUL elements and other controls will be still focus dependent.
Blocks: 2013q3a11y
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #779323 - Flags: review?(trev.saunders)
Comment on attachment 779323 [details] [diff] [review]
patch

>-
>-    // radiogroup in prefWindow is exposed as a list,
>-    // and panebutton is exposed as XULListitem in A11y.
>-    // XULListitemAccessible::GetStateInternal uses STATE_SELECTED in this case,
>-    // so we need to check states::SELECTED also.
>     bool isEnabled = (state & (states::CHECKED | states::SELECTED)) != 0;
> 
>-    nsRefPtr<AccEvent> accEvent =
>-      new AccStateChangeEvent(accessible, states::CHECKED, isEnabled);
>-    nsEventShell::FireEvent(accEvent);
>+    if (accessible->NeedsDOMUIEvent()) {

why can't we just bail out at the start if dom ui events aren't needed?

    var gQueue = null;
> 
>     // var gA11yEventDumpID = "eventdump"; // debug stuff
>-    //gA11yEventDumpToConsole = true; // debug stuff
>+    gA11yEventDumpToConsole = true; // debug stuff

keep commented
Attachment #779323 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 779323 [details] [diff] [review]
> patch
> 
> >-
> >-    // radiogroup in prefWindow is exposed as a list,
> >-    // and panebutton is exposed as XULListitem in A11y.
> >-    // XULListitemAccessible::GetStateInternal uses STATE_SELECTED in this case,
> >-    // so we need to check states::SELECTED also.
> >     bool isEnabled = (state & (states::CHECKED | states::SELECTED)) != 0;
> > 
> >-    nsRefPtr<AccEvent> accEvent =
> >-      new AccStateChangeEvent(accessible, states::CHECKED, isEnabled);
> >-    nsEventShell::FireEvent(accEvent);
> >+    if (accessible->NeedsDOMUIEvent()) {
> 
> why can't we just bail out at the start if dom ui events aren't needed?

in case of RadioStateChange it's still needed to fire focus events
https://hg.mozilla.org/mozilla-central/rev/7fe79f773eb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.