Bug 56752

Summary: [Qt] The minimum size of the select menu list is incorrect for qtwebkit
Product: WebKit Reporter: Doreen Jiang <doreen.jiang>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, benjamin, commit-queue, cshu, laszlo.gombos, ossy
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: S60 3rd edition   
URL: http://travelocity.com
Bug Depends on:    
Bug Blocks: 50925    
Attachments:
Description Flags
Fixes the returned width of characters for selection boxes
benjamin: review-
latest patch with testcase
benjamin: review+, benjamin: commit-queue-
updated patch with rebaselined failing testcases
none
Updated patch with the updated new test case
benjamin: review-
Updated new test case
benjamin: review+, commit-queue: commit-queue-
Updated Patch with rebaselined tests none

Description Doreen Jiang 2011-03-21 10:10:25 PDT
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.
Comment 1 Doreen Jiang 2011-03-21 12:53:24 PDT
Created attachment 86354 [details]
Fixes the returned width of characters for selection boxes
Comment 2 Doreen Jiang 2011-03-21 12:55:06 PDT
Patch submitted for review and commit
Comment 3 Doreen Jiang 2011-03-21 12:56:23 PDT
This was tested on QtTestBrowser and the behavior was confirmed.
Comment 4 Benjamin Poulain 2011-03-22 05:42:11 PDT
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.
Comment 5 Benjamin Poulain 2011-03-22 05:51:21 PDT
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.
Comment 6 Doreen Jiang 2011-03-30 06:41:01 PDT
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.
Comment 7 Doreen Jiang 2011-03-30 06:42:59 PDT
reopening the bug to submit the patch
Comment 8 Doreen Jiang 2011-03-30 07:11:09 PDT
Created attachment 87529 [details]
latest patch with testcase

Attached the latest patch with testcase included and updated with correct bug description
Comment 9 Benjamin Poulain 2011-03-30 07:26:35 PDT
(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.
Comment 10 Benjamin Poulain 2011-03-30 07:27:23 PDT
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.
Comment 11 Doreen Jiang 2011-03-30 07:49:15 PDT
> 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.
Comment 12 Benjamin Poulain 2011-03-30 08:18:23 PDT
(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.
Comment 13 Doreen Jiang 2011-04-01 08:43:33 PDT
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.
Comment 14 Benjamin Poulain 2011-04-01 08:50:52 PDT
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.
Comment 15 Doreen Jiang 2011-04-01 09:11:39 PDT
(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?
Comment 16 Doreen Jiang 2011-04-01 12:48:32 PDT
Created attachment 87900 [details]
Updated patch with the updated new test case

Ben, I updated the new test case according to your comment #14.
Comment 17 Benjamin Poulain 2011-04-06 05:52:26 PDT
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?
Comment 18 Doreen Jiang 2011-04-06 09:16:39 PDT
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
Comment 19 Benjamin Poulain 2011-04-06 09:25:47 PDT
(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.
Comment 20 Doreen Jiang 2011-04-11 12:04:16 PDT
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)
Comment 21 Doreen Jiang 2011-04-13 12:47:03 PDT
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.
Comment 22 Benjamin Poulain 2011-04-18 06:53:07 PDT
Comment on attachment 89437 [details]
Updated new test case

Great. I let you cq+ when you have time to make the bots happy.
Comment 23 WebKit Commit Bot 2011-05-31 14:03:15 PDT
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
Comment 24 Doreen Jiang 2011-06-03 08:17:59 PDT
Created attachment 95915 [details]
Updated Patch with rebaselined tests
Comment 25 WebKit Commit Bot 2011-06-03 10:03:36 PDT
Comment on attachment 95915 [details]
Updated Patch with rebaselined tests

Clearing flags on attachment: 95915

Committed r88029: <http://trac.webkit.org/changeset/88029>
Comment 26 Chang Shu 2011-06-03 11:45:49 PDT
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.
Comment 27 Ademar Reis 2011-06-03 14:11:57 PDT
Revision r88029 cherry-picked into qtwebkit-2.2 with commit 2e981d3 <http://gitorious.org/webkit/qtwebkit/commit/2e981d3>