Bug 56752

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

Patch submitted for review and commit

This was tested on QtTestBrowser and the behavior was confirmed.

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.

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.

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.

reopening the bug to submit the patch

Created attachment 87529 [details] latest patch with testcase Attached the latest patch with testcase included and updated with correct bug description

(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 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.

> 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.

(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.

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 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.

(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?

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 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?

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

(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.

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)

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 on attachment 89437 [details] Updated new test case Great. I let you cq+ when you have time to make the bots happy.

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

Created attachment 95915 [details] Updated Patch with rebaselined tests

Comment on attachment 95915 [details] Updated Patch with rebaselined tests Clearing flags on attachment: 95915 Committed r88029: <http://trac.webkit.org/changeset/88029>

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.

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