Bug 65896

Summary:

[Qt] Need spin-button implementation

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abecsi, feherzs, jarred, kbalazs, kkristof, kling, loki, ossy, webkit.review.bot, zoltan

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

67118

Bug Blocks:

Attachments:

Description	Flags
Proposed Patch	none
Proposed Patch	none
Mac Screenshot	none
Vista Screenshot	none
Windows Classic Screenshot	none
XP Screenshot	none
Proposed Patch	none

Kent Tamura

Reported 2011-08-08 20:53:26 PDT

The following tests are failing because of no spin-button implementation in Qt. fast/forms/input-appearance-spinbutton-disabled-readonly.html fast/forms/input-appearance-spinbutton-layer.html fast/forms/input-number-events.html fast/forms/input-spinbutton-capturing.html fast/forms/input-number-large-padding.html fast/forms/spin-button-gets-disabled-or-readonly.html fast/forms/input-number-size.html Need to implement virtual void adjustInnerSpinButtonStyle(CSSStyleSelector*, RenderStyle*, Element*) const; virtual bool paintInnerSpinButton(RenderObject*, const PaintInfo&, const IntRect&) { return true; } in RenderThemeQt.

Attachments
Proposed Patch (173.88 KB, patch) 2011-08-25 21:27 PDT, Jarred Nicholls	no flags	Details Formatted Diff Diff
Proposed Patch (10.88 KB, patch) 2011-08-25 21:48 PDT, Jarred Nicholls	no flags	Details Formatted Diff Diff
Mac Screenshot (8.30 KB, image/png) 2011-08-26 05:22 PDT, Jarred Nicholls	no flags	Details
Vista Screenshot (5.06 KB, image/png) 2011-08-26 05:23 PDT, Jarred Nicholls	no flags	Details
Windows Classic Screenshot (4.72 KB, image/png) 2011-08-26 05:23 PDT, Jarred Nicholls	no flags	Details
XP Screenshot (5.33 KB, image/png) 2011-08-26 05:24 PDT, Jarred Nicholls	no flags	Details
Proposed Patch (11.02 KB, patch) 2011-08-26 11:02 PDT, Jarred Nicholls	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2011-08-08 21:00:13 PDT

*** Bug 65851 has been marked as a duplicate of this bug. ***

Jarred Nicholls

Comment 2 2011-08-25 13:13:05 PDT

Need to fix some tests, but patch is coming soon. Finally got around to doing this today :) http://cl.ly/0A143K3Q0Z111V0q0341

Jarred Nicholls

Comment 3 2011-08-25 21:27:36 PDT

Created attachment 105303 [details] Proposed Patch

Jarred Nicholls

Comment 4 2011-08-25 21:48:50 PDT

Created attachment 105306 [details] Proposed Patch Whoops, text mate bundle removed white space :)

Kenneth Rohde Christiansen

Comment 5 2011-08-26 01:17:37 PDT

Comment on attachment 105306 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105306&action=review Any screenshot of how this looks? > Source/WebCore/platform/qt/RenderThemeQt.cpp:1075 > + int width = ScrollbarTheme::nativeTheme()->scrollbarThickness(); So this will mean that on mobile platforms such as the N9 where we are not showing scrollbars, the width will be 0. Not good. > Source/WebCore/platform/qt/RenderThemeQt.cpp:1080 > +bool RenderThemeQt::paintInnerSpinButton(RenderObject* o, const PaintInfo& pi, const IntRect& r) We try to write out things like r to rect etc. o is often used though due to 'object' being quite overloaded. > Source/WebCore/platform/qt/RenderThemeQt.cpp:1109 > + // render mini aqua spin buttons We use proper sentences. (ie starts with capital, ends with punctuation mark)

Jarred Nicholls

Comment 6 2011-08-26 05:22:49 PDT

Created attachment 105348 [details] Mac Screenshot

Jarred Nicholls

Comment 7 2011-08-26 05:23:27 PDT

Created attachment 105349 [details] Vista Screenshot

Jarred Nicholls

Comment 8 2011-08-26 05:23:49 PDT

Created attachment 105350 [details] Windows Classic Screenshot

Jarred Nicholls

Comment 9 2011-08-26 05:24:07 PDT

Created attachment 105351 [details] XP Screenshot

Jarred Nicholls

Comment 10 2011-08-26 05:33:03 PDT

(In reply to comment #5) > (From update of attachment 105306 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105306&action=review > > Any screenshot of how this looks? Attached a few. > > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1075 > > + int width = ScrollbarTheme::nativeTheme()->scrollbarThickness(); > > So this will mean that on mobile platforms such as the N9 where we are not showing scrollbars, the width will be 0. Not good. > Yeah I was aware of that, and was counting on someone from Nokia to clarify what value (if any) should be given here for QT_MOBILE_THEME, and if we need to add to QtMobileWebTheme's drawComplexControl. Or, do we keep it the status quo. > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1080 > > +bool RenderThemeQt::paintInnerSpinButton(RenderObject* o, const PaintInfo& pi, const IntRect& r) > > We try to write out things like r to rect etc. o is often used though due to 'object' being quite overloaded. > Okay, I agree and I typically use clearer variable names, but more importantly I keep consistent w/ existing code. If you look around, almost every other template method in RenderThemeQt uses 1 char names. I hate it, so I'm happy to revise ;) > > Source/WebCore/platform/qt/RenderThemeQt.cpp:1109 > > + // render mini aqua spin buttons > > We use proper sentences. (ie starts with capital, ends with punctuation mark) Cool, I'll re-write this. Thanks!

Kent Tamura

Comment 11 2011-08-26 05:56:45 PDT

Comment on attachment 105306 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105306&action=review >>> Source/WebCore/platform/qt/RenderThemeQt.cpp:1075 >>> + int width = ScrollbarTheme::nativeTheme()->scrollbarThickness(); >> >> So this will mean that on mobile platforms such as the N9 where we are not showing scrollbars, the width will be 0. Not good. > > Yeah I was aware of that, and was counting on someone from Nokia to clarify what value (if any) should be given here for QT_MOBILE_THEME, and if we need to add to QtMobileWebTheme's drawComplexControl. Or, do we keep it the status quo. I think mobile platforms need a way other than inner-spin-button because it would be hard to click it. Mobile Safari on iOS and Android Browser don't have inner-spin-button.

Jarred Nicholls

Comment 12 2011-08-26 06:19:48 PDT

(In reply to comment #11) > (From update of attachment 105306 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105306&action=review > > >>> Source/WebCore/platform/qt/RenderThemeQt.cpp:1075 > >>> + int width = ScrollbarTheme::nativeTheme()->scrollbarThickness(); > >> > >> So this will mean that on mobile platforms such as the N9 where we are not showing scrollbars, the width will be 0. Not good. > > > > Yeah I was aware of that, and was counting on someone from Nokia to clarify what value (if any) should be given here for QT_MOBILE_THEME, and if we need to add to QtMobileWebTheme's drawComplexControl. Or, do we keep it the status quo. > > I think mobile platforms need a way other than inner-spin-button because it would be hard to click it. > Mobile Safari on iOS and Android Browser don't have inner-spin-button. Precisely. Andreas Kling thinks it should be something that's delegated to the platformplugin. I'll wait to hear for next steps, if any, and then will submit a revised patch based on Kenneth's comments.

Jarred Nicholls

Comment 13 2011-08-26 11:02:16 PDT

Created attachment 105381 [details] Proposed Patch Kenneth's comments, opening new ticket for mobile which is deemed out-of-scope for this bug.

Jarred Nicholls

Comment 14 2011-08-26 11:09:27 PDT

Qt mobile theme: bug #67047

WebKit Review Bot

Comment 15 2011-08-27 04:06:36 PDT

Comment on attachment 105381 [details] Proposed Patch Clearing flags on attachment: 105381 Committed r93937: <http://trac.webkit.org/changeset/93937>

WebKit Review Bot

Comment 16 2011-08-27 04:06:42 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 17 2011-08-29 02:07:45 PDT

It made 4 tests fail and 3 tests flakey: http://build.webkit.org/results/Qt%20Linux%20Release/r93937%20%2836869%29/results.html Will you fix it, or should we roll out the patch? Additionally it broke Qt minimal build ... Zoltan fixed it instead of you: http://trac.webkit.org/changeset/93955 Please respect the "Keeping the tree green" policy: http://www.webkit.org/coding/contributing.html "Your change must at least compile on all platforms." "Run the layout tests using the run-webkit-tests script and make sure they all pass." "You can watch the tree at build.webkit.org to make sure your patch builds and passes tests on all platforms."

Csaba Osztrogonác

Comment 18 2011-08-29 02:09:37 PDT

I cc-ed Szeged gardeners of monday.

Jarred Nicholls

Comment 19 2011-08-29 03:56:15 PDT

Hey Ossy, all fast/forms tests passed on my machine faithfully, there were no failures or flaky results. Of course I run the tests before I submit; but thanks for those kind links that I already read a year ago ;) I'll review further when I get in to my office why they're green for me but not for you. No sense in a roll out. I was also not aware of the minimal build wrt spinbox so I apologize for that, I'll get a min build of Qt here to avoid that going forward.

Zoltan Horvath

Comment 20 2011-08-29 04:20:06 PDT

I skipped them in http://trac.webkit.org/changeset/93968. Bug for the regression: https://bugs.webkit.org/show_bug.cgi?id=67118

Balazs Kelemen

Comment 21 2011-08-29 04:21:03 PDT

(In reply to comment #19) > Hey Ossy, all fast/forms tests passed on my machine faithfully, there were no failures or flaky results. Of course I run the tests before I submit; but thanks for those kind links that I already read a year ago ;) > > I'll review further when I get in to my office why they're green for me but not for you. No sense in a roll out. I was also not aware of the minimal build wrt spinbox so I apologize for that, I'll get a min build of Qt here to avoid that going forward. Thank you for taking care of that. However I don't understand your (I mean the Trolls :) ) policy of rollouting. The bot is valuable if it is green almost all the time. What is the benefit of not rolling out smg that makes it red? It doesn't depend on the kind of the failure. I think even a good patch that only needs rebaselined results should be rolled out if nobody is able to fix the redness immediately. It's not a big deal to reland a patch with updated results. Actually it is not really more tiring than just land the new results, is it?

Jarred Nicholls

Comment 22 2011-08-29 04:36:12 PDT

@Balazs Yeah I agree, immediate roll out makes sense over new patches/bug tickets/etc. I'm at my workstation now so I'll investigate and check in over at bug #67118

Jarred Nicholls

Comment 23 2011-08-29 05:00:14 PDT

Just FYI, I unskipped the failing/flaky tests and still getting green results after a fresh build from upstream: http://cl.ly/2o0K2C3t0v3r2E0I1n2F http://cl.ly/1r1D1h3P1Q3F3Z1R1H09 I'm downloading the Debian build/test VM because there's clearly some discrepancy between my os x box and the bot. Also running a full sweep of the tests.

Jarred Nicholls

Comment 24 2011-08-29 12:31:28 PDT

This is 100% a DRT difference between OS X and Linux. Layers simply aren't calculated to be the same size. I'll be running and (re)baselining tests on Linux from now on, that's for sure :)

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