Bug 20445 – HTML listbox is not resized horizontally when zooming

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Artem Ananiev]

HTML listbox is defined using <select multiple=".."> tag. When a page with listbox is zoomed in/out (Ctrl+'+' or Ctrl+'-' in Safari on Windows), the listbox font is changed, and listbox vertical size is adjusted. However, horizontal size always remains the same regardless of the font size, and no horizontal scrollbar appears. This leads to some options to be partially invisible in the listbox. If user later clicks on the listbox, it is expanded horizontally according to the current font size.

Steps to reproduce the problem:
1. Load the URL: http://www.htmlcodetutorial.com/forms/_SELECT_MULTIPLE.html
2. Find 'produces this' listbox on the page. It should contain 5 items, and listbox width is adjusted to the longest item.
3. Press Ctrl+'+'.
4. Listbox font is increased, listbox height is adjusted according to the font, but listbox width remains the same. The longest listbox item is truncated.
5. Click on the listbox. It is immediately enlarged, so its width again corresponds to the longest item.

If you see the described behavior, the bug is reproduced.

[Artem Ananiev]

Created attachment 22876 [details]
Patch

Set a flag to recalculate listbox width when its style is changed.

[Artem Ananiev]

I found the listbox width is calculated in RenderListBox::updateFromElement() method which first checks for a flag 'm_optionsChanged'. This flag is only set to true if any elements are added/removed from the listbox. However, when listbox style is changed, its width should be also recalculated, so the flag should also be set to true in RenderListBox::setStyle(). The corresponding patch is attached.

Another option may be to introduced a new field 'm_styleChanged', set it to true in setStyle() and check it in updateFromElement() as well as m_optionsChanged.

[Alexey Proskuryakov]

This patch was not marked for review, which is why it got overlooked. Please mark it for review if it's intended for landing.

Please see <http://webkit.org/coding/contributing.html> for more detail.

[Alexey Proskuryakov]

This sounds like a possible duplicate of bug 14629.

[Renata Hodovan]

Created attachment 202502 [details]
Patch without test case

The problem in this bug is still valid. You can check it under the URL what Artem linked.
I also will add a similar test case if you find the solution correct.

[Renata Hodovan]

*** Bug 14629 has been marked as a duplicate of this bug. ***

[Renata Hodovan]

Can anybody take a look at this? :)

[Simon Fraser (smfr)]

Comment on attachment 202502 [details]
Patch without test case

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

r- for lack of test case.

> Source/WebCore/html/HTMLSelectElement.cpp:95
> +    setOptionsChangedOnRenderer();

Does this need to happen if styleChange is NoDiff?

[Renata Hodovan]

Created attachment 204887 [details]
Proposed patch

[Renata Hodovan]

(In reply to comment #8)
> (From update of attachment 202502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202502&action=review
> 
> r- for lack of test case.
Test case is added already.

> > Source/WebCore/html/HTMLSelectElement.cpp:95
> > +    setOptionsChangedOnRenderer();
> 
> Does this need to happen if styleChange is NoDiff?

Yeah, right. We need this only if the style is changed.

[Build Bot]

Comment on attachment 204887 [details]
Proposed patch

Attachment 204887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/857559

New failing tests:
fast/transforms/listbox-zoom.html
fast/css/text-transform-select.html

[Build Bot]

Created attachment 204897 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204887 [details]
Proposed patch

Attachment 204887 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/906583

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 204903 [details]
Archive of layout-test-results from APPLE-EWS-1 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-1  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Build Bot]

Comment on attachment 204887 [details]
Proposed patch

Attachment 204887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/850477

New failing tests:
fast/transforms/listbox-zoom.html
fast/css/text-transform-select.html

[Build Bot]

Created attachment 204992 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Renata Hodovan]

Created attachment 205387 [details]
Proposed patch

Test expectation is moved to a platform specific directory.

[Build Bot]

Comment on attachment 205387 [details]
Proposed patch

Attachment 205387 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/965758

New failing tests:
fast/transforms/listbox-zoom.html
fast/css/text-transform-select.html

[Build Bot]

Created attachment 205398 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205387 [details]
Proposed patch

Attachment 205387 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/926618

New failing tests:
fast/transforms/listbox-zoom.html
fast/css/text-transform-select.html

[Build Bot]

Created attachment 205407 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205387 [details]
Proposed patch

Attachment 205387 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/869805

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 205416 [details]
Archive of layout-test-results from APPLE-EWS-7 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-7  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Renata Hodovan]

Created attachment 206973 [details]
Proposed patch

[Renata Hodovan]

(In reply to comment #8)
> (From update of attachment 202502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202502&action=review
> 
> r- for lack of test case.
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:95
> > +    setOptionsChangedOnRenderer();
> 
> Does this need to happen if styleChange is NoDiff?

I missed the answer before. As the failing fast/css/text-transform-select test showed we need to set m_optionChanged even if the change is NoChange unless we lose the start style settings.

[Build Bot]

Comment on attachment 206973 [details]
Proposed patch

Attachment 206973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1096450

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 206978 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Renata Hodovan]

Okay, it seems the only problem that makes mac ews red is the missing platform specific expected what will be added by the gardeners (at least IMO, 'cos I don't have mac build).

[Build Bot]

Comment on attachment 206973 [details]
Proposed patch

Attachment 206973 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1106576

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 206990 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Renata Hodovan]

Can anybody take a look at this? :)

[Renata Hodovan]

Is this fix right already?

[Renata Hodovan]

Could somebody review it please? :)

[Ryosuke Niwa]

Comment on attachment 206973 [details]
Proposed patch

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

It looks sensible.

> LayoutTests/fast/transforms/listbox-zoom.html:7
> +	<script>
> +	function runTest() {
> +		document.getElementById('toZoom').style.zoom = 2;
> +	}
> +	</script>

Could you add a description and make it a dump-as-text test?

[Renata Hodovan]

(In reply to comment #34)
> (From update of attachment 206973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206973&action=review
> 
> It looks sensible.
> 
> > LayoutTests/fast/transforms/listbox-zoom.html:7
> > +	<script>
> > +	function runTest() {
> > +		document.getElementById('toZoom').style.zoom = 2;
> > +	}
> > +	</script>
> 
> Could you add a description and make it a dump-as-text test?

Sure, thanks for checking it!

[Renata Hodovan]

Created attachment 213087 [details]
Proposed patch

This patch updates the StyleChange parameter to Style::Change and makes the test to result a text dump.
The reference value of the listboxs width in the test is set to 55 (width 10 allowance). This value was checked on nix and qt too, they resulted 55.5 and 56 values respectively, while these values were about 30 without the patch. This way I think it's able to catch this failure.

[Darin Adler]

Comment on attachment 213087 [details]
Proposed patch

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

I’m going to say review+ but I have some serious reservations about this patch.

> Source/WebCore/ChangeLog:9
> +        If any style changes happens on a HTMLSelectElement, we need to set the m_optionsChanged property
> +        of its renderer (RenderListBox) unless its size won't follow the changed content.

The word "unless" needs to be replaced by the word "otherwise".

> Source/WebCore/html/HTMLSelectElement.cpp:91
> +    setOptionsChangedOnRenderer();

Is it really true that *any* style change needs to do this? I think this is not correct. While the change works, it seems likely it is needlessly inefficient.

This code is non obvious, because the function name "setOptionsChangedOnRenderer" is telling a lie. We are saying that the options changed, but the options did not change. It’s worth considering changing names to make this clear. Or at the very least we need a comment that says something like:

    // Even though the options didn’t necessarily change, we will call setOptionsChangedOnRenderer for its side effect
    // of recomputing the width of the element. We need to do that if the style change included a change in zoom level.

I’m not sure if the comment I wrote is accurate. We should make a comment in that style that correctly explains the reason for this line of code.

> Source/WebCore/html/HTMLSelectElement.h:110
> +    virtual void didRecalcStyle(Style::Change);

Should be marked OVERRIDE and FINAL, and should be private rather than protected. Unless there’s a reason that one of those is incorrect.

[Build Bot]

Comment on attachment 213087 [details]
Proposed patch

Attachment 213087 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2904144

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 213093 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213087 [details]
Proposed patch

Attachment 213087 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2908122

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 213094 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213087 [details]
Proposed patch

Attachment 213087 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2959003

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 213099 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213087 [details]
Proposed patch

Attachment 213087 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2905174

New failing tests:
fast/transforms/listbox-zoom.html

[Build Bot]

Created attachment 213100 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Renata Hodovan]

(In reply to comment #37)
> (From update of attachment 213087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213087&action=review
> 
> I’m going to say review+ but I have some serious reservations about this patch.

Thank you for the review. Your requests are fulfilled on the patch. Furthermore the test is modified a bit with removing the actual width value from the expected what made the mac ews red. I'll land it soon...

[Renata Hodovan]

Committed r156766: <http://trac.webkit.org/changeset/156766>

[Simon Fraser (smfr)]

(In reply to comment #46)
> (In reply to comment #37)
> > (From update of attachment 213087 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213087&action=review
> > 
> > I’m going to say review+ but I have some serious reservations about this patch.
> 
> Thank you for the review. Your requests are fulfilled on the patch.

I'm not sure they are. I still think that HTMLSelectElement::didRecalcStyle() is doing needless work in many cases, which may hurt performance on pages with lots of selects. Have you measured the perf impact?

[Renata Hodovan]

(In reply to comment #48)
> (In reply to comment #46)
> > (In reply to comment #37)
> > > (From update of attachment 213087 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=213087&action=review
> > > 
> > > I’m going to say review+ but I have some serious reservations about this patch.
> > 
> > Thank you for the review. Your requests are fulfilled on the patch.
> 
> I'm not sure they are. I still think that HTMLSelectElement::didRecalcStyle() is doing needless work in many cases, which may hurt performance on pages with lots of selects. Have you measured the perf impact?

Not yet. Could you suggest a benchmark or method to measure this possible performance loss?

[Simon Fraser (smfr)]

Make a  page with 1000 <select> with 100 options each. Change a CSS class that affects a trivial style (e.g. background color) on each. Measure perf with and without your patch.