Bug 56752 – [Qt] The minimum size of the select menu list is incorrect for qtwebkit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Doreen Jiang]

STEPS TO REPRODUCE:

1. Load the url www.iphone.travelocity.com

2.Select the Book Hotel icon.

3. Enter Boston in the city Field and select the Search Button.

4. Select Boston Logan international Airport from the city list.

5. Select any options from the Minors selection list in Hotel Search Input page

ACTUAL RESULTS:

The overlap of "Minor ages" selection lists is not proper.

[Doreen Jiang]

Created attachment 86354 [details]
Fixes the returned width of characters for selection boxes

[Doreen Jiang]

Patch submitted for review and commit

[Doreen Jiang]

This was tested on QtTestBrowser and the behavior was confirmed.

[Benjamin Poulain]

Comment on attachment 86354 [details]
Fixes the returned width of characters for selection boxes

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

R- because:
-change not justified. You do not explain why your code is correct, you just said it fixes a particular website
-no tests

Note that you forgot the review flag.

> Source/WebCore/ChangeLog:12
> +	https://bugs.webkit.org/show_bug.cgi?id=56752
> +
> +	The method RenderThemeQt::minimumMenuListSize returns a width that too wide for
> +	each character. This leaves extra spaces in a selection list and makes
> +	selection lists overlap each other in web pages.
> +
> +	Change minimumMenuListSize to return the size (fm.width*1) instead of (fm.witdh*7).

Indent is wrong.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:437
> +    return 1 * fm.width(QLatin1Char('x'));

You could just return fm.width(QLatin1Char('x'));, the "1 * " does not add clarity.

[Benjamin Poulain]

Closing as invalid.

The standard do not define the minimal width for those input. The current values are sensible for general use.

You should be using the mobile style IMHO.

[Doreen Jiang]

The root cause of the issue is about hardcoded value of 7*(size of one character) to render a menu list instead of actual size of the menu list.

So, i have modified RederThemeQt.cpp to use the minimal size of one character instead of 7 characters.

Here we are matching the rendering behavior of Chrome, Safari, Firefox and IE.
For each of these browsers, the width of the select-box is calculated to be as small as possible;
just wide enough to hold the widest entry.
So it seems to make sense to match this rendering behavior, especially since the current implementation
(a hard minimum of seven characters) causes us to misrender a popular sites.

[Doreen Jiang]

reopening the bug to submit the patch

[Doreen Jiang]

Created attachment 87529 [details]
latest patch with testcase

Attached the latest patch with testcase included and updated with correct bug description

[Benjamin Poulain]

(In reply to comment #6)
> Here we are matching the rendering behavior of Chrome, Safari, Firefox and IE.
> For each of these browsers, the width of the select-box is calculated to be as small as possible;
> just wide enough to hold the widest entry.
> So it seems to make sense to match this rendering behavior, especially since the current implementation
> (a hard minimum of seven characters) causes us to misrender a popular sites.

Fair enough. With some explanation the thing make sense

Did you generate the test result with the VM? Otherwise the metrics are likely wrong for the bots.

[Benjamin Poulain]

Comment on attachment 87529 [details]
latest patch with testcase

I cq- for now. I'll cq+ if you confirm the layout test results are using the right metrics.

[Doreen Jiang]

> Did you generate the test result with the VM? Otherwise the metrics are likely wrong for the bots.

I ran the test on mac only.

[Benjamin Poulain]

(In reply to comment #11)
> > Did you generate the test result with the VM? Otherwise the metrics are likely wrong for the bots.
> 
> I ran the test on mac only.

So it's good I cq- if you took the test lightly.

Maybe start by checking if you should not simply update existing layout test results. If not you should just make new results for you test with the right metrics.

[Doreen Jiang]

Created attachment 87862 [details]
updated patch with rebaselined failing testcases

Ben, The attached patch does not contain any additional code changes after your review but i rebaselined the failing testcases of select-list with reference to this change on VM. I make sure my change is not causing any other test case failures.

[Benjamin Poulain]

Comment on attachment 87862 [details]
updated patch with rebaselined failing testcases

> Ben, The attached patch does not contain any additional code changes after your review but i rebaselined the failing testcases of select-list with reference to this change on VM. I make sure my change is not causing any other test case failures.

What value does the test add over existing tests? For me, it looks like the case where it adds coverage over the other tests is 
<select id="list3">
  <option></option>
</select> 

So I would put that one first to make thing clear. I would also add a <p>paragraph</p> at the top of the test to explain what it tests. Finally, it should probably be commited in the common tests and not as a Qt specific one.

[Doreen Jiang]

(In reply to comment #14)

This change is only apply to Qt code and testcases. Can you reply to confirm that so that I can make change to the new test case based on your comment?

[Doreen Jiang]

Created attachment 87900 [details]
Updated patch with the updated new test case

Ben, I updated the new test case according to your comment #14.

[Benjamin Poulain]

Comment on attachment 87900 [details]
Updated patch with the updated new test case

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

r- because of the test being Qt specific without justification for that.
If a layout issue is not covered by existing test, you should improve the tests for everyone, not just for Qt.

> Source/WebCore/ChangeLog:5
> +        [QT]The minimum size of the select menu list is incorrect for qtwebkit 

Qt, not QT.

> LayoutTests/ChangeLog:5
> +        [QT]The minimum size of the select menu list is incorrect for qtwebkit 

Qt

> LayoutTests/platform/qt/fast/forms/selectlist-minsize.html:5
> +            [QT]The minimum size of the select menu list is incorrect for qtwebkit <br>

Qt

> LayoutTests/platform/qt/fast/forms/selectlist-minsize.html:9
> +        <select id="list3">

Why is there an id?

[Doreen Jiang]

Hi Ben,
I found a testcase "fast/replaced/three-selects-break.html" which compare the Render tree for similar usecase of blank select list and it will be baselined with my fix. So i think the new layouttest is not needed anymore.
Kindly confirm, Accordingly i will submit the updated patch with baselined test-cases & integrating your comments.

Thanks in Advance...
-Doreen

[Benjamin Poulain]

(In reply to comment #18)
> I found a testcase "fast/replaced/three-selects-break.html" which compare the Render tree for similar usecase of blank select list and it will be baselined with my fix. So i think the new layouttest is not needed anymore.
> Kindly confirm, Accordingly i will submit the updated patch with baselined test-cases & integrating your comments.

This is not stricto sensu the same test. Your test has an empty option, "fast/replaced/three-selects-break.html" only has empty selects.

[Doreen Jiang]

Hi Ben,
This issue is not reproducible for any other ports and the changes are qt specific (RenderThemeQt.cpp)

My only doubt is that RenderMenuList/RenderBlock sizes for each port(mac, qt, gtk etc) will change. So, you want to create the expected files for each of the above ports?

But my understanding is that, this usecase works fine for all other webkit ports but for qt we are using a different minimal width. So i think the layout test is needed for Qt port only as the code changes are specific to Qt port.

Correct me if i am wrong...

I will wait for your comments to create the layout test (qt specific / for all ports)

[Doreen Jiang]

Created attachment 89437 [details]
Updated new test case

Ben, I updated the new test case and made it generic. This test case was executed on both Mac and VM.  I also made corrections according to your comment #17.

[Benjamin Poulain]

Comment on attachment 89437 [details]
Updated new test case

Great. I let you cq+ when you have time to make the bots happy.

[WebKit Commit Bot]

Comment on attachment 89437 [details]
Updated new test case

Rejecting attachment 89437 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
ected.txt
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/tables/mozilla/bugs/bug29326-expected.txt.rej
patching file LayoutTests/platform/qt/tables/mozilla/core/margins-expected.txt
Hunk #1 FAILED at 11.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/tables/mozilla/core/margins-expected.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Benjamin Poulain', u'-..." exit_code: 1

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

[Doreen Jiang]

Created attachment 95915 [details]
Updated Patch with rebaselined tests

[WebKit Commit Bot]

Comment on attachment 95915 [details]
Updated Patch with rebaselined tests

Clearing flags on attachment: 95915

Committed r88029: <http://trac.webkit.org/changeset/88029>

[Chang Shu]

Comment on attachment 86354 [details]
Fixes the returned width of characters for selection boxes

Doreen, please mark the old ones obsolete when you upload a new patch.

[Ademar Reis]

Revision r88029 cherry-picked into qtwebkit-2.2 with commit 2e981d3 <http://gitorious.org/webkit/qtwebkit/commit/2e981d3>