Bug 45803 – Support keyboard operations for <input type=range>

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Support keyboard operations for <input type=range>

[Kent Tamura]

Created attachment 67640 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 67640 [details]
Patch

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

Aren't you worried about the refactoring gods getting angry with you? :) This defaultEventHandler method is now nearly as long as the Ecclesiastes.

[chris fleizach]

Comment on attachment 67640 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +        Support keyboard operations for <input type=range>

end in period

> WebCore/ChangeLog:5
> +        Support keyboard operations for <input type=range>

end in period.

> WebCore/html/HTMLInputElement.cpp:2264
> +        } else if (inputType() == RANGE) {

instead of making this if block even longer, i'd suggest putting the RANGE code into another method for easing readability

> WebCore/html/HTMLInputElement.cpp:2265
> +            if (key == "Up" || key == "Right" || key == "Down" || key == "Left") {

an early return would help here.

> WebCore/html/HTMLInputElement.cpp:2281
> +                        }

the previous 5 lines can be combined with the 5 lines in Down and Left

> WebCore/html/HTMLInputElement.cpp:2296
> +                        step = -1;

shouldn't step honor what's set in the "stepAttr" field?

[Kent Tamura]

Created attachment 68333 [details]
Patch 2

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #3)
> > LayoutTests/ChangeLog:5
> > +        Support keyboard operations for <input type=range>
> 
> end in period

done.

> > WebCore/ChangeLog:5
> > +        Support keyboard operations for <input type=range>
> 
> end in period.

done.

> > WebCore/html/HTMLInputElement.cpp:2264
> > +        } else if (inputType() == RANGE) {
> 
> instead of making this if block even longer, i'd suggest putting the RANGE code into another method for easing readability

done.

> 
> > WebCore/html/HTMLInputElement.cpp:2265
> > +            if (key == "Up" || key == "Right" || key == "Down" || key == "Left") {
> 
> an early return would help here.

done.

> > WebCore/html/HTMLInputElement.cpp:2281
> > +                        }
> 
> the previous 5 lines can be combined with the 5 lines in Down and Left

done.

> > WebCore/html/HTMLInputElement.cpp:2296
> > +                        step = -1;
> 
> shouldn't step honor what's set in the "stepAttr" field?

This code reflects stepAttr value because the parameter of stepUp() is a magnification.
I renamed "step" to "stepMagnification".

[Kent Tamura]

(In reply to comment #4)
> Created an attachment (id=68333) [details]
> Patch 2

The patch adds new call of deprecatedInputType().  It is going to be removed by Bug#45872.

[chris fleizach]

Comment on attachment 68333 [details]
Patch 2

r=me

[Kent Tamura]

Comment on attachment 68333 [details]
Patch 2

Clearing flags on attachment: 68333

Committed r68024: <http://trac.webkit.org/changeset/68024>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.