Closed
Bug 320369
Opened 19 years ago
Closed 18 years ago
wrong behavior of radiobox status update in GOK
Categories
(Firefox :: Disability Access, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: evan.yan, Assigned: evan.yan)
References
(Depends on 1 open bug)
Details
(Keywords: access, fixed1.8.1)
Attachments
(1 file, 3 obsolete files)
3.73 KB,
patch
|
neil
:
review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
in gok, back and re UI grab will be OK. no radiostatechange event for the unchecked element. Aaron, is it necessary?
FYI: On Windows, Firefox only fire STATECHANGE and FOCUS event for checked radio.
Assignee: nobody → Evan.Yan
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Status: NEW → ASSIGNED
Comment 4•19 years ago
|
||
I think option #2 is okay -- on Linux. Keep things the same on Windows, otherwise some screen readers will accidentally read both radio buttons.
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?
Updated•18 years ago
|
Blocks: fox2access
Comment 6•18 years ago
|
||
How does the fix for bug 333833 affect this?
(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
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.
Attachment #220113 -
Flags: review?(aaronleventhal)
Comment 9•18 years ago
|
||
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 neil@httl.net. I don't know if my second suggestion 2 remove lines of code is correct, but he would know.
Attachment #220113 -
Flags: review?(aaronleventhal) → review-
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #220113 -
Attachment is obsolete: true
Attachment #220222 -
Flags: review?(neil)
Assignee | ||
Comment 11•18 years ago
|
||
(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•18 years ago
|
||
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.
Attachment #220222 -
Flags: review?(neil) → review-
Assignee | ||
Comment 13•18 years ago
|
||
Attachment #220222 -
Attachment is obsolete: true
Attachment #220287 -
Flags: review?(neil)
Comment 14•18 years ago
|
||
Comment on attachment 220287 [details] [diff] [review] patch v3 Just to clarify, the intent is that previousItem is an element, not an index.
Attachment #220287 -
Flags: review?(neil) → review-
Comment 15•18 years ago
|
||
(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.
Assignee | ||
Comment 16•18 years ago
|
||
sorry about my misunderstanding.
Attachment #220287 -
Attachment is obsolete: true
Attachment #221273 -
Flags: review?(neil)
Comment 17•18 years ago
|
||
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) {
Attachment #221273 -
Flags: review?(neil) → review+
Comment 18•18 years ago
|
||
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.
Attachment #221273 -
Flags: approval-branch-1.8.1?(mconnor)
Comment 19•18 years ago
|
||
Comment on attachment 221273 [details] [diff] [review] patch v4 a=me with Neil's comment addressed
Attachment #221273 -
Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Comment 20•18 years ago
|
||
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•18 years ago
|
||
Could this have caused regression bug 338963 ?
Comment 22•18 years ago
|
||
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.
Updated•16 years ago
|
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•