Bug 65896 – [Qt] Need spin-button implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kent Tamura]

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.

[Kent Tamura]

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

[Jarred Nicholls]

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

[Jarred Nicholls]

Created attachment 105303 [details]
Proposed Patch

[Jarred Nicholls]

Created attachment 105306 [details]
Proposed Patch

Whoops, text mate bundle removed white space :)

[Kenneth Rohde Christiansen]

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]

Created attachment 105348 [details]
Mac Screenshot

[Jarred Nicholls]

Created attachment 105349 [details]
Vista Screenshot

[Jarred Nicholls]

Created attachment 105350 [details]
Windows Classic Screenshot

[Jarred Nicholls]

Created attachment 105351 [details]
XP Screenshot

[Jarred Nicholls]

(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 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]

(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]

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]

Qt mobile theme: bug #67047

[WebKit Review Bot]

Comment on attachment 105381 [details]
Proposed Patch

Clearing flags on attachment: 105381

Committed r93937: <http://trac.webkit.org/changeset/93937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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]

I cc-ed Szeged gardeners of monday.

[Jarred Nicholls]

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]

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]

(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]

@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]

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]

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 :)