Closed
Bug 496235
Opened 15 years ago
Closed 15 years ago
Menulist should not set current item to null when using an out-of-range value for selectedIndex
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a1
People
(Reporter: whimboo, Assigned: whimboo)
Details
Attachments
(2 files)
65.69 KB,
image/jpeg
|
Details | |
733 bytes,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090603 Shiretoko/3.5pre ID:20090603031333 Using an out-of-bounds value for selectedIndex will cause the menulist to reset the selectedItem and set it to null. See http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/menulist.xml#181 Since we apply changes on OS X directly this could cause unpredictable behavior. I have seen this problem while testing a Mozmill API which gives us access to menulists. Supplying a value which is not within the length of popup items should not change the index of a menulist.
Assignee | ||
Comment 1•15 years ago
|
||
This shows the privacy panel after an index of 3 was set. The Mozmill test which shows this behavior is: var mozmill = {}; Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill); var elementslib = {}; Components.utils.import('resource://mozmill/modules/elementslib.js', elementslib); var setupModule = function(module) { controller = mozmill.getPreferencesController(); } var testRecorded = function () { var menulist = new elementslib.ID(controller.window.document, "historyMode"); controller.select(menulist, 3, null, null); }
Assignee | ||
Comment 2•15 years ago
|
||
As you can see other fields which rely on this index change aren't updated.
Assignee | ||
Comment 3•15 years ago
|
||
Mmh don't we even fire a index changed event when setting the new value automatically without a user interaction by directly accessing selectedIndex. I used 0 for the index and I still see all options from custom settings.
Comment 4•15 years ago
|
||
Aren't you supposed to use onsyncto/frompreference anyway?
Assignee | ||
Comment 5•15 years ago
|
||
Those are the only menulists which I was able to find in the UI. They are connected to prefs, yes. But what about any menulist which is stand-alone. It will show this behavior.
Assignee | ||
Comment 6•15 years ago
|
||
Neil, this only occurs when directly assigning the value to selectedIndex. But when using getItemAtIndex(index).click() it works fine. Is it intended that we don't have a range check for selectedIndex?
Assignee | ||
Comment 7•15 years ago
|
||
Ok, I was a bit too fast. Using getItemAtIndex with an out-of-range value will indeed return null which makes click fail. So shall someone never assign the index directly to selectedIndex?
Comment 8•15 years ago
|
||
Assigning -1 to selectedIndex should clear the selection. Other out-of-range values should probably do nothing.
Comment 9•15 years ago
|
||
Other bindings: Name -1 Large deck clear clear listbox clear nothing radio clear clear tabs nothing nothing tree clear clear
Assignee | ||
Comment 10•15 years ago
|
||
(In reply to comment #8) > Assigning -1 to selectedIndex should clear the selection. Other out-of-range > values should probably do nothing. When I use a value which is greater as the count of items selectedIndex is also set to -1. Given the comment from Neil it should select the last item in the list?
Comment 11•15 years ago
|
||
(In reply to comment #10) > When I use a value which is greater as the count of items selectedIndex is also > set to -1. Given the comment from Neil it should select the last item in the > list? No, it should do nothing.
Assignee | ||
Comment 12•15 years ago
|
||
Means we should go this way then.
Updated•15 years ago
|
Attachment #394068 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 13•15 years ago
|
||
Neil, would the following test be the right one to add a test or do we have another one which fits better? http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_menulist_null_value.xul
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 14•15 years ago
|
||
Ideally the two menulist tests would be combined and just called test_menulist.xul
Comment 15•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/aff827e779e6
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Updated•15 years ago
|
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1
Assignee | ||
Comment 16•15 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre ID:20090821030931
Status: RESOLVED → VERIFIED
Version: unspecified → 1.9.0 Branch
Assignee | ||
Updated•15 years ago
|
Attachment #394068 -
Flags: approval1.9.2?
Comment 17•15 years ago
|
||
Comment on attachment 394068 [details] [diff] [review] Patch v1 approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.
Attachment #394068 -
Flags: approval1.9.2?
You need to log in
before you can comment on or make changes to this bug.
Description
•