Closed Bug 371260 Opened 13 years ago Closed 13 years ago

<radio> cleanup

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(3 files)

Similar to bug 370742, but for radio/radiogroup.

Mainly need bug 367400 fixed though so that the list of radios is kept up to date properly.
fixes:
 - initialize radio group using selected/value attribute
 - don't assume a group with no radios is disabled
 - clear value attribute when changing selection to null
 - make checkAdjacentElement work even when the group isn't focused
 - change mRadioChildren to _radioChildren
 - implement control property
Attachment #257132 - Flags: first-review?(neil)
Duplicate of this bug: 372489
Btw, do we still need radioGroup property if we'll have control?
(In reply to comment #1)
>  - don't assume a group with no radios is disabled
How do you propose to indicate focus on a radiogroup with no radios?
(In reply to comment #4)
> (In reply to comment #1)
> >  - don't assume a group with no radios is disabled
> How do you propose to indicate focus on a radiogroup with no radios?
> 

I guess focus doens't make sense for radiogroup without radios.
Blocks: 372733
(In reply to comment #4)
> (In reply to comment #1)
> >  - don't assume a group with no radios is disabled
> How do you propose to indicate focus on a radiogroup with no radios?
> 

OK, just ignore that part of the patch. (the disabled property getter)
Blocks: 377253
Attachment #257132 - Flags: first-review?(neil) → review?(neil)
Comment on attachment 257132 [details] [diff] [review]
fix various issues

>           var currentElement = this.focusedItem;
>+          if (!currentElement)
>+            currentElement = this.selectedItem;
var currentElement = this.focusedItem || this.selectedItem; perhaps?

>-          var radioList = this.radioGroup.mRadioChildren;
>-          if (!radioList)
>-            return;
>-          for (var i = 0; i < radioList.length; ++i) {
>-            if (radioList[i] == this) {
>-              radioList.splice(i, 1);
>-              return;
>-            }
>-          }
>+          var control = this.control;
>+          if (control)
>+            control._radioChildren = null;
Why this change?
(In reply to comment #7)
> var currentElement = this.focusedItem || this.selectedItem; perhaps?

Will change this.

> Why this change?

I think it was just a cleanup bug, but not necessary. Bug 367400 would be more useful here.
Attachment #257132 - Flags: review?(neil) → review+
adds some functions to EventUtils.js for mouse/key simulation and checking for whether events were fired.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 264240 [details] [diff] [review]
radio buttons api testcase

maybe Robert can review the changes to EventUtils.js
Attachment #264240 - Flags: review?(sayrer)
Comment on attachment 264240 [details] [diff] [review]
radio buttons api testcase

These look good to me, and I'll assume the event details are correct if your tests are passing.
Attachment #264240 - Flags: review?(sayrer) → review+
Checked in the events changes
Attachment #264481 - Flags: review?(mano)
Comment on attachment 264481 [details] [diff] [review]
this patch is just the radio buttons testcase

r=mano
Attachment #264481 - Flags: review?(mano) → review+
Checked in the tests
Flags: in-testsuite? → in-testsuite+
(In reply to comment #15)
> Checked in the tests
> 

disabled these tests. they were causing red on qm-winxp01.

Checking in toolkit/content/tests/widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
removing from the makefile doesn't stop the tests from running.

Removing toolkit/content/tests/widgets/test_radio.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_radio.xul,v  <--  test_radio.xul
new revision: delete; previous revision: 1.1
done
Removing toolkit/content/tests/widgets/xul_selectcontrol.js;
/cvsroot/mozilla/toolkit/content/tests/widgets/xul_selectcontrol.js,v  <--  xul_selectcontrol.js
new revision: delete; previous revision: 1.1
done
sorry, these didn't need to be removed. disabling them in the makefile should have stopped them from running.

however, according to the tinderbox log, that *did* in fact fix it:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&hours=24&maxdate=1186149592&legend=0

i'm not sure why i would have removed them if the build succeeded, but the log clearly shows that was the case.
Depends on: 541313
Depends on: 1076765
You need to log in before you can comment on or make changes to this bug.