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

RESOLVED FIXED in mozilla25

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla25
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.

Comment 1

5 years ago
Fwiw, NVDA needs this too. I just never thought of it before. :)
(Assignee)

Comment 2

5 years ago
(In reply to James Teh [:Jamie] from comment #1)
> Fwiw, NVDA needs this too. I just never thought of it before. :)

ok, cool :)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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?
(Assignee)

Comment 6

4 years ago
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: 887794
(Assignee)

Comment 7

4 years ago
Created attachment 779323 [details] [diff] [review]
patch
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+
(Assignee)

Comment 9

4 years ago
(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
(Assignee)

Comment 10

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7fe79f773eb4
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/7fe79f773eb4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.