Bug 76801

Summary: Listboxes incorrectly display contents when cleared and then re-populated
Product: WebKit Reporter: Patrick Gillespie <patorjk>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, joethomas, simon.fraser, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://patorjk.com/browser-issues/list-clear-restore.htm
Attachments:
Description Flags
ProposedPatch
none
PatchWithTestCase
none
patch3
simon.fraser: review+, simon.fraser: commit-queue-
Patch4 none

Patrick Gillespie
Reported 2012-01-22 12:26:08 PST
If a user scrolls to the bottom of the list, and then the list is then cleared and re-populated, the listbox will incorrectly display the listbox contents. I created a simple test case here: http://patorjk.com/browser-issues/list-clear-restore.htm You populate a list, scroll to the bottom, clear it, and then re-populate it, and you will see incorrect results (both FireFox and IE9 do not have the problem - though they each handle the issue differently). Instead of the bottom of the list, the top of the list is displayed, even though the scroll bar indicates that you are at the bottom of the list.
Attachments
ProposedPatch (1.92 KB, patch)
2012-01-23 18:01 PST, Joe Thomas 		no flags
DetailsFormatted DiffDiff
PatchWithTestCase (5.10 KB, patch)
2012-01-27 10:49 PST, Joe Thomas 		no flags
DetailsFormatted DiffDiff
patch3 (5.02 KB, patch)
2012-01-30 15:12 PST, Joe Thomas 		simon.fraser: review+
simon.fraser: commit-queue-
DetailsFormatted DiffDiff
Patch4 (5.01 KB, patch)
2012-01-31 05:42 PST, Joe Thomas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2012-01-22 16:26:38 PST
Confirmed with Safari 5.1.
Joe Thomas
Comment 2 2012-01-23 18:01:54 PST
Created attachment 123682 [details] ProposedPatch I have not added any test cases as I am not sure what kind of regression test can added for this.
Joe Thomas
Comment 3 2012-01-25 22:04:45 PST
> I have not added any test cases as I am not sure what kind of regression test can added for this. As far as I understand, it is not possible to access the scrollbar of Select box from javascript. A test can be written for this only if we get the scroll-offset of the Select box scrollBar. I think this bug can be verified only visually. Please let me know if there is any other alternative to write a test for this.
Alexey Proskuryakov
Comment 4 2012-01-25 22:25:45 PST
This can likely be verified with a pixel test, or better yet, a reftest. For a reftest, one provides two HTML files, one of which uses a different way to achieve an identical look.
Joe Thomas
Comment 5 2012-01-26 00:34:54 PST
(In reply to comment #4) > This can likely be verified with a pixel test, or better yet, a reftest. > > For a reftest, one provides two HTML files, one of which uses a different way to achieve an identical look. Thanks Alexey! I am trying to verify it with reftest. But I am not able to reproduce the original problem with the script I have written. The test is given below. I tried document.getElementById("myList").selectedIndex = 99, to move to the bottom of the list with 100 elements. This does move to the bottom, but after I clear and repopulate the content, the issue described in the bug is not seen. I think I need to scroll to the bottom of the list without doing a selection as there is no selection involved in the original steps to reproduce the issue. <!DOCTYPE> <html> <body onload="test()"> <select id="myList" size="10" multiple></select> <script> function populate() { var myList = document.getElementById("myList"), item, ii; for (ii = 0; ii < 100; ii++) { item = document.createElement("option"); item.value = ii; item.appendChild(document.createTextNode("Item #" + ii)); myList.appendChild(item); } } function clear() { var myList = document.getElementById("myList"), items = myList.getElementsByTagName("option"), ii; for (ii = items.length-1; ii >= 0; ii--) { myList.removeChild(items[ii]); } } function test() { populate(); document.getElementById("myList").selectedIndex = 99; clear(); populate(); } </script> </body> </html>
Alexey Proskuryakov
Comment 6 2012-01-26 09:04:36 PST
You can simulate a click in scroll bar with eventSender (grep for it in LayoutTests/fast directory).
Alexey Proskuryakov
Comment 7 2012-01-26 09:05:49 PST
See also: <http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree>.
Joe Thomas
Comment 8 2012-01-26 11:02:47 PST
(In reply to comment #6) > You can simulate a click in scroll bar with eventSender (grep for it in LayoutTests/fast directory). Thanks. window.eventSender is defined only when LayoutTest is run. Is is possible to enable eventSender outside the test-suite so that I can run my test case alone and verify that the scrolling happened?
Alexey Proskuryakov
Comment 9 2012-01-26 11:25:09 PST
It won't work outside run-webkit-tests. But you can run the test alone by passing its (relative) path to run-webkit-tests, and checking the tool's output.
Joe Thomas
Comment 10 2012-01-26 13:56:43 PST
I tried eventSender to send the mouse events. I could not get the scrolling working with it, but just the listbox is getting focused. function test() { var index; populate(); var element = document.getElementById("myList"); var bottomX = element.offsetLeft + element.offsetWidth; var bottomY = element.offsetTop + element.offsetHeight; eventSender.mouseMoveTo(bottomX - 5, bottomY - 5); for(index = 0; index < 50; index++) { eventSender.mouseDown(); eventSender.mouseUp(); } }
Alexey Proskuryakov
Comment 11 2012-01-26 14:22:24 PST
I'm not sure why this doesn't work.
Joe Thomas
Comment 12 2012-01-26 15:39:53 PST
Looks like Scrollbar for Select element will not work with eventSender. Please see the bug https://bugs.webkit.org/show_bug.cgi?id=53628 whether we encountered the same issue. Should I submit a manual test for this like bug 53628?
Alexey Proskuryakov
Comment 13 2012-01-26 15:58:24 PST
Hmm... I don't know this code all that well, but I doubt that this is the most elegant approach. We shouldn't have to scroll - ScrollableArea should reset scroll position on its own when its content shrinks. Why doesn't that happen? Also, you say that this works after a programmatic scroll (so it's difficult to make a test), but not after a manual one. What causes the difference between these cases?
Joe Thomas
Comment 14 2012-01-26 22:47:32 PST
(In reply to comment #13) > Hmm... I don't know this code all that well, but I doubt that this is the most elegant approach. We shouldn't have to scroll - ScrollableArea should reset scroll position on its own when its content shrinks. Why doesn't that happen? > ScrollableArea is the base class of RenderListBox and it gets the scroll-offset from RenderListBox (using ScrollableArea::scrollToYOffsetWithoutAnimation) In the given use case, when the list-items get removed, we disable the scroll bar in RenderListBox::computeLogicalHeight but we never updates the scroll offset with ScrollableArea. In the patch, I am calling scrollToYOffsetWithoutAnimation(0) after disabling the scrollbar, so apart from updating the offsets at ScrollableArea, it may not initiate any painting(not verified). > Also, you say that this works after a programmatic scroll (so it's difficult to make a test), but not after a manual one. What causes the difference between these cases? I did some more trials and this works in programmatic scroll only if we execute all the below steps in one go. (1) populate(); (2) document.getElementById("myList").selectedIndex = 99; (3) clear(); (4) populate(); Step 2 schedules a layout and the selectedIndex gets updated with ScrollableArea when RenderListBox::Layout gets called. But before layout happens, step 3 and 4 gets executed which sets scrollOffset of HTMLSelectElement back to -1. So ScrollableArea never gets updated with the selectedIndex. If we break the above steps into 2 groups (first two steps in one group) and execute them separately, then the issue will be reproducible.
Joe Thomas
Comment 15 2012-01-27 08:13:44 PST
I could make a programmatic test case by adding Zero timeout after setting the selectedIndex. I will update patch soon.
Alexey Proskuryakov
Comment 16 2012-01-27 10:22:50 PST
Yes, you can add a timeout, or you can also force layout by calling a method that depends on it having been done, e.g. document.offsetTop.
Joe Thomas
Comment 17 2012-01-27 10:49:07 PST
Created attachment 124335 [details] PatchWithTestCase Added testcase
Alexey Proskuryakov
Comment 18 2012-01-27 10:58:39 PST
Comment on attachment 124335 [details] PatchWithTestCase Thanks for adding a test case. I'd like to leave final review for someone else, as I have some doubts about the fix (discussed above).
Joe Thomas
Comment 19 2012-01-27 13:39:13 PST
Listbox should notify the ScrollableArea every time the scroll offset gets changed. This notification was not sent to ScrollableArea when listbox items are cleared and the scrollbar is disabled. The patch sends this additional notification to ScrollableArea.
Alexey Proskuryakov
Comment 20 2012-01-27 13:59:04 PST
scrollToYOffsetWithoutAnimation() is not really a notification. Something like contentsResized() is.
Simon Fraser (smfr)
Comment 21 2012-01-30 12:38:33 PST
Comment on attachment 124335 [details] PatchWithTestCase View in context: https://bugs.webkit.org/attachment.cgi?id=124335&action=review You need pixel results for the new tests (run-webkit-tests --pixel). r- for lack of pixel results. > Source/WebCore/rendering/RenderListBox.cpp:258 > + ScrollableArea::scrollToYOffsetWithoutAnimation(0); Does the call to scrollToYOffsetWithoutAnimation() have to be prefixed with the class name? Are you specifically calling the base class implementation? > LayoutTests/fast/forms/listbox-clear-restore-expected.html:10 > + var myList = document.getElementById("myList"), > + item, > + ii; We don't declare variables like this normally. > LayoutTests/fast/forms/listbox-clear-restore-expected.html:12 > + for (ii = 0; ii < 100; ii++) { ii could be a loop variable for (var ii ... > LayoutTests/fast/forms/listbox-clear-restore.html:12 > + var myList = document.getElementById("myList"), > + item, > + ii; > + > + for (ii = 0; ii < 100; ii++) { Ditto.
Joe Thomas
Comment 22 2012-01-30 13:35:40 PST
(In reply to comment #21) > (From update of attachment 124335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124335&action=review > Thanks for the review. > You need pixel results for the new tests (run-webkit-tests --pixel). r- for lack of pixel results. > I added a reftest for this defect as per Alexey's suggestion in comment #4. I am new to webkit. How do I get pixel results for this? I tried running run-webkit-tests --pixel, but it did not generate any pixel results. Could you please point me to a similar defect which I can refer to? > > Source/WebCore/rendering/RenderListBox.cpp:258 > > + ScrollableArea::scrollToYOffsetWithoutAnimation(0); > > Does the call to scrollToYOffsetWithoutAnimation() have to be prefixed with the class name? Are you specifically calling the base class implementation? > It is not needed. scrollToYOffsetWithoutAnimation has got implementation only in the base class. I added it because I saw similar usage in RenderListBox. I will remove this.
Alexey Proskuryakov
Comment 23 2012-01-30 13:51:54 PST
You don't need pixel results with a reftest. It would be however nice to remove fast/forms/listbox-clear-restore-expected.html from WebCore/ChangeLog, as it's not one of the tests (there is probably a bug about improving prepare-ChangeLog to ignore retest results).
Simon Fraser (smfr)
Comment 24 2012-01-30 14:27:15 PST
Comment on attachment 124335 [details] PatchWithTestCase View in context: https://bugs.webkit.org/attachment.cgi?id=124335&action=review r=me but please simplify the expected.html if possible. > LayoutTests/fast/forms/listbox-clear-restore-expected.html:4 > +<select id="myList" size="10" multiple></select> Why does the expected result need any JS to run? Can't it just hardcode the final state of the list box?
Joe Thomas
Comment 25 2012-01-30 14:52:07 PST
(In reply to comment #24) > (From update of attachment 124335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124335&action=review > > r=me but please simplify the expected.html if possible. > > > LayoutTests/fast/forms/listbox-clear-restore-expected.html:4 > > +<select id="myList" size="10" multiple></select> > > Why does the expected result need any JS to run? Can't it just hardcode the final state of the list box? The height of the scrollbar will be different if the number of elements in the listbox is different in test-html and expected-html and this fails the test. I will reduce the number of elements in the listbox to 20 from 100 in both test and expected.
Joe Thomas
Comment 26 2012-01-30 15:12:31 PST
Created attachment 124609 [details] patch3 Modified patch with review comments incorporated.
Simon Fraser (smfr)
Comment 27 2012-01-31 00:19:46 PST
Comment on attachment 124609 [details] patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=124609&action=review > LayoutTests/fast/forms/listbox-clear-restore.html:36 > +function clearTimeout() There is already a window.clearTimeout(). You should give this a different name. > LayoutTests/fast/forms/listbox-clear-restore.html:45 > + if (window.layoutTestController) > + layoutTestController.waitUntilDone(); Bad indentation here.
Joe Thomas
Comment 28 2012-01-31 05:42:14 PST
Created attachment 124713 [details] Patch4 With review comments incorporated. Changed the function names in the test case clearTimeout -> clearListTimeout populateTimeout -> populateListTimeout populate -> populateList clear -> clearList
Joe Thomas
Comment 29 2012-01-31 05:43:03 PST
(In reply to comment #27) > (From update of attachment 124609 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124609&action=review > > > LayoutTests/fast/forms/listbox-clear-restore.html:36 > > +function clearTimeout() > > There is already a window.clearTimeout(). You should give this a different name. > Done > > LayoutTests/fast/forms/listbox-clear-restore.html:45 > > + if (window.layoutTestController) > > + layoutTestController.waitUntilDone(); > > Bad indentation here. Corrected.
WebKit Review Bot
Comment 30 2012-01-31 05:45:36 PST
Attachment 124713 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Joe Thomas
Comment 31 2012-01-31 06:22:45 PST
(In reply to comment #30) > Attachment 124713 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 > > Updating OpenSource > First, rewinding head to replay your work on top of it... > Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. > Using index info to reconstruct a base tree... > Falling back to patching base and 3-way merge... > Auto-merging LayoutTests/ChangeLog > CONFLICT (content): Merge conflict in LayoutTests/ChangeLog > Auto-merging LayoutTests/platform/qt/Skipped > CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped > Auto-merging Source/WebCore/ChangeLog > CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog > Failed to merge in the changes. > Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. > > When you have resolved this problem run "git rebase --continue". > If you would prefer to skip this patch, instead run "git rebase --skip". > To restore the original branch and stop rebasing run "git rebase --abort". > > rebase refs/remotes/origin/master: command returned error: 1 > > Died at Tools/Scripts/update-webkit line 164. > > > If any of these errors are false positives, please file a bug against check-webkit-style. Not sure the reason for style-check failure. I will re-submit the patch created from an updated webkit source
Joe Thomas
Comment 32 2012-01-31 06:36:10 PST
> Not sure the reason for style-check failure. I will re-submit the patch created from an updated webkit source Looks like the style-check is failing with the same reason for other recent commits(bug 77199, bug 77408). Please let me know if I need to do something here.
WebKit Review Bot
Comment 33 2012-01-31 21:38:05 PST
Comment on attachment 124713 [details] Patch4 Clearing flags on attachment: 124713 Committed r106430: <http://trac.webkit.org/changeset/106430>
WebKit Review Bot
Comment 34 2012-01-31 21:38:11 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.