Bug 29564 - [Qt] Webkit Button Size
: [Qt] Webkit Button Size
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
: Qt
:
:
  Show dependency treegraph
 
Reported: 2009-09-21 08:12 PST by
Modified: 2010-01-27 16:37 PST (History)


Attachments
Patch with test case (6.23 KB, patch)
2010-01-15 01:46 PST, Daniel Bates
vestbo: review-
Review Patch | Details | Formatted Diff | Diff
Self-contained layout test (2.55 KB, text/html)
2010-01-15 01:49 PST, Daniel Bates
no flags Details
Patch with test cases (7.93 KB, patch)
2010-01-15 10:25 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Patch with test case (7.99 KB, patch)
2010-01-15 10:28 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Patch with test case (7.99 KB, patch)
2010-01-15 10:41 PST, Daniel Bates
vestbo: review-
Review Patch | Details | Formatted Diff | Diff
Patch with test case (6.72 KB, patch)
2010-01-18 20:10 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff
Patch with test case (11.18 KB, patch)
2010-01-25 00:00 PST, Daniel Bates
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2009-09-21 08:12:13 PST
This bug report originated from Nokia internal issue QT-1730


--- Comments ---

Product
Qt

Function
ActiveQt

Version
4.5.2

Platform
Windows XP

Platform details
Both Linux & Windows XP

Compilers
MSVC

Compiler details
Buoth MSVC and Gcc

Subject
Webkit Button Size

Steps to reproduce / test case
Try this page

<html>
<head>
</head>
<body >
<button type="button" style="width:300px;height:250px;" value="bootone">botz</button>
</body>
</html>

Webkit display correctly button width, but height is fixed to 30 pixels.
------- Comment #1 From 2010-01-15 01:46:25 PST -------
Created an attachment (id=46656) [details]
Patch with test case

Notice, the Mac ports ignore the specified height for <input type="button"> elements unless a border, and/or background CSS property is also specified (because method RenderTheme::isControlStyled returns false on line 72 of RenderTheme.cpp <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTheme.cpp?rev=52432#L72>).
------- Comment #2 From 2010-01-15 01:49:23 PST -------
Created an attachment (id=46658) [details]
Self-contained layout test

For convenience, here is a self-contained version of the layout test included in the patch <https://bugs.webkit.org/attachment.cgi?id=46656>.
------- Comment #3 From 2010-01-15 02:30:54 PST -------
What is up with the self-contained layout test? It doesnt even open here
------- Comment #4 From 2010-01-15 02:54:36 PST -------
Cool stuff, thanks for looking into this Daniel!

It does not seem like we return anything for navigator.platform on Qt/Mac, so
the check expects 40, resulting in a failed layout-tests on Qt/Mac.

Also, I'm wondering if we should perhaps base these decisions in the
RenderTheme at runtime based on the current QStyle, rather then Q_WS_MAC, since
we might be running with -style windows, etc.
------- Comment #5 From 2010-01-15 02:58:35 PST -------
(From update of attachment 46656 [details])
sorry, r- as per the above comments

Won't the expected result with button5=18 fail when put into LayoutTests/fast/css/button-height-expected.txt ? I mean, it's only going to work for mac and possibly qt-mac, the other platforms, win, qt, etc should have a expected file with 40 no?
------- Comment #6 From 2010-01-15 10:25:59 PST -------
Created an attachment (id=46687) [details]
Patch with test cases

Updated patch based on IRC discussion last night with Tor Arne Vestbø and Kenneth Christiansen.

Patch also substitutes a runtime check for the macro Q_WS_MAC.

Now, that bug #33711 has been resolved we can use navigator.platform in the layout test.
------- Comment #7 From 2010-01-15 10:28:35 PST -------
Created an attachment (id=46688) [details]
Patch with test cases

Fixed lack of indent on line added to RenderThemeQt::RenderThemeQt.
------- Comment #8 From 2010-01-15 10:32:39 PST -------
Attachment 46688 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/qt/RenderThemeQt.cpp:412:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1
------- Comment #9 From 2010-01-15 10:41:34 PST -------
Created an attachment (id=46689) [details]
Patch with test case

Fix style error found by style bot.
------- Comment #10 From 2010-01-15 12:26:54 PST -------
Attachment 46689 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/190443
------- Comment #11 From 2010-01-15 12:29:34 PST -------
(From update of attachment 46689 [details])
Looks like QMacStyle cannot be found. Clearing review flag while I look into this.
------- Comment #12 From 2010-01-18 01:59:19 PST -------
(From update of attachment 46689 [details])
> +    if (qobject_cast<QMacStyle*>(qStyle()) {

There's a closing ) missing here
------- Comment #13 From 2010-01-18 04:26:54 PST -------
(From update of attachment 46689 [details])
Thanks for the updated patch Daniel!

> +#include <QMacStyle>

QMacStyle is only available on mac, so this has to be wrapped in Q_WS_MAC

>      QFont defaultButtonFont = QApplication::font(&button);
>      QFontInfo fontInfo(defaultButtonFont);
>      m_buttonFontFamily = defaultButtonFont.family();
> -#ifdef Q_WS_MAC
> -    m_buttonFontPixelSize = fontInfo.pixelSize();
> -#endif
> +    if (qobject_cast<QMacStyle*>(qStyle()))
> +        m_buttonFontPixelSize = fontInfo.pixelSize();
>  }

We can't reference the style in the ctor like this, it's too early. You could move the init code into adjustButtonSizes() where those two members are used and use static variables there to cache it.

the QMacStyle check also has to be wrapped in Q_WS_MAC like the include

> +    if (qobject_cast<QMacStyle*>(qStyle()) && style->appearance() == PushButtonPart) {

Same

> +    if (qobject_cast<QMacStyle*>(qStyle()) {

Same


> Index: LayoutTests/fast/css/button-height-expected.txt
> ===================================================================
> --- LayoutTests/fast/css/button-height-expected.txt	(revision 0)
> +++ LayoutTests/fast/css/button-height-expected.txt	(revision 0)
> @@ -0,0 +1,13 @@

This is a cross-platform expected file, which means it must be valid for mac, win, qt, gtk, qt-mac, etc if there are no overrides in platform/port-spesific results.

> +PASS document.getElementById('button1').offsetHeight is document.getElementById('button2').offsetHeight
> +PASS document.getElementById('button3').offsetHeight is 40
> +PASS document.getElementById('button4').offsetHeight is 40
> +PASS document.getElementById('button5').offsetHeight is 18

The hard-coded result of 18 here will not match for example qt, gtk, where the button will have a height of 40.

> +    shouldEvaluateTo("document.getElementById('button5').offsetHeight", 
> +        (navigator.platform.indexOf("Mac") !== -1) ? document.getElementById("button1").offsetHeight : 40);

This one is tricky. For Qt we run the DRT with -style windows on all platforms, which means that with the runtime check in RenderThemeQt we'll render the button 40 pixel high, but navigator.platform will still return MacIntel, so we'll expect the height to be button1.

I guess the easiest here would be to change this line into dumping the height of the button5, and then using the expected-file to detect failure, with the cross-platform expected file expecting 40, and then a platform-specific expected file with 18 in the mac results.

(or a number of platform-specific expected files with 40 instead of a shared cross-platform one) + the mac one with 18
------- Comment #14 From 2010-01-18 20:10:00 PST -------
Created an attachment (id=46882) [details]
Patch with test case

It is unclear to me how we benefit from using QMacStyle instead of the macro definition Q_WS_MAC since we need to use Q_WS_MAC anyway to conditionally #include <QMacStyle> and surround any code blocks that use QMacStyle. So, I have removed the changes related to QMacStyle from the patch. If we do decide to switch to using QMacStyle, I suggest we do this in a separate bug.

Moreover, I have updated the layout test and expected results so that we do not need platform-specific results. I chose to output a custom PASS/FAIL message when testing the fifth button in the test case instead of outputting the height of the button (which is platform-specific) so as to avoid the need for platform-specific results.
------- Comment #15 From 2010-01-20 01:33:32 PST -------
(In reply to comment #14)
> It is unclear to me how we benefit from using QMacStyle instead of the macro
> definition Q_WS_MAC since we need to use Q_WS_MAC anyway to conditionally
> #include <QMacStyle> and surround any code blocks that use QMacStyle.

QMacStyle is only available on Mac, so we have to ifdef it's use to make QtWebKit compile on other platforms. But on Mac, where the style is available, we can make a runtime check, to see if QtWebKit is running with the Mac style or one of the other styles (-style windows or -style plastique for example).

But that's okey, we can defer runtime stylechecks to a later patch. It has problems of its own since we don't report the runtime style back to JS or the DRT in any way, so layout-tests will still think they are running against "MacIntel" for example, even though the tests are running with the windows-style.
------- Comment #16 From 2010-01-20 18:31:46 PST -------
(From update of attachment 46882 [details])
Clearing flags on attachment: 46882

Committed r53591: <http://trac.webkit.org/changeset/53591>
------- Comment #17 From 2010-01-20 18:31:54 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2010-01-20 19:29:42 PST -------
This caused failures on the Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r53591%20(6284)/fast/replaced/table-percent-height-pretty-diff.html
------- Comment #19 From 2010-01-20 21:08:26 PST -------
Rolled out patch <https://bugs.webkit.org/attachment.cgi?id=46882> in change set 53601 <http://trac.webkit.org/changeset/53601> as it caused a regression of test /fast/replaced/table-percent-height.html on the Qt bot.

Need to look into this further.
------- Comment #20 From 2010-01-21 22:54:36 PST -------
I have confirmed that both the Apple and Chromium Windows ports fail the test <http://trac.webkit.org/export/53675/trunk/LayoutTests/fast/replaced/table-percent-height.html> (when run by hand) with respect to percentage button heights within a parent with an unspecified height. 

Note, the reason that the Apple Windows DRT bot does not fail the test is because it uses the Mac theme for controls instead of the native Windows theme, see bug #25592. Hence, it matches the Mac result, which sets the height of the button to the minimum height necessary to display the button label (by default - see comment in patch for more details).

Moreover, the Apple, Chromium, and Qt ports don't conform to the CSS 2.1 spec. with regards to the heights of buttons. We should file this as a separate bug.
------- Comment #21 From 2010-01-21 23:13:38 PST -------
I mean't to add that this patch brings Qt in line with the behavior on both the Mac ports and the Windows ports.

Since the Qt DRT bot does not use the Mac theme (like the Win DRT bot does), it fails fast/replaced/table-percent-height.html.

Although we should make all the ports adhere to the CSS 2.1 spec. with regards to button heights, I suggest we do this in a separate bug.

We have the following options: 1) Land this patch with expected failing results for Qt Win/Linux and file a follow-up bug. 2) Land this patch and skip the test on Qt Win/Linux.

I think landing this patch with expected failing results for the Qt Win/Linux ports is best, since the majority of the test cases in the test pass (4/44 tests fail).

I am open to thoughts on this matter.
------- Comment #22 From 2010-01-25 00:00:18 PST -------
Created an attachment (id=47320) [details]
Patch with test case

Updated patch to include expected failure results for the test /LayoutTests/fast/replaced/table-percent-height.html on the Qt port.

I suggest that we land this patch with expected failure results for now since this patch brings the Qt port in line with the functionality in both the Mac and Windows ports.

As aforementioned, I have confirmed that the other Windows ports also fail this test when run by hand. I filed bug #34071 about these failures and updated the test case table-percent-height.html (in this patch) to reference this bug number.
------- Comment #23 From 2010-01-26 21:29:25 PST -------
(From update of attachment 47320 [details])
Clearing flags on attachment: 47320

Committed r53897: <http://trac.webkit.org/changeset/53897>
------- Comment #24 From 2010-01-26 21:29:34 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #25 From 2010-01-27 16:24:24 PST -------
This test failed on Windows since it was created. Landed expected failing results for Windows in r53959.
------- Comment #26 From 2010-01-27 16:37:31 PST -------
Just want to clarify that we are landing expected failure results because of bug #25592. That is, Apple Windows DRT uses the Mac theme to render buttons instead of the native Windows theme. So, the button height under DRT is equivalent to the Mac button height. The test assumes that the theme used corresponds to the native theme for the platform. That is, if the platform is Windows, then controls are drawn using the Windows theme. Hence, the fifth button test in the test case fails because we determine the expected height of the button with respect to the platform instead of the theme. 

Note, the test case passes under the Apple Windows port when run manually. 

(In reply to comment #25)
> This test failed on Windows since it was created. Landed expected failing
> results for Windows in r53959.