Bug 65896

Summary: [Qt] Need spin-button implementation
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, feherzs, jarred, kbalazs, kkristof, kling, loki, ossy, webkit.review.bot, zoltan
Priority: P2 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

Description Kent Tamura 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.
Comment 1 Kent Tamura 2011-08-08 21:00:13 PDT
*** Bug 65851 has been marked as a duplicate of this bug. ***
Comment 2 Jarred Nicholls 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
Comment 3 Jarred Nicholls 2011-08-25 21:27:36 PDT
Created attachment 105303 [details]
Proposed Patch
Comment 4 Jarred Nicholls 2011-08-25 21:48:50 PDT
Created attachment 105306 [details]
Proposed Patch

Whoops, text mate bundle removed white space :)
Comment 5 Kenneth Rohde Christiansen 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)
Comment 6 Jarred Nicholls 2011-08-26 05:22:49 PDT
Created attachment 105348 [details]
Mac Screenshot
Comment 7 Jarred Nicholls 2011-08-26 05:23:27 PDT
Created attachment 105349 [details]
Vista Screenshot
Comment 8 Jarred Nicholls 2011-08-26 05:23:49 PDT
Created attachment 105350 [details]
Windows Classic Screenshot
Comment 9 Jarred Nicholls 2011-08-26 05:24:07 PDT
Created attachment 105351 [details]
XP Screenshot
Comment 10 Jarred Nicholls 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!
Comment 11 Kent Tamura 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.
Comment 12 Jarred Nicholls 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.
Comment 13 Jarred Nicholls 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.
Comment 14 Jarred Nicholls 2011-08-26 11:09:27 PDT
Qt mobile theme: bug #67047
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2011-08-27 04:06:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Csaba Osztrogonác 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."
Comment 18 Csaba Osztrogonác 2011-08-29 02:09:37 PDT
I cc-ed Szeged gardeners of monday.
Comment 19 Jarred Nicholls 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.
Comment 20 Zoltan Horvath 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
Comment 21 Balazs Kelemen 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?
Comment 22 Jarred Nicholls 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
Comment 23 Jarred Nicholls 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.
Comment 24 Jarred Nicholls 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 :)