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.
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.
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.
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.
(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.
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
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
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
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
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
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
(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.
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
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).
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
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?
(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!
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.
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.
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
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
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
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
(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...
(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?
(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?
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.
2008-08-19 01:51 PDT, Artem Ananiev
2013-05-21 23:57 PDT, Renata Hodovan
rhodovan.u-szeged: commit-queue-
2013-06-18 00:47 PDT, Renata Hodovan
2013-06-18 04:05 PDT, Build Bot
2013-06-18 05:16 PDT, Build Bot
2013-06-19 05:17 PDT, Build Bot
2013-06-25 05:48 PDT, Renata Hodovan
2013-06-25 06:35 PDT, Build Bot
2013-06-25 08:58 PDT, Build Bot
2013-06-25 12:21 PDT, Build Bot
2013-07-18 02:43 PDT, Renata Hodovan
2013-07-18 03:40 PDT, Build Bot
2013-07-18 07:31 PDT, Build Bot
2013-10-01 09:02 PDT, Renata Hodovan
darin: commit-queue-
2013-10-01 10:13 PDT, Build Bot
2013-10-01 10:13 PDT, Build Bot
2013-10-01 10:51 PDT, Build Bot
2013-10-01 11:13 PDT, Build Bot