WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
56752
[Qt] The minimum size of the select menu list is incorrect for qtwebkit
https://bugs.webkit.org/show_bug.cgi?id=56752
Summary
[Qt] The minimum size of the select menu list is incorrect for qtwebkit
Doreen Jiang
Reported
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.
Attachments
Fixes the returned width of characters for selection boxes
(1.49 KB, patch)
2011-03-21 12:53 PDT
,
Doreen Jiang
benjamin
: review-
Details
Formatted Diff
Diff
latest patch with testcase
(4.14 KB, patch)
2011-03-30 07:11 PDT
,
Doreen Jiang
benjamin
: review+
benjamin
: commit-queue-
Details
Formatted Diff
Diff
updated patch with rebaselined failing testcases
(30.45 KB, patch)
2011-04-01 08:43 PDT
,
Doreen Jiang
no flags
Details
Formatted Diff
Diff
Updated patch with the updated new test case
(30.62 KB, patch)
2011-04-01 12:48 PDT
,
Doreen Jiang
benjamin
: review-
Details
Formatted Diff
Diff
Updated new test case
(30.45 KB, patch)
2011-04-13 12:47 PDT
,
Doreen Jiang
benjamin
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Updated Patch with rebaselined tests
(54.10 KB, patch)
2011-06-03 08:17 PDT
,
Doreen Jiang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Doreen Jiang
Comment 1
2011-03-21 12:53:24 PDT
Created
attachment 86354
[details]
Fixes the returned width of characters for selection boxes
Doreen Jiang
Comment 2
2011-03-21 12:55:06 PDT
Patch submitted for review and commit
Doreen Jiang
Comment 3
2011-03-21 12:56:23 PDT
This was tested on QtTestBrowser and the behavior was confirmed.
Benjamin Poulain
Comment 4
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.
Benjamin Poulain
Comment 5
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.
Doreen Jiang
Comment 6
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.
Doreen Jiang
Comment 7
2011-03-30 06:42:59 PDT
reopening the bug to submit the patch
Doreen Jiang
Comment 8
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
Benjamin Poulain
Comment 9
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.
Benjamin Poulain
Comment 10
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.
Doreen Jiang
Comment 11
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.
Benjamin Poulain
Comment 12
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.
Doreen Jiang
Comment 13
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.
Benjamin Poulain
Comment 14
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.
Doreen Jiang
Comment 15
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?
Doreen Jiang
Comment 16
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
.
Benjamin Poulain
Comment 17
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?
Doreen Jiang
Comment 18
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
Benjamin Poulain
Comment 19
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.
Doreen Jiang
Comment 20
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)
Doreen Jiang
Comment 21
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
.
Benjamin Poulain
Comment 22
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.
WebKit Commit Bot
Comment 23
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
Doreen Jiang
Comment 24
2011-06-03 08:17:59 PDT
Created
attachment 95915
[details]
Updated Patch with rebaselined tests
WebKit Commit Bot
Comment 25
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
>
Chang Shu
Comment 26
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.
Ademar Reis
Comment 27
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug