Summary: | [Qt] Improper rendering of <button> tag when it contains a <br> | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dawit A. <adawit> | ||||||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, aravind.akella, benjamin, cmarcelo, commit-queue, diegohcg, eric, kenneth, ossy, vestbo, webkit.review.bot | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Dawit A.
2010-12-04 10:08:38 PST
Created attachment 75695 [details]
Test case
Investigating Could be related to #53373 This bug is not reproducible on symbian or meego because the QtTestBrowser uses qt_mobile_theme on these devices. This bug occurs only on a linux machine. The content size of a push button is set to the size of a single line ... WebCore/platform/qt/RenderThemeQt.cpp +481 QSize contentSize = fm.size(Qt::TextShowMnemonic, QString::fromLatin1("X")); So anything beyond a single line is getting clipped. Do you want me to fix this ?(considering that it doesn't occur on symbian/meego) (In reply to comment #4) > This bug is not reproducible on symbian or meego because the QtTestBrowser uses qt_mobile_theme on these devices. This bug occurs only on a linux machine. Linux machines only ? You mean all the Unix desktops where QtWebKit is in use correct ? > The content size of a push button is set to the size of a single line ... > WebCore/platform/qt/RenderThemeQt.cpp +481 > QSize contentSize = fm.size(Qt::TextShowMnemonic, QString::fromLatin1("X")); Yes, I saw that but I could not figure out how to get hold of the appropriate content size from the element to avoid the clipping. Otherwise, I would have offered a patch with the bug report as usual... > So anything beyond a single line is getting clipped. Do you want me to fix this ?(considering that it doesn't occur on symbian/meego) Yes. This is a rather glaring bug for all of us that are pushing/using QtWebKit on a *nix desktop environment. Yes, by linux machines I mean linux desktops running QtTestBrowser. RenderThemeQt.cpp .. RenderThemeQt::adjustButtonStyle(..,Element *e) calls the setSizeMethod which in turn calls the computeSizeBasedOnStyle(). If we can count the number of <br>s in the <button> element, we can set the content size to a string with an equal number of newlines('\n'). For .. <Button> line1 <br> line2 </Button> We can set QSize contentSize = fm.size(Qt::TextShowMnemonic,QString::fromLatin1("X \n X")); But if the button element contains <b> or <i> tags the actual size may be a little bigger than the computedSize. Will a solution along these lines be acceptable ? (In reply to comment #6) > Yes, by linux machines I mean linux desktops running QtTestBrowser. I sitll do not understand why this issue is not relevant on the mobile platform because of the use of the qt_mobile_theme... Is that because the particular theme used does haved fixed size for the button height ? IOW, why would this only be issue on Linux machines ? Due the specific styles used on them ? > RenderThemeQt.cpp .. > RenderThemeQt::adjustButtonStyle(..,Element *e) calls the setSizeMethod which in turn calls the computeSizeBasedOnStyle(). > > If we can count the number of <br>s in the <button> element, we can set the content size to a string with an equal number of newlines('\n'). > > For .. > <Button> > line1 <br> > line2 </Button> > We can set > QSize contentSize = fm.size(Qt::TextShowMnemonic,QString::fromLatin1("X \n X")); > But if the button element contains <b> or <i> tags the actual size may be a little > bigger than the computedSize. Will a solution along these lines be acceptable ? Hmm... Well that is not really a solution, but a hack then. Also simply counting the number of new lines will breakdown if the html is <button><br>Test<br></button> which should be translated into "\nTest\n" and not "Test\n\n". I think the real resolution is fixing the issue at the source. If this problem does not occur with the use of qt_mobile_theme, then we need to find out exactly what is being done in that theme that allows it to properly render buttons with multi line text. Is this platform specific styling issue, i.e. the style used in KDE (oxygen) has effect on how the button gets rendered ? BTW, changing if (renderStyle->height().isAuto() && size.height() > 0) renderStyle->setHeight(Length(size.height(), Fixed)); in RenderThemeQt::computeSizeBasedOnStyle to if (renderStyle->height().isAuto() && size.height() > 0) renderStyle->setMinHeight(Length(size.height(), Fixed)); seems to partially fix the problem. At least noting gets clipped with that, but the padding around the buttons is not correct either. The question I have here is since the size is being computed based on a single letter "X", at least for buttons, why is hard code its width and height instead of only using them as a minimum size ? Created attachment 82989 [details]
proposed patch...
Do not hard code the width/height of the render style in RenderThemeQt::computeSizeBasedOnStyle. Instead set the calculated values as minimum width and height values.
Set the original values of the bottom and top margin variables in RenderThemeQt::setButtonPadding to the same value as the right and left ones. No idea why one is set to 1 and the other to 0.
No idea what kind of impact this has on the mobile platforms with their qt_mobile_theme, but these changes definitely fix the problem with multi-line text buttons on Linux desktops.
One more problem.. <Button>line1 line2</Button> line1 & line2 should be rendered on the same line. But it is being rendered on different lines even though there is no <br> separating them. (In reply to comment #10) > One more problem.. > <Button>line1 > line2</Button> > > line1 & line2 should be rendered on the same line. But it is being > rendered on different lines even though there is no <br> separating them. This is different issue, reported in bug 53373. I think I have a fix for that. Comment on attachment 82989 [details]
proposed patch...
How do we test this? Does Qt run pixel tests?
(In reply to comment #12) > (From update of attachment 82989 [details]) > How do we test this? Does Qt run pixel tests? (In reply to comment #12) > (From update of attachment 82989 [details]) > How do we test this? Does Qt run pixel tests? Pehaps a layout test similar to the ones for testing whitespace inside of buttons ? LayoutTests/fast/forms/button-white-space.html. As far as I am concerned, the test from this bug report can even be added into the above test file since I consider HTML line breaks and whitespaces to be in the same category. Created attachment 83086 [details]
proposed patch v2
Added layout tests...
Comment on attachment 83086 [details]
proposed patch v2
The test will likely fail on all platforms other than Qt, due to font metrics difference.
(In reply to comment #15) > (From update of attachment 83086 [details]) > The test will likely fail on all platforms other than Qt, due to font metrics difference. Well then would not the same issue be true for the whitespace layout test this one is based on ? LayoutTests/fast/forms/button-white-space.html LayoutTests/platform/qt/fast/forms/button-white-space-expected.txt (In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 83086 [details] [details]) > > The test will likely fail on all platforms other than Qt, due to font metrics difference. > > Well then would not the same issue be true for the whitespace layout test this one is based on ? > > LayoutTests/fast/forms/button-white-space.html > LayoutTests/platform/qt/fast/forms/button-white-space-expected.txt I did not notice the "/platform/qt" part. Ignore me ... Created attachment 85084 [details]
proposed patch v3
Added changelog for the newly added layout files. Can someone please review this patch and commit it ?
Comment on attachment 85084 [details] proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=85084&action=review > Source/WebCore/ChangeLog:7 > + Please explain your change here. This change is not trivial, even if short. Explaining what is wrong and how your change fix it is useful for future reference when modifying this file. > Source/WebCore/platform/qt/RenderThemeQt.cpp:516 > // FIXME: Check is flawed, since it doesn't take min-width/max-width into account. Is this FIXME still valid with your change? Tor Arne, you were the original author. Do you remember why you hardcoded paddingTop and paddingBottom to 1 and 0 respectively? Dawit, did you generated the layout test results with the VM? The text metrics can be wrong otherwise. (In reply to comment #19) > (From update of attachment 85084 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85084&action=review > > > Source/WebCore/ChangeLog:7 > > + > > Please explain your change here. This change is not trivial, even if short. Explaining what is wrong and how your change fix it is useful for future reference when modifying this file. > > Source/WebCore/platform/qt/RenderThemeQt.cpp:516 > > // FIXME: Check is flawed, since it doesn't take min-width/max-width into account. > > Is this FIXME still valid with your change? Well, I am not sure what that FIXME pretains to exactly so I cannot say for sure if it does or not. However, hardcoding the width and the height causes text to be clipped in the desktop environment. (In reply to comment #20) > Tor Arne, you were the original author. Do you remember why you hardcoded paddingTop and paddingBottom to 1 and 0 respectively? > > Dawit, did you generated the layout test results with the VM? The text metrics can be wrong otherwise. No, it was not generated with the VM. Created attachment 85677 [details]
proposed patch v4
updated changelog as requested.
(In reply to comment #20) > Tor Arne, you were the original author. Do you remember why you hardcoded paddingTop and paddingBottom to 1 and 0 respectively? Don't remember the specifics, sorry :-/ Likely it matched the styles at that time, on mac and linux, but things might have changed since then. RenderTheme is a monster. Created attachment 87553 [details]
screenshot before fix
Created attachment 87555 [details]
screenshot after fix
Your screenshot should show how it affects normal buttons without <br> Diego, you need to test this with the mobile theme. (In reply to comment #27) > Your screenshot should show how it affects normal buttons without <br> > > Diego, you need to test this with the mobile theme. It's happens with mobile theme as well. The fix will useful for us Created attachment 87588 [details]
Test case no <br>
Created attachment 87589 [details]
screenshot without <br> before fix
Created attachment 87590 [details]
screenshot without <br> after fix
Created attachment 87592 [details]
correct screenshot without <br> after fix
In chrome and safari I get the behavior for a button without <br> as before the fix. Ie, the fix can be considered a regression for that case. Ah the screenshot was wrong :-) Comment on attachment 85677 [details] proposed patch v4 Rejecting attachment 85677 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 726.54s total testing time 23153 test cases (99%) succeeded 1 test case (<1%) was new 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8316045 (In reply to comment #35) > (From update of attachment 85677 [details]) > Rejecting attachment 85677 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Makes no sense. Patch was rejected because the build bot failed to apply the patch or because it failed to run tests ? Nothing related to this patch is in the given full output link... Comment on attachment 85677 [details]
proposed patch v4
Let us try this again and see if the bot tests pass this time around...
(In reply to comment #36) > (In reply to comment #35) > > (From update of attachment 85677 [details] [details]) > > Rejecting attachment 85677 [details] [details] from commit-queue. > > > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 > > Makes no sense. Patch was rejected because the build bot failed to apply the patch or because it failed to run tests ? Nothing related to this patch is in the given full output link... You're adding a test w/o adding results for mac. :) Comment on attachment 85677 [details] proposed patch v4 Rejecting attachment 85677 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: mlmp . http/tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 761.47s total testing time 23279 test cases (99%) succeeded 1 test case (<1%) was new 15 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8450014 Created attachment 89724 [details]
Archive of layout-test-results from cr-jail-8
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-8 Port: Mac Platform: Mac OS X 10.6.6
(In reply to comment #38) > (In reply to comment #36) > > (In reply to comment #35) > > > (From update of attachment 85677 [details] [details] [details]) > > > Rejecting attachment 85677 [details] [details] [details] from commit-queue. > > > > > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 > > > > Makes no sense. Patch was rejected because the build bot failed to apply the patch or because it failed to run tests ? Nothing related to this patch is in the given full output link... > > You're adding a test w/o adding results for mac. :) Hmm... I see, but then how does that get resolved ? I sure do not have a Mac. There does not seem to be a platform specific place to put the test .html file either. The only thing I see in the platform specific sections is the .txt files that contain the expected results. Created attachment 90531 [details]
proposed patch v5
Moved the layout test html file to the platform specific folder for qt in order not to crap out the bot that runs the mac tests.
Comment on attachment 90531 [details] proposed patch v5 Clearing flags on attachment: 90531 Committed r85009: <http://trac.webkit.org/changeset/85009> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/85009 might have broken Qt Linux Release The following tests are not passing: editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/3690719.html editing/selection/5240265.html fast/block/float/float-avoidance.html fast/block/positioning/inline-block-relposition.html fast/css/continuationCrash.html fast/css/margin-top-bottom-dynamic.html fast/dom/HTMLTableColElement/resize-table-using-col-width.html fast/dynamic/positioned-movement-with-positioned-children.html fast/forms/basic-buttons.html fast/forms/basic-selects.html fast/forms/blankbuttons.html fast/forms/button-align.html fast/forms/button-cannot-be-nested.html fast/forms/button-generated-content.html fast/forms/button-inner-block-reuse.html fast/forms/button-positioned.html fast/forms/button-sizes.html fast/forms/button-style-color.html fast/forms/button-table-styles.html fast/forms/button-text-transform.html fast/forms/control-clip-overflow.html fast/forms/control-restrict-line-height.html fast/forms/formmove3.html fast/forms/input-button-sizes.html fast/forms/select-baseline.html fast/forms/targeted-frame-submission.html fast/replaced/replaced-breaking.html fast/replaced/table-percent-height.html fast/replaced/width100percent-button.html fast/selectors/064.html fast/table/append-cells2.html fast/table/remove-td-display-none.html fast/text/textIteratorNilRenderer.html http/tests/navigation/javascriptlink-frames.html platform/qt/fast/forms/button-line-break.html svg/custom/inline-svg-in-xhtml.xml tables/mozilla/bugs/bug1188.html tables/mozilla/bugs/bug1318.html tables/mozilla/bugs/bug138725.html tables/mozilla/bugs/bug18359.html tables/mozilla/bugs/bug2479-2.html tables/mozilla/bugs/bug2479-3.html tables/mozilla/bugs/bug2479-4.html tables/mozilla/bugs/bug26178.html tables/mozilla/bugs/bug28928.html tables/mozilla/bugs/bug33855.html tables/mozilla/bugs/bug39209.html tables/mozilla/bugs/bug4429.html tables/mozilla/bugs/bug44505.html tables/mozilla/bugs/bug46368-1.html tables/mozilla/bugs/bug46368-2.html tables/mozilla/bugs/bug51037.html tables/mozilla/bugs/bug51727.html tables/mozilla/bugs/bug52505.html tables/mozilla/bugs/bug52506.html tables/mozilla/bugs/bug60749.html tables/mozilla/bugs/bug68912.html tables/mozilla/bugs/bug7342.html tables/mozilla/collapsing_borders/bug41262-4.html tables/mozilla/dom/tableDom.html tables/mozilla/other/move_row.html tables/mozilla_expected_failures/bugs/bug1725.html tables/mozilla_expected_failures/bugs/bug58402-2.html tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html tables/mozilla_expected_failures/collapsing_borders/bug41262-6.html Hey men, you should have run layout tests before committing ... and update results, of course ... :S Now I did it instead of you: http://trac.webkit.org/changeset/85025 (In reply to comment #46) > Hey men, you should have run layout tests before committing > ... and update results, of course ... :S > > Now I did it instead of you: http://trac.webkit.org/changeset/85025 First, thanks but I do not understand how adding a single platform specific layout test file and its expected result can break all of the ones listed in your fix. Anyhow, thanks. |