Closed Bug 496235 Opened 11 years ago Closed 10 years ago

Menulist should not set current item to null when using an out-of-range value for selectedIndex

Categories

(Toolkit :: XUL Widgets, defect)

1.9.0 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1

People

(Reporter: whimboo, Assigned: whimboo)

Details

Attachments

(2 files)

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.
Attached image screenshot
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);
}
As you can see other fields which rely on this index change aren't updated.
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.
Aren't you supposed to use onsyncto/frompreference anyway?
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.
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?
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?
Assigning -1 to selectedIndex should clear the selection. Other out-of-range values should probably do nothing.
Other bindings:
Name	-1	Large
deck	clear	clear
listbox	clear	nothing
radio	clear	clear
tabs	nothing	nothing
tree	clear	clear
(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?
(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.
Attached patch Patch v1Splinter Review
Means we should go this way then.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #394068 - Flags: review?(enndeakin)
Attachment #394068 - Flags: review?(enndeakin) → review+
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
Ideally the two menulist tests would be combined and just called test_menulist.xul
http://hg.mozilla.org/mozilla-central/rev/aff827e779e6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1
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
Attachment #394068 - Flags: approval1.9.2?
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.