Extra accessibility API focus events fired while traversing pref categories

VERIFIED FIXED

Status

()

VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

16 years ago
While in the preferences category tree, arrowing up and down causes extra
MSAA/ATK focus events to be fired on radio/checbox elements in the right-hand
prefs panels.

These items are not focused, and the extra focus events will cause screen
readers to read the incorrect focus.
(Assignee)

Comment 1

16 years ago
Created attachment 103606 [details] [diff] [review]
Fire CheckboxStateChange only if change is occuring, and RadioStateChange only if change is occuring and radio group has focus

Seeking r=/sr=

Comment 2

16 years ago
+            var event = document.createEvent("Events");
+            event.initEvent("select", false, true);
+            this.dispatchEvent(event);
Blake, I wonder why we need this event here? Is that event used anywhere? The 
w3c dom specification says, "The select event occurs when a user selects some 
text in a text field. This event is valid for INPUT and TEXTAREA elements." XUL 
should be consistent with HTML.
(Assignee)

Comment 3

16 years ago
Kyle, I still think this is r= worthy.

That seems like a separate bug.

Comment 4

16 years ago
okay, go ahead to seek r=/sr=. I'll file a new bug against this.
(Assignee)

Comment 5

16 years ago
Seeking r=Samir

Comment 6

16 years ago
Comment on attachment 103606 [details] [diff] [review]
Fire CheckboxStateChange only if change is occuring, and RadioStateChange only if change is occuring and radio group has focus

r=sgehani
Attachment #103606 - Flags: review+
Comment on attachment 103606 [details] [diff] [review]
Fire CheckboxStateChange only if change is occuring, and RadioStateChange only if change is occuring and radio group has focus

>       <!-- public implementation -->
>-      <property name="checked"    onset="if (val) this.setAttribute('checked', 'true');
>+      <property name="checked"    onset="var change = (val != (this.getAttribute('checked') == 'true'));
>+                                         if (val) this.setAttribute('checked', 'true');
>                                          else this.removeAttribute('checked');
>-                                         var event = document.createEvent('Events');
>-                                         event.initEvent('CheckboxStateChange', false, true);
>-                                         this.dispatchEvent(event);
>+                                         if (change) {
>+                                           var event = document.createEvent('Events');
>+                                           event.initEvent('CheckboxStateChange', false, true);
>+                                           this.dispatchEvent(event);
>+                                         }
>                                          return val;"

I'd prefer if you made this a separate function, rather than putting it all in
the onset=.


>-          var event = document.createEvent("Events");
>-          event.initEvent("select", false, true);
>-          this.dispatchEvent(event);
> 
>-          var myEvent = document.createEvent("Events");
>-          myEvent.initEvent("RadioStateChange", true, true);
>-          val.dispatchEvent(myEvent);
>+          if (!alreadySelected) {
>+            // Only report if actual change
>+            var event = document.createEvent("Events");
>+            event.initEvent("select", false, true);
>+            this.dispatchEvent(event);
>+
>+            if (focused) {
>+              var myEvent = document.createEvent("Events");
>+              myEvent.initEvent("RadioStateChange", true, true);
>+              val.dispatchEvent(myEvent);
>+            }
>+          }
>+

If we're going to revisit the "select" event in a separate bug, I'd prefer to
take the safe approach and not change the behavior here by moving it inside |if
(!alreadySelected)|.  Just do the extra check for the accessibility event.
(Assignee)

Comment 8

16 years ago
Created attachment 104584 [details] [diff] [review]
Contains bryner's fixes.

Seeking sr=
(Assignee)

Updated

16 years ago
Attachment #103606 - Attachment is obsolete: true
(Assignee)

Comment 9

16 years ago
Comment on attachment 104584 [details] [diff] [review]
Contains bryner's fixes.

carrying r= forward
Attachment #104584 - Flags: review+
Comment on attachment 104584 [details] [diff] [review]
Contains bryner's fixes.

Sorry, one more nit:  when you're checking to see if the checked state changed,
I think you could use this.hasAttribute rather than getAttribute.  It's more
efficient, and 'checked' is always either not present or set to true. 
sr=bryner with that change.
Attachment #104584 - Flags: superreview+
Actually, don't worry about the nit.  getAttribute() is more correct, to account
for setting the attribute to the string "false".
(Assignee)

Comment 12

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 13

16 years ago
Comment on attachment 104584 [details] [diff] [review]
Contains bryner's fixes.

I don't think we want to take this change into 1.2 this late in the game. If
this is needed for any branch consumers post 1.2 then maybe it can land then.
Attachment #104584 - Flags: approval-

Comment 14

16 years ago
-- Verified in current Mozilla trunk build(20021114).
Arrowing up and down in the preferences category tree does not cause any extra
focus events to be fired.
Marking Verified.
Status: RESOLVED → VERIFIED

Comment 15

16 years ago
> I'd prefer if you made this a separate function, rather than putting it all in
> the onset=.

Actually you should use <setter> here.
You need to log in before you can comment on or make changes to this bug.