Bug 50521

Summary: [Qt] Improper rendering of <button> tag when it contains a <br>
Product: WebKit Reporter: Dawit A. <adawit>
Component: WebKit QtAssignee: 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 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

Description Dawit A. 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.
Comment 1 Benjamin Poulain 2010-12-06 07:34:15 PST
Created attachment 75695 [details]
Test case
Comment 2 aravind.akella 2011-01-25 11:13:12 PST
Investigating
Comment 3 Benjamin Poulain 2011-01-29 12:30:18 PST
Could be related to #53373
Comment 4 aravind.akella 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)
Comment 5 Dawit A. 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.
Comment 6 aravind.akella 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 ?
Comment 7 Dawit A. 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 ?
Comment 8 Dawit A. 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 ?
Comment 9 Dawit A. 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.
Comment 10 aravind.akella 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.
Comment 11 Caio Marcelo de Oliveira Filho 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.
Comment 12 Eric Seidel (no email) 2011-02-18 12:38:50 PST
Comment on attachment 82989 [details]
proposed patch...

How do we test this?  Does Qt run pixel tests?
Comment 13 Dawit A. 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.
Comment 14 Dawit A. 2011-02-19 18:11:06 PST
Created attachment 83086 [details]
proposed patch v2

Added layout tests...
Comment 15 Antonio Gomes 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.
Comment 16 Dawit A. 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
Comment 17 Antonio Gomes 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 ...
Comment 18 Dawit A. 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 ?
Comment 19 Benjamin Poulain 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?
Comment 20 Benjamin Poulain 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.
Comment 21 Dawit A. 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.
Comment 22 Dawit A. 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.
Comment 23 Dawit A. 2011-03-14 08:24:29 PDT
Created attachment 85677 [details]
proposed patch v4

updated changelog as requested.
Comment 24 Tor Arne Vestbø 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.
Comment 25 Dawit A. 2011-03-30 09:53:21 PDT
Created attachment 87553 [details]
screenshot before fix
Comment 26 Dawit A. 2011-03-30 09:53:52 PDT
Created attachment 87555 [details]
screenshot after fix
Comment 27 Kenneth Rohde Christiansen 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.
Comment 28 Diego Gonzalez 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
Comment 29 Dawit A. 2011-03-30 11:55:58 PDT
Created attachment 87588 [details]
Test case no <br>
Comment 30 Dawit A. 2011-03-30 11:56:41 PDT
Created attachment 87589 [details]
screenshot without <br> before fix
Comment 31 Dawit A. 2011-03-30 11:57:34 PDT
Created attachment 87590 [details]
screenshot without <br> after fix
Comment 32 Dawit A. 2011-03-30 11:59:10 PDT
Created attachment 87592 [details]
correct screenshot without <br> after fix
Comment 33 Kenneth Rohde Christiansen 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.
Comment 34 Kenneth Rohde Christiansen 2011-03-30 12:00:30 PDT
Ah the screenshot was wrong :-)
Comment 35 WebKit Commit Bot 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
Comment 36 Dawit A. 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...
Comment 37 Dawit A. 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...
Comment 38 Eric Seidel (no email) 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. :)
Comment 39 WebKit Commit Bot 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
Comment 40 WebKit Commit Bot 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
Comment 41 Dawit A. 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.
Comment 42 Dawit A. 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.
Comment 43 WebKit Commit Bot 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>
Comment 44 WebKit Commit Bot 2011-04-26 21:29:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 WebKit Review Bot 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
Comment 46 Csaba Osztrogonác 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
Comment 47 Dawit A. 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.