Bug 50521

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:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Test case	none
proposed patch...	none
proposed patch v2	none
proposed patch v3	benjamin: review-
proposed patch v4	kenneth: review+, commit-queue: commit-queue-
screenshot before fix	none
screenshot after fix	none
Test case no <br>	none
screenshot without <br> before fix	none
screenshot without <br> after fix	none
correct screenshot without <br> after fix	none
Archive of layout-test-results from cr-jail-8	none
proposed patch v5	none

Dawit A.

Reported 2010-12-04 10:08:38 PST

Any button element that contains a <br> is not rendered properly in QtWebKit. Here is a simple example that demonstrate the issue: <html><body><button>Test<br>Button</button></body></html> The sample html above is rendered correctly in Chromium.

Attachments
Test case (58 bytes, text/html) 2010-12-06 07:34 PST, Benjamin Poulain	no flags	Details
proposed patch... (1.99 KB, patch) 2011-02-18 11:50 PST, Dawit A.	no flags	Details Formatted Diff Diff
proposed patch v2 (5.52 KB, patch) 2011-02-19 18:11 PST, Dawit A.	no flags	Details Formatted Diff Diff
proposed patch v3 (6.17 KB, patch) 2011-03-08 13:06 PST, Dawit A.	benjamin: review-	Details Formatted Diff Diff
proposed patch v4 (6.49 KB, patch) 2011-03-14 08:24 PDT, Dawit A.	kenneth: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
screenshot before fix (2.14 KB, image/png) 2011-03-30 09:53 PDT, Dawit A.	no flags	Details
screenshot after fix (2.41 KB, image/png) 2011-03-30 09:53 PDT, Dawit A.	no flags	Details
Test case no <br> (55 bytes, text/html) 2011-03-30 11:55 PDT, Dawit A.	no flags	Details
screenshot without <br> before fix (2.17 KB, image/png) 2011-03-30 11:56 PDT, Dawit A.	no flags	Details
screenshot without <br> after fix (2.41 KB, image/png) 2011-03-30 11:57 PDT, Dawit A.	no flags	Details
correct screenshot without <br> after fix (2.15 KB, image/png) 2011-03-30 11:59 PDT, Dawit A.	no flags	Details
Archive of layout-test-results from cr-jail-8 (51.87 KB, application/zip) 2011-04-14 20:20 PDT, WebKit Commit Bot	no flags	Details
proposed patch v5 (6.35 KB, patch) 2011-04-21 07:55 PDT, Dawit A.	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Benjamin Poulain

Comment 1 2010-12-06 07:34:15 PST

Created attachment 75695 [details] Test case

aravind.akella

Comment 2 2011-01-25 11:13:12 PST

Investigating

Benjamin Poulain

Comment 3 2011-01-29 12:30:18 PST

Could be related to #53373

aravind.akella

Comment 4 2011-02-14 08:46:10 PST

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)

Dawit A.

Comment 5 2011-02-14 11:05:27 PST

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

aravind.akella

Comment 6 2011-02-16 08:15:07 PST

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 ?

Dawit A.

Comment 7 2011-02-18 10:48:40 PST

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

Dawit A.

Comment 8 2011-02-18 11:26:48 PST

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 ?

Dawit A.

Comment 9 2011-02-18 11:50:53 PST

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.

aravind.akella

Comment 10 2011-02-18 12:33:45 PST

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.

Caio Marcelo de Oliveira Filho

Comment 11 2011-02-18 12:35:41 PST

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

Eric Seidel (no email)

Comment 12 2011-02-18 12:38:50 PST

Comment on attachment 82989 [details] proposed patch... How do we test this? Does Qt run pixel tests?

Dawit A.

Comment 13 2011-02-18 14:54:52 PST

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

Dawit A.

Comment 14 2011-02-19 18:11:06 PST

Created attachment 83086 [details] proposed patch v2 Added layout tests...

Antonio Gomes

Comment 15 2011-02-19 20:14:24 PST

Comment on attachment 83086 [details] proposed patch v2 The test will likely fail on all platforms other than Qt, due to font metrics difference.

Dawit A.

Comment 16 2011-02-21 09:20:51 PST

(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

Antonio Gomes

Comment 17 2011-02-21 13:35:32 PST

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

Dawit A.

Comment 18 2011-03-08 13:06:10 PST

Created attachment 85084 [details] proposed patch v3 Added changelog for the newly added layout files. Can someone please review this patch and commit it ?

Benjamin Poulain

Comment 19 2011-03-14 05:30:14 PDT

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?

Benjamin Poulain

Comment 20 2011-03-14 05:32:08 PDT

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.

Dawit A.

Comment 21 2011-03-14 07:56:02 PDT

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

Dawit A.

Comment 22 2011-03-14 07:57:26 PDT

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

Dawit A.

Comment 23 2011-03-14 08:24:29 PDT

Created attachment 85677 [details] proposed patch v4 updated changelog as requested.

Tor Arne Vestbø

Comment 24 2011-03-14 08:37:27 PDT

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

Dawit A.

Comment 25 2011-03-30 09:53:21 PDT

Created attachment 87553 [details] screenshot before fix

Dawit A.

Comment 26 2011-03-30 09:53:52 PDT

Created attachment 87555 [details] screenshot after fix

Kenneth Rohde Christiansen

Comment 27 2011-03-30 09:56:20 PDT

Your screenshot should show how it affects normal buttons without <br> Diego, you need to test this with the mobile theme.

Diego Gonzalez

Comment 28 2011-03-30 11:30:56 PDT

(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

Dawit A.

Comment 29 2011-03-30 11:55:58 PDT

Created attachment 87588 [details] Test case no <br>

Dawit A.

Comment 30 2011-03-30 11:56:41 PDT

Created attachment 87589 [details] screenshot without <br> before fix

Dawit A.

Comment 31 2011-03-30 11:57:34 PDT

Created attachment 87590 [details] screenshot without <br> after fix

Dawit A.

Comment 32 2011-03-30 11:59:10 PDT

Created attachment 87592 [details] correct screenshot without <br> after fix

Kenneth Rohde Christiansen

Comment 33 2011-03-30 12:00:03 PDT

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.

Kenneth Rohde Christiansen

Comment 34 2011-03-30 12:00:30 PDT

Ah the screenshot was wrong :-)

WebKit Commit Bot

Comment 35 2011-03-31 15:46:51 PDT

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

Dawit A.

Comment 36 2011-04-01 06:52:42 PDT

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

Dawit A.

Comment 37 2011-04-14 13:58:50 PDT

Comment on attachment 85677 [details] proposed patch v4 Let us try this again and see if the bot tests pass this time around...

Eric Seidel (no email)

Comment 38 2011-04-14 14:23:05 PDT

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

WebKit Commit Bot

Comment 39 2011-04-14 20:20:32 PDT

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

WebKit Commit Bot

Comment 40 2011-04-14 20:20:36 PDT

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

Dawit A.

Comment 41 2011-04-14 21:31:58 PDT

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

Dawit A.

Comment 42 2011-04-21 07:55:41 PDT

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.

WebKit Commit Bot

Comment 43 2011-04-26 21:29:30 PDT

Comment on attachment 90531 [details] proposed patch v5 Clearing flags on attachment: 90531 Committed r85009: <http://trac.webkit.org/changeset/85009>

WebKit Commit Bot

Comment 44 2011-04-26 21:29:38 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 45 2011-04-26 22:11:12 PDT

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

Csaba Osztrogonác

Comment 46 2011-04-26 23:54:32 PDT

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

Dawit A.

Comment 47 2011-04-27 06:41:51 PDT

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

Note You need to log in before you can comment on or make changes to this bug.