Bug 76801 - Listboxes incorrectly display contents when cleared and then re-populated
Summary: Listboxes incorrectly display contents when cleared and then re-populated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://patorjk.com/browser-issues/lis...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-22 12:26 PST by Patrick Gillespie
Modified: 2012-01-31 21:38 PST (History)
5 users (show)

See Also:


Attachments
ProposedPatch (1.92 KB, patch)
2012-01-23 18:01 PST, Joe Thomas
no flags Details | Formatted Diff | Diff
PatchWithTestCase (5.10 KB, patch)
2012-01-27 10:49 PST, Joe Thomas
no flags Details | Formatted Diff | Diff
patch3 (5.02 KB, patch)
2012-01-30 15:12 PST, Joe Thomas
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch4 (5.01 KB, patch)
2012-01-31 05:42 PST, Joe Thomas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Gillespie 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.
Comment 1 Alexey Proskuryakov 2012-01-22 16:26:38 PST
Confirmed with Safari 5.1.
Comment 2 Joe Thomas 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.
Comment 3 Joe Thomas 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.
Comment 4 Alexey Proskuryakov 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.
Comment 5 Joe Thomas 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>
Comment 6 Alexey Proskuryakov 2012-01-26 09:04:36 PST
You can simulate a click in scroll bar with eventSender (grep for it in LayoutTests/fast directory).
Comment 7 Alexey Proskuryakov 2012-01-26 09:05:49 PST
See also: <http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree>.
Comment 8 Joe Thomas 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?
Comment 9 Alexey Proskuryakov 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.
Comment 10 Joe Thomas 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();
    } 
}
Comment 11 Alexey Proskuryakov 2012-01-26 14:22:24 PST
I'm not sure why this doesn't work.
Comment 12 Joe Thomas 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?
Comment 13 Alexey Proskuryakov 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?
Comment 14 Joe Thomas 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.
Comment 15 Joe Thomas 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.
Comment 16 Alexey Proskuryakov 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.
Comment 17 Joe Thomas 2012-01-27 10:49:07 PST
Created attachment 124335 [details]
PatchWithTestCase

Added testcase
Comment 18 Alexey Proskuryakov 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).
Comment 19 Joe Thomas 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.
Comment 20 Alexey Proskuryakov 2012-01-27 13:59:04 PST
scrollToYOffsetWithoutAnimation() is not really a notification. Something like contentsResized() is.
Comment 21 Simon Fraser (smfr) 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.
Comment 22 Joe Thomas 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.
Comment 23 Alexey Proskuryakov 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).
Comment 24 Simon Fraser (smfr) 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?
Comment 25 Joe Thomas 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.
Comment 26 Joe Thomas 2012-01-30 15:12:31 PST
Created attachment 124609 [details]
patch3

Modified patch with review comments incorporated.
Comment 27 Simon Fraser (smfr) 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.
Comment 28 Joe Thomas 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
Comment 29 Joe Thomas 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.
Comment 30 WebKit Review Bot 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.
Comment 31 Joe Thomas 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
Comment 32 Joe Thomas 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.
Comment 33 WebKit Review Bot 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>
Comment 34 WebKit Review Bot 2012-01-31 21:38:11 PST
All reviewed patches have been landed.  Closing bug.