Bug 29564 - [Qt] Webkit Button Size
Summary: [Qt] Webkit Button Size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-09-21 08:12 PDT by Tor Arne Vestbø
Modified: 2010-01-27 16:37 PST (History)
9 users (show)

See Also:


Attachments
Patch with test case (6.23 KB, patch)
2010-01-15 01:46 PST, Daniel Bates
vestbo: review-
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 Details | Formatted Diff | Diff
Patch with test case (7.99 KB, patch)
2010-01-15 10:28 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test case (7.99 KB, patch)
2010-01-15 10:41 PST, Daniel Bates
vestbo: review-
Details | Formatted Diff | Diff
Patch with test case (6.72 KB, patch)
2010-01-18 20:10 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch with test case (11.18 KB, patch)
2010-01-25 00:00 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-09-21 08:12:13 PDT
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 Daniel Bates 2010-01-15 01:46:25 PST
Created attachment 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 Daniel Bates 2010-01-15 01:49:23 PST
Created attachment 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 Kenneth Rohde Christiansen 2010-01-15 02:30:54 PST
What is up with the self-contained layout test? It doesnt even open here
Comment 4 Tor Arne Vestbø 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 Tor Arne Vestbø 2010-01-15 02:58:35 PST
Comment on attachment 46656 [details]
Patch with test case

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 Daniel Bates 2010-01-15 10:25:59 PST
Created attachment 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 Daniel Bates 2010-01-15 10:28:35 PST
Created attachment 46688 [details]
Patch with test case

Fixed lack of indent on line added to RenderThemeQt::RenderThemeQt.
Comment 8 WebKit Review Bot 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 Daniel Bates 2010-01-15 10:41:34 PST
Created attachment 46689 [details]
Patch with test case

Fix style error found by style bot.
Comment 10 WebKit Review Bot 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 Daniel Bates 2010-01-15 12:29:34 PST
Comment on attachment 46689 [details]
Patch with test case

Looks like QMacStyle cannot be found. Clearing review flag while I look into this.
Comment 12 Tor Arne Vestbø 2010-01-18 01:59:19 PST
Comment on attachment 46689 [details]
Patch with test case

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

There's a closing ) missing here
Comment 13 Tor Arne Vestbø 2010-01-18 04:26:54 PST
Comment on attachment 46689 [details]
Patch with test case

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 Daniel Bates 2010-01-18 20:10:00 PST
Created attachment 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 Tor Arne Vestbø 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 Daniel Bates 2010-01-20 18:31:46 PST
Comment on attachment 46882 [details]
Patch with test case

Clearing flags on attachment: 46882

Committed r53591: <http://trac.webkit.org/changeset/53591>
Comment 17 Daniel Bates 2010-01-20 18:31:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Daniel Bates 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 Daniel Bates 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 Daniel Bates 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 Daniel Bates 2010-01-25 00:00:18 PST
Created attachment 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 Daniel Bates 2010-01-26 21:29:25 PST
Comment on attachment 47320 [details]
Patch with test case

Clearing flags on attachment: 47320

Committed r53897: <http://trac.webkit.org/changeset/53897>
Comment 24 Daniel Bates 2010-01-26 21:29:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Brian Weinstein 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 Daniel Bates 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.