Summary: | REGRESSION(NativeListBox): List not scrolled to preselected option | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Lilek <dev+webkit> | ||||||||||
Component: | Forms | Assignee: | Antti Koivisto <koivisto> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | KwhiteRight, mitz, sam | ||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Matt Lilek
2006-10-28 21:36:00 PDT
Created attachment 11272 [details]
testcase
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. Are we sure this is a bug? Firefox exhibits the same behavior as ToT. (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. 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).
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 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.
Created attachment 13314 [details]
initiate scroll from insertedIntoDocument to cover dynamic cases too
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.
Created attachment 13321 [details]
updated patch
- fixed ChangeLog
- cleaned up testcase
- added tests for size=1 (MenuList) case too
- removed unnecessary = 0
Comment on attachment 13321 [details]
updated patch
r=me
|