320369 - wrong behavior of radiobox status update in GOK

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Evan Yan]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051214 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051214 Firefox/1.6a1

Using GOK UI grab function, when changing selection of radiobox in UI, the selection status in GOK does not changing correspondingly.

Reproducible: Always

Steps to Reproduce:
1. Open GOK.
2. In firefox, select Menu->Edit->Preference->Connection Settings
3. In GOK, select UI Grab
4. Change the radiobox selection from firefox UI or GOK control panel

Actual Results:  
All of the radioboxes you ever selected are marked as selected status.

Expected Results:  
Mark the last selected radiobox as selected, and clear the others.

Comment 1

[Nian Liu(n/a in a long time)]

in gok, back and re UI grab will be OK.

no radiostatechange event for the unchecked element.

Aaron, is it necessary?

Comment 2

[Ginn Chen]

FYI:
On Windows, Firefox only fire STATECHANGE and FOCUS event for checked radio.

Comment 3

[Evan Yan]

when select a radio, /toolkit/content/widgets/radio.xml will fire "select" and "RadioStateChange" event for the selected radio, see
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/radio.xml#96
then, nsRootAccessible::HandleEvent() will fire EVENT_STATE_CHANGE and EVENT_FOCUS when it handles the "RadioStateChange" event. see
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#843
But no EVENT_STATE_CHANGE fired for the unselected radio.

I think there are two solutions.
1. in nsRootAccessible::HandleEvent(), try to get the other radios in the same radiogroup and fire EVENT_STATE_CHANGE for them. But it may not so easy and elegant to get other radios in accessible module.
2. fire "RadioStateChange" event for both of the selected and unselected radios in Radio.xml. But it may make the event listeners other than nsRootAccessible::HandleEvent() be called unnecessarily.

In fact, it also cause other bugs that events are only fired for the current widget. For example, when check/uncheck a radio/checkbox makes some other widgets enable/disable, the states of the enabled/disabled widgets won't be updated in GOK because of no event fired for them.

It may need a big change to fix the bug thoroughly.

Aaron, how do you think? We need your comments.

thanks,

evan

Comment 4

[Aaron Leventhal]

I think option #2 is okay -- on Linux.

Keep things the same on Windows, otherwise some screen readers will accidentally read both radio buttons.

Comment 5

[Ginn Chen]

Aaron,
what about other widgets relate to the radiobox?
e.g. In "Connection Settings" dialog, switching between "Direct connection ..." and "Manual proxy ..." will cause TextBoxes and checkbox disable/enable.
With Firefox 1.5/Windows, I didn't find events for them using AccEvent Watcher.
Do you think we should also fix them?

Comment 6

[Aaron Leventhal]

How does the fix for bug 333833 affect this?

Comment 7

[Evan Yan]

(In reply to comment #6)
> How does the fix for bug 333833 affect this?
> 
no. that patch doesn't fix this bug.

I have worked out a patch for XUL radio button, and still working on HTML radio button.

The current situation of HTML radio button:
both of selected and unselected radio buttons' status are not updated in GOK. no "RadioStateChange" event is fired for any of them.

there is no event dispatch implementation for HTML radio button, see
http://lxr.mozilla.org/seamonkey/source/layout/forms/resources/content/radio.xml

Comment 8

[Evan Yan]

Attachment: patch

fire "RadioStateChange" event for unselected radio.

this patch works for xul radio.
for html input element, like radio/checkbox, event was fired from nsHTMLInputElement::FireEventForAccessibility(), but never reach nsRootAccessible::HandleEvent(). There seems something wrong in event handling.
I think that should be another bug.

Comment 9

[Aaron Leventhal]

Comment on attachment 220113 [details] [diff] [review]
patch

+ children[unselectedIndex].dispatchEvent(myEvent);
Make sure unselected is never undefined when you use it.

+            myEvent = document.createEvent("Events");
+            myEvent.initEvent("RadioStateChange", true, true);
I believe these 2 lines of code are unnecessary and redundant? Or does the same type of event need to be created again because it was fired once?

Chrome (UI) patches only need review, not super review. But, the review must be from a UI peer. I recommend submitting a new patch and using [e-mail]. I don't know if my second suggestion 2 remove lines of code is correct, but he would know.

Comment 10

[Evan Yan]

Attachment: patch v2

Comment 11

[Evan Yan]

(In reply to comment #9)
> +            myEvent = document.createEvent("Events");
> +            myEvent.initEvent("RadioStateChange", true, true);
> I believe these 2 lines of code are unnecessary and redundant? Or does the same
> type of event need to be created again because it was fired once?
> 
yes, I tried to reuse myEvent, but it didn't work. so I called creatEvent() again.

Comment 12

[[e-mail]]

Comment on attachment 220222 [details] [diff] [review]
patch v2

I suppose the accessibility code is so ugly because of all the constraints that need to be satisfied before an event can be fired?

>           var children = this._getRadioChildren();
>+          var unselectedIndex = -1;
I think I'd like this to be "previousItem" which is initially null and is set in the loop below to the actual element. That would also mean you could use if (previousItem) { ... }

>+              if (children[i].getAttribute("selected") == "true") {
>+                unselectedIndex = i;
>+              }
In these files one-line ifs should not use {}s.

Comment 13

[Evan Yan]

Attachment: patch v3

Comment 14

[[e-mail]]

Comment on attachment 220287 [details] [diff] [review]
patch v3

Just to clarify, the intent is that previousItem is an element, not an index.

Comment 15

[[e-mail]]

(In reply to comment #12)
>I suppose the accessibility code is so ugly because of all the constraints that
>need to be satisfied before an event can be fired?
By which I mean I would have suggested using a radio button selected setter to fire the radio state change but extra code would have been needed to limit the event to the cases when the state actually changed and the group was focused.

Comment 16

[Evan Yan]

Attachment: patch v4

sorry about my misunderstanding.

Comment 17

[[e-mail]]

Comment on attachment 221273 [details] [diff] [review]
patch v4

>+            if (previousItem != null) {
We don't compare things to true, false or null unless there's a potential type-safety issue (e.g. some variables could legitimately be 0 or ""). r=me if you change both of these to
              if (previousItem) {

Comment 18

[Aaron Leventhal]

Comment on attachment 221273 [details] [diff] [review]
patch v4

Not sure if this one needs any further reviews / superreviews. It doesn't affect the UI.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 221273 [details] [diff] [review]
patch v4

a=me with Neil's comment addressed

Comment 20

[Ginn Chen]

Checking in toolkit/content/widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.20; previous revision: 1.19
done
Checking in xpfe/global/resources/content/bindings/radio.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v  <--  radio.xml
new revision: 1.42; previous revision: 1.41
done

Checking in toolkit/content/widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.17.8.4; previous revision: 1.17.8.3
done
Checking in xpfe/global/resources/content/bindings/radio.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v  <--  radio.xml
new revision: 1.39.8.2; previous revision: 1.39.8.1
done

Comment 21

[Kai Engert (:KaiE:)]

Could this have caused regression bug 338963 ?

Comment 22

[Kai Engert (:KaiE:)]

Please disregard my previous comment for now. It seems the bug was present in builds around 2006-05-19, but most recent trunk builds behave correctly for me.