Bug 45803 - Support keyboard operations for <input type=range>
Summary: Support keyboard operations for <input type=range>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-14 21:05 PDT by Kent Tamura
Modified: 2010-09-22 01:10 PDT (History)
3 users (show)

See Also:

Attachments
Patch (10.28 KB, patch)
2010-09-14 21:11 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Patch 2 (10.26 KB, patch)
2010-09-21 20:11 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-09-14 21:05:59 PDT 
Support keyboard operations for <input type=range>
Comment 1 Kent Tamura 2010-09-14 21:11:15 PDT 
Created attachment 67640 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2010-09-15 07:31:59 PDT 
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.
Comment 3 chris fleizach 2010-09-21 14:50:09 PDT 
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?
Comment 4 Kent Tamura 2010-09-21 20:11:57 PDT 
Created attachment 68333 [details]
Patch 2
Comment 5 Kent Tamura 2010-09-21 20:15:06 PDT 
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".
Comment 6 Kent Tamura 2010-09-21 20:18:55 PDT 
(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.
Comment 7 chris fleizach 2010-09-21 23:49:53 PDT 
Comment on attachment 68333 [details]
Patch 2

r=me
Comment 8 Kent Tamura 2010-09-22 01:09:54 PDT 
Comment on attachment 68333 [details]
Patch 2

Clearing flags on attachment: 68333

Committed r68024: <http://trac.webkit.org/changeset/68024>
Comment 9 Kent Tamura 2010-09-22 01:10:02 PDT 
All reviewed patches have been landed.  Closing bug.