Bug 86846 – [EFL] <input type="number"> is not a spinbutton

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Alexander Shalamov]

RenderThemeEfl does not implement inner spin button theming methods:
virtual void adjustInnerSpinButtonStyle(StyleResolver*, RenderStyle*, Element*) const;
virtual bool paintInnerSpinButton(RenderObject*, const PaintInfo&, const IntRect&);

[Alexander Shalamov]

Created attachment 142695 [details]
Patch

Implemented spinner widget for input element

[Alexander Shalamov]

Created attachment 143028 [details]
Patch

Updated layout of arrow elements.

[Gyuyoung Kim]

Comment on attachment 143028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143028&action=review

When you submit png format you need to use --binary option. I couldn't download .png file in this bug.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:84
> +            description { state: "default" 0.0;

It is good to consistent with other file. I think property definition doesn't go on the same line.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:98
> +            description { state: "pressed" 0.0;

ditto.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:114
> +        part { name: "down_bt";

ditto.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:129
> +            description { state: "pressed" 0.0;

ditto.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:163
> +        program { name: "spinup";

ditto.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:170
> +

Unneeded line.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:178
> +

ditto.

[Alexander Shalamov]

Created attachment 143254 [details]
Patch

Fixed review comments, added binary patch

[Alexander Shalamov]

Created attachment 144507 [details]
Patch

rebased to master

[Gyuyoung Kim]

Comment on attachment 144507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144507&action=review

Almost looks good to me. BTW, can't this patch cover test cases in layout test ?

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:83
> +        part { name: "up_bt";

Move *name: "up_bt"* field to new line as well.

> Source/WebKit/efl/DefaultTheme/widget/spinner/spinner.edc:116
> +        part { name: "down_bt";

ditto.

[Alexander Shalamov]

Created attachment 144739 [details]
Patch

Moved up_bt and down_bt definitions to new line
Unskipped related layout tests from test_expectations.txt

[WebKit Review Bot]

Attachment 144739 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit/efl/DefaultTheme/widget/spinner/sp_up_default.png:0:  Image lacks a checksum. Generate pngs using run-webkit-tests to ensure they have a checksum.  [image/png] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Shalamov]

Created attachment 144741 [details]
Patch

rebased to master

[WebKit Review Bot]

Attachment 144741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit/efl/DefaultTheme/widget/spinner/sp_up_default.png:0:  Image lacks a checksum. Generate pngs using run-webkit-tests to ensure they have a checksum.  [image/png] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

It looks you need to check to add png files. BTW, can you unskip the tests with this patch ?

[Alexander Shalamov]

(In reply to comment #11)
> It looks you need to check to add png files.
I don't think I need. Those pngs are not test expectations, just a graphics for buttons.

>BTW, can you unskip the tests with this patch ?
I've unskipped tests in my last 2 patches.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > It looks you need to check to add png files.
> I don't think I need. Those pngs are not test expectations, just a graphics for buttons.

When I make a patch with your patch, there is no style error on my side. Did you have style error when you check it on your local ?

> >BTW, can you unskip the tests with this patch ?
> I've unskipped tests in my last 2 patches.

Do you mean did you already unskip test cases ?

[Alexander Shalamov]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > It looks you need to check to add png files.
> > I don't think I need. Those pngs are not test expectations, just a graphics for buttons.
> 
> When I make a patch with your patch, there is no style error on my side. Did you have style error when you check it on your local ?

After rebasing to master, yes I get same style error.
New check was added to png.py checker commit a6a6a184cd992aa880f262b6c07a7725166a7da6

> > >BTW, can you unskip the tests with this patch ?
> > I've unskipped tests in my last 2 patches.
> 
> Do you mean did you already unskip test cases ?

Yes, I removed failing tests from Skipped and moved to test_expectations.txt
https://bugs.webkit.org/show_bug.cgi?id=87629

[Gyuyoung Kim]

Comment on attachment 144741 [details]
Patch

In code level, looks good to me.

[Raphael Kubo da Costa (:rakuco)]

Looks great, thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 144741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144741&action=review

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:990
> +    if (!m_page && element && element->document()->page()) {

I think this deserves a comment.

When can m_page become null, and there still be elements?

Maybe you want an accessor instead like page() { return m_page ? m_page : ...

[Alexander Shalamov]

(In reply to comment #17)
> (From update of attachment 144741 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144741&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:990
> > +    if (!m_page && element && element->document()->page()) {
> 
> I think this deserves a comment.

Adding. Thanks.

> When can m_page become null, and there still be elements?

When style for an element is adjusted, default theme is used (StyleResolver::adjustRenderStyle). Default theme doesn't have page associated with it, therefore page will be null. In that case, we use theme of a page that is associated with an element which style is adjusted.

[Alexander Shalamov]

Created attachment 145043 [details]
Patch

- Rebased to master
- Added comments for RenderThemeEfl::adjustInnerSpinButtonStyle

[WebKit Review Bot]

Attachment 145043 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/efl/RenderThemeEfl.cpp:990:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 145043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145043&action=review

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:990
>> +    // When style is recalculated, default theme is used. In that case, m_page will be NULL.
> 
> Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]

I see this is all over the code, so lets commit your original patch

[WebKit Review Bot]

Comment on attachment 144741 [details]
Patch

Clearing flags on attachment: 144741

Committed r119195: <http://trac.webkit.org/changeset/119195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.