Bug 11447 – REGRESSION(NativeListBox): List not scrolled to preselected option

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Matt Lilek]

The scrollbar in the new list boxes default to the very top despite the inital selection being farther down among the options.

When you load the testcase, the scroll position should be about halfway down.  The correct item is selected and displayed, but the scrollbar is all the way at the top.  Also, when the list box loses focus, it jumps back to default selection (scroll all the way to the bottom then click outside).

[Matt Lilek]

Created attachment 11272 [details]
testcase

[Matt Lilek]

A few things have changed since this was originally filed (as of r19135):

(In reply to comment #0)
> The scrollbar in the new list boxes default to the very top despite the inital
> selection being farther down among the options.

This still happens and is the current behavior.

> The correct item is selected and displayed, but the scrollbar is all the way at
> the top.

This no longer happens.  The selected item isn't displayed, the list box is positioned all the way at the top, completely ignoring the selected option, though it is properly selected when scrolling through.

> Also, when the list box loses focus, it jumps back to default
> selection (scroll all the way to the bottom then click outside).

This also no longer happens.

[Matt Lilek]

<rdar://problem/4957463>

[Sam Weinig]

Are we sure this is a bug?  Firefox exhibits the same behavior as ToT.

[Matt Lilek]

(In reply to comment #4)
> Are we sure this is a bug?  Firefox exhibits the same behavior as ToT.
> 

For me, Firefox exhibits the same behavior as Safari 2.0.4 while ToT ignores the pre-selected option in the testcase.

[Antti Koivisto]

Created attachment 13299 [details]
Scroll list to initial position if needed

Also ensure that item list is up to date after attach();detach(); changes renderer from listbox to menulist (this is needed along with the other change to prevent a layout test failure).

[Maciej Stachowiak]

Depending on closeRenderer is always a bit suspicious since it only gets called by the parser, and not in the dynamic construction case. If you construct a <select> element via pure DOM calls, do we still do the right thing?

[Maciej Stachowiak]

Comment on attachment 13299 [details]
Scroll list to initial position if needed

Per discussion with Antti, this also needs code to handle a dynamically constructed select element.

[Antti Koivisto]

Created attachment 13314 [details]
initiate scroll from insertedIntoDocument to cover dynamic cases too

[Darin Adler]

Comment on attachment 13314 [details]
initiate scroll from insertedIntoDocument to cover dynamic cases too

I'd like to see the test case cover all three of the code paths.

The ChangeLog doesn't mention the new HTMLOptionElement files in this patch.

+    HTMLSelectElement* select = 0;

No reason to initialize this to 0.

+//sel.setAttribute('selected','selected');

Is this commented out intentionally?

This is pretty good, but I'm going to review- it since the ChangeLog and test case don't match the code.

[Antti Koivisto]

Created attachment 13321 [details]
updated patch

- fixed ChangeLog
- cleaned up testcase
- added tests for size=1 (MenuList) case too
- removed unnecessary = 0

[Darin Adler]

Comment on attachment 13321 [details]
updated patch

r=me

[Antti Koivisto]

r19797