Bug 70765 – Default <select multiple> expands up to 10 items instead of showing 4

• Component: Forms
• Status: RESOLVED FIXED

[Jon Lee]

According to specs:
If applying those rules to the attribute's value is not successful, or if the size attribute is absent, then the element's display size is 4 if the element's multiple content attribute is present, and 1 otherwise.

Right now the select expands based on the number of items, and then caps out at 10.

[Radar WebKit Bug Importer]

<rdar://problem/10336453>

[Antaryami Pandia]

Created attachment 113026 [details]
Proposed Patch

Proposed patch

[Darin Adler]

Comment on attachment 113026 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113026&action=review

> Source/WebCore/rendering/RenderListBox.cpp:217
> +        return max(defaultSize, specifiedSize);

This change is incorrect. The default size and the minimum are not the same thing. We have a minimum of 4 because of the minimum size that is big enough so a scrollbar works. Platforms that do scrolling with different scrollbar designs might even want a different minimum. This is not the same number as the 4 in the HTML standard.

> Source/WebCore/rendering/RenderListBox.cpp:218
> +    return min(defaultSize, numItems());

This is not what the specification says. It says that the size should be 4 even if there is only 1 item. And the old code did that. You are removing that old minimum size of 4 here.

[Antaryami Pandia]

(In reply to comment #3)
> (From update of attachment 113026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113026&action=review
> 
> > Source/WebCore/rendering/RenderListBox.cpp:217
> > +        return max(defaultSize, specifiedSize);
> 
> This change is incorrect. The default size and the minimum are not the same thing. We have a minimum of 4 because of the minimum size that is big enough so a scrollbar works. Platforms that do scrolling with different scrollbar designs might even want a different minimum. This is not the same number as the 4 in the HTML standard.

Thanks for the clarification.

> > Source/WebCore/rendering/RenderListBox.cpp:218
> > +    return min(defaultSize, numItems());
> 
> This is not what the specification says. It says that the size should be 4 even if there is only 1 item. And the old code did that. You are removing that old minimum size of 4 here.

Yes.I will modify.

Will upload a modified patch after incorporating your review comments.

[Antaryami Pandia]

Created attachment 113134 [details]
Modified Patch

Patch.

[Jon Lee]

Comment on attachment 113134 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113134&action=review

> Source/WebCore/rendering/RenderListBox.cpp:73
> +// The minSize is a value that is big enough as to a scrollbar works.

For clarity I might rephrase this as "The minSize constant was originally defined to render scrollbars correctly."

> Source/WebCore/rendering/RenderListBox.cpp:74
> +// This might vary for different platform.

"platforms"

> Source/WebCore/rendering/RenderListBox.cpp:80
> +// IDL attribute is either 1 or 4 depending on the presence of the multiple attribute.

This comment is incorrect. The size IDL attribute is not a reflection of the actual size used. The link is also unnecessary. I would suggest just keeping the first line.

> Source/WebCore/rendering/RenderListBox.cpp:226
> +    return min(max(minSize, numItems()), defaultSize);

This evaluation is unnecessary. Because defaultSize and minSize are the same now, this will always evaluate to defaultSize. It's better to just return defaultSize.

> LayoutTests/fast/forms/select-multiple-attr-with-no-size.html:23
> +</html>

I'd like to see another test case (if it doesn't exist already) where the # of options is either 0, or fewer than the default 4 options.

> LayoutTests/fast/forms/select-multiple-attr-with-size-attr.html:23
> +</html>

I'd like to see another test case (if it doesn't exist already) where the size specified is less than the minimum size.

[Antaryami Pandia]

(In reply to comment #6)
> For clarity I might rephrase this as "The minSize constant was originally defined to render scrollbars correctly."
Done. 
> > Source/WebCore/rendering/RenderListBox.cpp:74
> > +// This might vary for different platform.
> 
> "platforms"
Done.
> > Source/WebCore/rendering/RenderListBox.cpp:80
> > +// IDL attribute is either 1 or 4 depending on the presence of the multiple attribute.
> 
> This comment is incorrect. The size IDL attribute is not a reflection of the actual size used. The link is also unnecessary. I would suggest just keeping the first line.
Done.
> > Source/WebCore/rendering/RenderListBox.cpp:226
> > +    return min(max(minSize, numItems()), defaultSize);
> 
> This evaluation is unnecessary. Because defaultSize and minSize are the same now, this will always evaluate to defaultSize. It's better to just return defaultSize.
Done.
> > LayoutTests/fast/forms/select-multiple-attr-with-no-size.html:23
> > +</html>
> 
> I'd like to see another test case (if it doesn't exist already) where the # of options is either 0, or fewer than the default 4 options.
Ok.
> > LayoutTests/fast/forms/select-multiple-attr-with-size-attr.html:23
> > +</html>
> 
> I'd like to see another test case (if it doesn't exist already) where the size specified is less than the minimum size.
Ok.

[Antaryami Pandia]

Created attachment 113152 [details]
Updated patch

Patch with Jon's review comments.

[Darin Adler]

Comment on attachment 113152 [details]
Updated patch

It would be easy to make a test that uses offsetHeight to figure out what size the list box is. That test could have plain text results and be cross-platform. New tests that require pixel test results are a lot more work.

[Antaryami Pandia]

(In reply to comment #9)
> (From update of attachment 113152 [details])
> It would be easy to make a test that uses offsetHeight to figure out what size the list box is. That test could have plain text results and be cross-platform. New tests that require pixel test results are a lot more work.

I have added the pixel tests, since the change affects visually i.e earlier it used to display 10 option, now it will display options according to the prsence of multiple attr.

Also are you talking of test case which will look like:-
1. With multiple attr and number of options are greater then the default (4)
<select multiple id="sel" >
    <option value="1">One</option>
    ------- ------ ----- ------
</select>
<script>
    var sel = document.getElementById('sel');
    shouldBeTrue('sel.scrollHeight > sel.clientHeight');
</script>
2. With multiple attr and number of options are less then the default (4)
<select multiple id="sel">
    <option value="1">One</option>
    <option value="2">Two</option>
    <option value="3">Three</option>
</select>
<script>
    var sel = document.getElementById('sel');
    shouldBeTrue('sel.scrollHeight == sel.clientHeight');
</script>

I am using clientHeight since it excluded border width.

On some test cases, I have seen usage of value 17 (I think thats the default font size). So can I use it for checking the space when we have less elements than the default.

[Antaryami Pandia]

Hi Darin,
Please provide your feedback on the test cases, I mentioned in my earlier comments.Is that what you intended?

[Darin Adler]

(In reply to comment #10)
> I have added the pixel tests, since the change affects visually i.e earlier it used to display 10 option, now it will display options according to the prsence of multiple attar.

Yes, but you don’t have to make a pixel test just to check if the height of the element changed.

> Also are you talking of test case which will look like:-
> 1. With multiple attr and number of options are greater then the default (4)
> <select multiple id="sel" >
>     <option value="1">One</option>
>     ------- ------ ----- ------
> </select>
> <script>
>     var sel = document.getElementById('sel');
>     shouldBeTrue('sel.scrollHeight > sel.clientHeight');
> </script>
> 2. With multiple attr and number of options are less then the default (4)
> <select multiple id="sel">
>     <option value="1">One</option>
>     <option value="2">Two</option>
>     <option value="3">Three</option>
> </select>
> <script>
>     var sel = document.getElementById('sel');
>     shouldBeTrue('sel.scrollHeight == sel.clientHeight');
> </script>
> 
> I am using clientHeight since it excluded border width.
> 
> On some test cases, I have seen usage of value 17 (I think thats the default font size). So can I use it for checking the space when we have less elements than the default.

Yes, something like that. I wasn't thinking of comparing scrollHeight to clientHeight. More comparing clientHeight of differently sized select elements. A single test could really easily cover all sorts of different combinations and expected heights and test them by checking the relationships between the heights.

You could even calibrate by checking the height of a single item one, and then make a function that does the math and returns the size in rows, and "error" if the size isn’t an even multiple of rows.

[Antaryami Pandia]

Created attachment 113633 [details]
Patch

Patch

[Darin Adler]

Comment on attachment 113633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113633&action=review

> LayoutTests/fast/forms/select-clientheight-with-multiple-attr-expected.txt:6
> +PASS clientHeight('sel2') is clientHeight('sel1')
> +PASS getElemById('sel2').setAttribute('size', '5'); clientHeight('sel2') > clientHeight('sel1') is true
> +PASS clientHeight('sel3') is clientHeight('sel1')
> +PASS getElemById('sel3').setAttribute('size', '2'); clientHeight('sel3') is clientHeight('sel1')

Good approach on this test now.

I think we should test more than just the fact that the height is larger when there are more items. We can compare to an exact multiple, and I think it would work fine on all platforms.

And, I think we should test a *lot* more than these two cases. We want to cover all the edge cases. Really giant sizes, really small sizes, missing size attribute, specified size attribute, size attribute that is a number but has garbage digits after it, size attribute that is the empty string.

[Antaryami Pandia]

(In reply to comment #14)
> I think we should test more than just the fact that the height is larger when there are more items. We can compare to an exact multiple, and I think it would work fine on all platforms.

By exact multiple, do you mean exact multiple of clientheight (with single element)? But with current behavior webkit doesn't return the actual clientheight with single option.This is because the use of minSize used to render scrollbar. With this the client height returned is always more than what is expected.

> And, I think we should test a *lot* more than these two cases. We want to cover all the edge cases. Really giant sizes, really small sizes, missing size attribute, specified size attribute, size attribute that is a number but has garbage digits after it, size attribute that is the empty string.

Yes, you are right.I will add more cases.

[Antaryami Pandia]

Created attachment 113997 [details]
Patch

Patch with added test cases.Added test cases to consider multiple of client height .Here I am considering multiple of 4 as its the minsize used for correct rendering of scrollbar.

[Darin Adler]

Comment on attachment 113997 [details]
Patch

It’s too to have more coverage.

[Darin Adler]

(In reply to comment #17)
> (From update of attachment 113997 [details])
> It’s too to have more coverage.

Not "too", "good".

[WebKit Review Bot]

Comment on attachment 113997 [details]
Patch

Rejecting attachment 113997 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
cts to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/fast/forms/listbox-clip-expected.txt
patching file LayoutTests/platform/mac/test_expectations.txt
patching file LayoutTests/platform/qt/test_expectations.txt
Hunk #1 succeeded at 16 (offset -8 lines).
patching file LayoutTests/platform/win/test_expectations.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10368047

[Antaryami Pandia]

Created attachment 114204 [details]
Patch for landing.

Patch with updated source code to avoid merge conflicts.

[WebKit Review Bot]

Comment on attachment 114204 [details]
Patch for landing.

Clearing flags on attachment: 114204

Committed r99653: <http://trac.webkit.org/changeset/99653>

[Ojan Vafai]

Comment on attachment 114204 [details]
Patch for landing.

View in context: https://bugs.webkit.org/attachment.cgi?id=114204&action=review

> LayoutTests/fast/forms/select-clientheight-large-size.html:54
> +    // Add large number of options
> +    for (i=0 ; i<= 10000 ;i++)
> +        AddItem(i, i);

Do we really need to test with 10000 options? It seems like 100 would be sufficient to cover all the edge cases. This test is timing out on all the chromium debug bots. Unless there are objections, I'll reduce the number of options to 100.

[Ojan Vafai]

Tracking fixing the test in bug 71880.

[Eric Seidel (no email)]

Comment on attachment 113633 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 113633 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 113997 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 113997 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Antaryami Pandia]

The patch has been landed.But the bug status is still showing "NEW". Request some one to resolve the issue.