365773 - radiogroup.selectedItem = null fails if group is focused

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Wladimir Palant]

The radiogroup XUL tag implementation contains the following code in the selectedItem setter:

          if (!alreadySelected && focused) {
            // Only report if actual change
            var myEvent = document.createEvent("Events");
            myEvent.initEvent("RadioStateChange", true, true);
            val.dispatchEvent(myEvent);

Problem: that val might be null and this case is not checked - the setter will throw an exception. Condition is that current document focus is on this radio group, otherwise focused will be false.

Comment 1

[Wladimir Palant]

Attachment: mochitest compatible testcase

Comment 2

[Wladimir Palant]

Attachment: mochitest compatible testcase

Sorry, attached wrong file - now the correct testcase.

Comment 3

[Wladimir Palant]

Attachment: Add missing check

Trivial fix, selectedItem setter no longer throws an exception.

Neil, can you review this? You reviewed the previous patch in this area.

Comment 4

[[e-mail]]

Comment on attachment 250336 [details] [diff] [review]
Add missing check

Nit: scoping of myEvent "looks" incorrect. Please move it before if (val)

Note: Setting selectedItem to null seems to kill the focus and break keyboard navigation because focusedItem should not be null while the radiogroup has focus. (Aside: Why don't we have a focusedIndex property?)

Comment 5

[Wladimir Palant]

Attachment: Add missing check (Neil's comment addressed)

Does it need a second review to be checked in?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/radio.xml 	1.24
mozilla/xpfe/global/resources/content/bindings/radio.xml 	1.45

Comment 7

[[e-mail]]

Comment on attachment 250335 [details]
mochitest compatible testcase

Sorry, I don't know the test harness.

Comment 8

[Wladimir Palant]

Comment on attachment 250335 [details]
mochitest compatible testcase

Robert, can you review this testcase?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mozilla/testing/mochitest/tests/test_bug365773.xul 	1.1

Comment 10

[Wladimir Palant]

Gavin, mozilla/testing/mochitest/tests/index.html needs to be updated as well when checking in testcases.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Ah, right, thanks for catching my mistake. I see Rob already fixed it.