Bug 29564

Summary:

[Qt] Webkit Button Size

Product:

WebKit

Reporter:

Tor Arne Vestbø <vestbo>

Component:

Layout and Rendering

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bweinstein, dbates, eric, hausmann, kenneth, ossy, vestbo, webkit.review.bot, zoltan

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch with test case	vestbo: review-
Self-contained layout test	none
Patch with test cases	none
Patch with test case	none
Patch with test case	vestbo: review-
Patch with test case	none
Patch with test case	none

Tor Arne Vestbø

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

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Daniel Bates

Comment 1 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>).

Daniel Bates

Comment 2 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>.

Kenneth Rohde Christiansen

Comment 3 2010-01-15 02:30:54 PST

What is up with the self-contained layout test? It doesnt even open here

Tor Arne Vestbø

Comment 4 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.

Tor Arne Vestbø

Comment 5 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?

Daniel Bates

Comment 6 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.

Daniel Bates

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

WebKit Review Bot

Comment 8 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

Daniel Bates

Comment 9 2010-01-15 10:41:34 PST

Created attachment 46689 [details] Patch with test case Fix style error found by style bot.

WebKit Review Bot

Comment 10 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

Daniel Bates

Comment 11 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.

Tor Arne Vestbø

Comment 12 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

Tor Arne Vestbø

Comment 13 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

Daniel Bates

Comment 14 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.

Tor Arne Vestbø

Comment 15 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.

Daniel Bates

Comment 16 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>

Daniel Bates

Comment 17 2010-01-20 18:31:54 PST

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

Comment 18 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

Daniel Bates

Comment 19 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.

Daniel Bates

Comment 20 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.

Daniel Bates

Comment 21 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.

Daniel Bates

Comment 22 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.

Daniel Bates

Comment 23 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>

Daniel Bates

Comment 24 2010-01-26 21:29:34 PST

All reviewed patches have been landed. Closing bug.

Brian Weinstein

Comment 25 2010-01-27 16:24:24 PST

This test failed on Windows since it was created. Landed expected failing results for Windows in r53959.

Daniel Bates

Comment 26 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.

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