Bug 11447 - REGRESSION(NativeListBox): List not scrolled to preselected option
Summary: REGRESSION(NativeListBox): List not scrolled to preselected option
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Antti Koivisto
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-10-28 21:36 PDT by Matt Lilek
Modified: 2007-04-24 22:42 PDT (History)
3 users (show)

See Also:


Attachments
testcase (1.75 KB, text/html)
2006-10-28 21:37 PDT, Matt Lilek
no flags Details
Scroll list to initial position if needed (35.03 KB, patch)
2007-02-21 09:57 PST, Antti Koivisto
mjs: review-
Details | Formatted Diff | Diff
initiate scroll from insertedIntoDocument to cover dynamic cases too (41.37 KB, patch)
2007-02-22 06:28 PST, Antti Koivisto
darin: review-
Details | Formatted Diff | Diff
updated patch (66.90 KB, patch)
2007-02-22 11:17 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lilek 2006-10-28 21:36:00 PDT
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).
Comment 1 Matt Lilek 2006-10-28 21:37:11 PDT
Created attachment 11272 [details]
testcase
Comment 2 Matt Lilek 2007-01-26 12:46:43 PST
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.
Comment 3 Matt Lilek 2007-01-26 12:47:48 PST
<rdar://problem/4957463>
Comment 4 Sam Weinig 2007-02-14 17:59:54 PST
Are we sure this is a bug?  Firefox exhibits the same behavior as ToT.
Comment 5 Matt Lilek 2007-02-14 18:56:28 PST
(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.
Comment 6 Antti Koivisto 2007-02-21 09:57:54 PST
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).
Comment 7 Maciej Stachowiak 2007-02-22 02:23:44 PST
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?
Comment 8 Maciej Stachowiak 2007-02-22 05:03:24 PST
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.
Comment 9 Antti Koivisto 2007-02-22 06:28:36 PST
Created attachment 13314 [details]
initiate scroll from insertedIntoDocument to cover dynamic cases too
Comment 10 Darin Adler 2007-02-22 08:23:09 PST
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.
Comment 11 Antti Koivisto 2007-02-22 11:17:18 PST
Created attachment 13321 [details]
updated patch

- fixed ChangeLog
- cleaned up testcase
- added tests for size=1 (MenuList) case too
- removed unnecessary = 0
Comment 12 Darin Adler 2007-02-22 11:34:46 PST
Comment on attachment 13321 [details]
updated patch

r=me
Comment 13 Antti Koivisto 2007-02-22 12:10:29 PST
r19797