Bug 27451 - stepUp() and stepDown() support for <input type=number/range>
Summary: stepUp() and stepDown() support for <input type=number/range>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Kent Tamura
URL: http://www.whatwg.org/specs/web-apps/...
Keywords:
Depends on: 28934 29069 31330 31331
Blocks: HTML5Forms 24703 27968 30847
  Show dependency treegraph
 
Reported: 2009-07-20 11:48 PDT by Peter Kasting
Modified: 2009-12-07 18:29 PST (History)
7 users (show)

See Also:


Attachments
Patch part 1: Expose ECMA-262-version of dtoa() (8.40 KB, patch)
2009-10-28 02:34 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch part 2: step attribute and ValidityState.stepMismatch (25.07 KB, patch)
2009-10-28 02:49 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch part 3: stepUp() and stepDown() (29.15 KB, patch)
2009-10-28 02:54 PDT, Kent Tamura
darin: review-
Details | Formatted Diff | Diff
Patch part 3: stepUp() and stepDown() (rev.2) (28.46 KB, patch)
2009-11-05 22:54 PST, Kent Tamura
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch part 3: stepUp() and stepDown() (rev.3) (28.10 KB, patch)
2009-11-05 23:02 PST, Kent Tamura
darin: review-
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch part 3: stepUp() and stepDown() (rev.4) (24.03 KB, patch)
2009-11-08 21:59 PST, Kent Tamura
tkent: commit-queue-
Details | Formatted Diff | Diff
stepUp() and stepDown() (rev.5) (25.05 KB, patch)
2009-11-10 22:39 PST, Kent Tamura
tkent: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (rev.6) (31.73 KB, patch)
2009-11-18 23:31 PST, Kent Tamura
darin: review-
Details | Formatted Diff | Diff
Proposed patch (rev.7) (36.73 KB, patch)
2009-11-19 17:04 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.8) (36.59 KB, patch)
2009-11-19 17:08 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.9) (36.61 KB, patch)
2009-11-30 16:49 PST, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-07-20 11:48:33 PDT
See spec section 4.10.4.2.10.

This encompasses parsing the "step" attribute and hooking it to the stepMismatch() method on the ValidityState object.
Comment 1 Kent Tamura 2009-10-06 18:27:13 PDT
I focus on type=number and type=range in this bug entry.
A work for date&time types will be another bug.
Comment 2 Kent Tamura 2009-10-28 02:34:31 PDT
Created attachment 42007 [details]
Patch part 1: Expose ECMA-262-version of dtoa()

This exposed function will be used in Patch part 3.
Comment 3 Kent Tamura 2009-10-28 02:49:15 PDT
Created attachment 42008 [details]
Patch part 2: step attribute and ValidityState.stepMismatch
Comment 4 Kent Tamura 2009-10-28 02:54:27 PDT
Created attachment 42009 [details]
Patch part 3: stepUp() and stepDown()

This patch has changes to JSC/V8 binding code because the parameters of stepUp() and stepDown() are optional.
Comment 5 Darin Adler 2009-10-28 09:27:26 PDT
Comment on attachment 42009 [details]
Patch part 3: stepUp() and stepDown()

> +JSValue JSHTMLInputElement::stepUp(ExecState* exec, const ArgList& args)
> +{
> +    HTMLInputElement* input = static_cast<HTMLInputElement*>(impl());
> +    ExceptionCode ec = 0;
> +    int step;
> +    if (args.size() < 1)
> +        input->stepUp(1, ec);
> +    else {
> +        bool ok;
> +        step = args.at(0).toInt32(exec, ok);
> +        if (exec->hadException())
> +            return jsUndefined();
> +        if (!ok)
> +            ec = TYPE_MISMATCH_ERR;
> +        else
> +            input->stepUp(step, ec);
> +    }
> +    setDOMException(exec, ec);
> +    return jsUndefined();
> +}

This sort of custom binding may be needed for this function for V8, but I do not think it’s needed for WebKit’s own JavaScript engine. DOM bindings already treat all arguments as optional, and missing arguments turn into the number 0 if the argument is of numeric type.

The only unusual bit is the idea that an argument of the type needs to lead to TYPE_MISMATCH_ERR. Other DOM functions that take integral parameters do not do this kind of type checking; they simply convert to a number. I don't think is a good idea to write custom bindings for these functions. If we do need to move to more type checking for functions that take integral parameters in the DOM, I'd like to do it within the automatic generation mechanism.

Again, the issues may be different for V8. I have not looked at the V8 DOM bindings at all.
Comment 6 Kent Tamura 2009-10-28 20:46:16 PDT
Thank you for reviewing.  I referred to JSHTMLOptionsCollection::add() for this code.

(In reply to comment #5)
> This sort of custom binding may be needed for this function for V8, but I do
> not think it’s needed for WebKit’s own JavaScript engine. DOM bindings already
> treat all arguments as optional, and missing arguments turn into the number 0
> if the argument is of numeric type.

We need to distinguish input.stepUp(0) and input.stepUp().  The latter is equivalent to input.stepUp(1).  So we need such custom binding code, right?


> The only unusual bit is the idea that an argument of the type needs to lead to
> TYPE_MISMATCH_ERR. Other DOM functions that take integral parameters do not do
> this kind of type checking; they simply convert to a number. I don't think is a
> good idea to write custom bindings for these functions. If we do need to move
> to more type checking for functions that take integral parameters in the DOM,
> I'd like to do it within the automatic generation mechanism.

I checked behavior of specifying strings to some functions which requires integer parameters, and confirmed no TYPE_MISMATCH_ERR.  I'll remove TYPE_MISMATCH_ERR in my patch.
Comment 7 Kent Tamura 2009-11-05 22:54:23 PST
Created attachment 42630 [details]
Patch part 3: stepUp() and stepDown() (rev.2)

- Stop to throw TYPE_MISMATCH_ERR.
Comment 8 Kent Tamura 2009-11-05 23:02:13 PST
Created attachment 42633 [details]
Patch part 3: stepUp() and stepDown() (rev.3)

Oops, forgot to remove unnecessary '#include "ExceptionCode.h"'
Comment 9 Darin Adler 2009-11-06 11:32:13 PST
Comment on attachment 42633 [details]
Patch part 3: stepUp() and stepDown() (rev.3)

We need a test case for what happens when we pass undefined or null to stepUp. The binding currently handles that differently from just leaving out the argument, and we need to document that so we don't accidentally change it later. Also need test cases for extra arguments, and arguments of the wrong type. All this is especially important because we have hand-written bindings. Otherwise we could just fall back on "its the same as everything else" and make excuses for less thorough testing.

The "rint" function is the wrong one to use for rounding. It rounds based on the current rounding mode, and we do not want the code's behavior to be different depending on the rounding mode. Instead you should use the "round" function, which always rounds in the same way.

> +    Vector<UChar> uchars = WTF::ecma262dtoa(num);
> +    return String(uchars.data(), uchars.size());

This patch does not include the change to add this function, ecma262dtoa, to WTF. I'm not certain that's a good name for the function, by the way, but we can debate that in a patch to export the function. Besides putting it into a header file it would need to be added to JavaScriptCore.exp to avoid breaking the Mac build and other changes as well.

The style of calling WTF is wrong here. Functions exported from WTF are supposed to have using declarations in the header file, and callers are supposed to simply call them without an explicit WTF prefix.

> +    // Implementations of HTMLInputElement::stepUp() and stepDown().
> +    void stepUp(int, ExceptionCode&);
> +    void stepDown(int, ExceptionCode&);

The overloading of stepUp/stepDown where there is one version without the int that goes up by 1 and another with the int should be handled by overloading in the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file could be enough to let the bindings generator handle this instead of hand-writing a binding. It is not good to hand-write a binding just for this simple reason. And I think we should look at enhancing the bindings generators to eliminate these hand-written functions.

> +    static String doubleToFormString(double);

Typically we name functions based on what they return, not what they take. So this would be formStringFromDouble. This makes the code read more clearly, otherwise it looks like you are calling a function named "doubleXXX" and getting a string.
Comment 10 Kent Tamura 2009-11-08 21:59:17 PST
Created attachment 42733 [details]
Patch part 3: stepUp() and stepDown() (rev.4)

Thank you for the comments.

(In reply to comment #9)
> The "rint" function is the wrong one to use for rounding. It rounds based on
> the current rounding mode, and we do not want the code's behavior to be
> different depending on the rounding mode. Instead you should use the "round"
> function, which always rounds in the same way.

Done.

> > +    Vector<UChar> uchars = WTF::ecma262dtoa(num);
> > +    return String(uchars.data(), uchars.size());
> 
> This patch does not include the change to add this function, ecma262dtoa, to
> WTF. I'm not certain that's a good name for the function, by the way, but we
> can debate that in a patch to export the function. Besides putting it into a
> header file it would need to be added to JavaScriptCore.exp to avoid breaking
> the Mac build and other changes as well.

Would you take a look at Part 1 and Part 2 patches in this bug please?

> The style of calling WTF is wrong here. Functions exported from WTF are
> supposed to have using declarations in the header file, and callers are
> supposed to simply call them without an explicit WTF prefix.

I couldn't find examples of "using namespace WTF" in dom/*.h and html/*.h, and found it at the top of *.cpp.
I followed it in the updated patch.

> > +    // Implementations of HTMLInputElement::stepUp() and stepDown().
> > +    void stepUp(int, ExceptionCode&);
> > +    void stepDown(int, ExceptionCode&);
> 
> The overloading of stepUp/stepDown where there is one version without the int
> that goes up by 1 and another with the int should be handled by overloading in
> the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file
> could be enough to let the bindings generator handle this instead of
> hand-writing a binding. It is not good to hand-write a binding just for this
> simple reason. And I think we should look at enhancing the bindings generators
> to eliminate these hand-written functions.

Oh, I didn't know the bindings generator was so clever.  I removed the custom binding code.
So we don't need to test parameter errors like the following your comment, right?

> We need a test case for what happens when we pass undefined or null to stepUp.
> The binding currently handles that differently from just leaving out the
> argument, and we need to document that so we don't accidentally change it
> later. Also need test cases for extra arguments, and arguments of the wrong
> type. All this is especially important because we have hand-written bindings.
> Otherwise we could just fall back on "its the same as everything else" and make
> excuses for less thorough testing.
> 


> > +    static String doubleToFormString(double);
> 
> Typically we name functions based on what they return, not what they take. So
> this would be formStringFromDouble. This makes the code read more clearly,
> otherwise it looks like you are calling a function named "doubleXXX" and
> getting a string.

Done.
Comment 11 Darin Adler 2009-11-09 09:59:51 PST
(In reply to comment #10)
> > > +    Vector<UChar> uchars = WTF::ecma262dtoa(num);
> > > +    return String(uchars.data(), uchars.size());
> > 
> > This patch does not include the change to add this function, ecma262dtoa, to
> > WTF. I'm not certain that's a good name for the function, by the way, but we
> > can debate that in a patch to export the function. Besides putting it into a
> > header file it would need to be added to JavaScriptCore.exp to avoid breaking
> > the Mac build and other changes as well.
> 
> Would you take a look at Part 1 and Part 2 patches in this bug please?

OK, I see now. Confusing!

> > The style of calling WTF is wrong here. Functions exported from WTF are
> > supposed to have using declarations in the header file, and callers are
> > supposed to simply call them without an explicit WTF prefix.
> 
> I couldn't find examples of "using namespace WTF" in dom/*.h and html/*.h, and
> found it at the top of *.cpp.
> I followed it in the updated patch.

That is not what I was talking about. I was talking about using declarations in the header files inside WTF. For example, look at the bottom of ASCIICType.h.

> > The overloading of stepUp/stepDown where there is one version without the int
> > that goes up by 1 and another with the int should be handled by overloading in
> > the C++ DOM too. Eventually, the [Optional] on the argument in the IDL file
> > could be enough to let the bindings generator handle this instead of
> > hand-writing a binding. It is not good to hand-write a binding just for this
> > simple reason. And I think we should look at enhancing the bindings generators
> > to eliminate these hand-written functions.
> 
> Oh, I didn't know the bindings generator was so clever.  I removed the custom
> binding code.
> So we don't need to test parameter errors like the following your comment,
> right?

We do need to test them. Just because the bindings generator provides the support doesn't mean we don't want test cases. We should always test such things to make sure we are getting the behavior we need.
Comment 12 Kent Tamura 2009-11-10 21:31:21 PST
I'll move Part 1 and 2 patches to their own bugs.
Comment 13 Kent Tamura 2009-11-10 22:39:00 PST
Created attachment 42926 [details]
stepUp() and stepDown() (rev.5)

(In reply to comment #11)
> > Would you take a look at Part 1 and Part 2 patches in this bug please?
> OK, I see now. Confusing!

I moved them to their own bugs.

> That is not what I was talking about. I was talking about using declarations in
> the header files inside WTF. For example, look at the bottom of ASCIICType.h.

Oh, I have understood.  Updated dtoa.h in the patch part 1.

> > So we don't need to test parameter errors like the following your comment,
> > right?
> 
> We do need to test them. Just because the bindings generator provides the
> support doesn't mean we don't want test cases. We should always test such
> things to make sure we are getting the behavior we need.

I added tests for null/undefined/extra arguments.
Comment 14 Kent Tamura 2009-11-11 23:58:43 PST
Comment on attachment 42926 [details]
stepUp() and stepDown() (rev.5)

I need to update the patch due to the ecma262dtoa() change.
Comment 15 Kent Tamura 2009-11-18 23:31:48 PST
Created attachment 43489 [details]
Proposed patch (rev.6)

- sync with the function name change of ecma262dtoa()
- improve the test result readability
- RenderSlider uses the double-to-string function introduced by this change
Comment 16 Darin Adler 2009-11-19 10:34:24 PST
Comment on attachment 43489 [details]
Proposed patch (rev.6)

> +Tests stepDown()/stepUp() for unsuppoted typpes

A couple spelling errors.

> +void HTMLInputElement::addForNumber(double step, double count, ExceptionCode& ec)

This function name doesn't seem great. I would call it something more like "applyStep". Having "for number" in the name of a function that works on both number and range seems like an immediate mistake.

> +    double current = 0.0;
> +    if (!formStringToDouble(value(), &current)) {
> +        ec = INVALID_STATE_ERR;
> +        return;
> +    }

No need to initialize "current" here.

> +        double minimum = 0.0;
> +        if (formStringToDouble(getAttribute(minAttr), &minimum)) {
> +            if (newValue < minimum) {
> +                ec = INVALID_STATE_ERR;
> +                return;
> +            }
> +        }
> +        double n = round((newValue - minimum) / step);
> +        newValue = minimum + n * step;

We might want to initialize minimum to "defaultMinimum" rather than an explicit 0.

I think the code would read more clearly as a single-line expression without the local variable "n".

> +        double maximum = 0.0;
> +        if (formStringToDouble(getAttribute(maxAttr), &maximum)) {
> +            if (newValue > maximum) {
> +                ec = INVALID_STATE_ERR;
> +                return;
> +            }
> +        }

No need to initialize "maximum" here.

I think it would be better to factor things so that the step, minimum, maximum fetching was in separate functions. I don't see any reason to have entirely separate code for range and number just because the minimum and maximum are computed differently for those two. I would have minimum and maximum functions, and return infinity if there is no maximum. Or a getMaximum function that returns a boolean.

Generally speaking it's annoying to have all the logic twice in this function!

I think you should just call getAllowedValueStep inside the applyStep function, and make the stepUp and stepDown functions just pass things through to applyStep.

> +String HTMLInputElement::formStringFromDouble(double num)

Please use whole words. Use "number" instead of "num".

> +    // Converts the specified number to a string.  This is an implementation of
> +    // HTML5's "algorithm to convert a number to a string" for NUMBER/RANGE types.
> +    static String formStringFromDouble(double);

We use one space after a period.

I'll say review- because I think at least the name "addForNumber" needs to change.
Comment 17 Kent Tamura 2009-11-19 17:04:04 PST
Created attachment 43535 [details]
Proposed patch (rev.7)

(In reply to comment #16)
> (From update of attachment 43489 [details])
> > +Tests stepDown()/stepUp() for unsuppoted typpes
> 
> A couple spelling errors.

Fixed.

> > +void HTMLInputElement::addForNumber(double step, double count, ExceptionCode& ec)
> 
> This function name doesn't seem great. I would call it something more like
> "applyStep". Having "for number" in the name of a function that works on both
> number and range seems like an immediate mistake.

Renamed to applyStepForNumberOrRange().

> > +    double current = 0.0;
> > +    if (!formStringToDouble(value(), &current)) {
> > +        ec = INVALID_STATE_ERR;
> > +        return;
> > +    }
> 
> No need to initialize "current" here.

Fixed.

> > +        double minimum = 0.0;
> > +        if (formStringToDouble(getAttribute(minAttr), &minimum)) {
> > +            if (newValue < minimum) {
> > +                ec = INVALID_STATE_ERR;
> > +                return;
> > +            }
> > +        }
> > +        double n = round((newValue - minimum) / step);
> > +        newValue = minimum + n * step;
> 
> We might want to initialize minimum to "defaultMinimum" rather than an explicit
> 0.

Removed the code.

> I think the code would read more clearly as a single-line expression without
> the local variable "n".

Done.

> > +        double maximum = 0.0;
> > +        if (formStringToDouble(getAttribute(maxAttr), &maximum)) {
> > +            if (newValue > maximum) {
> > +                ec = INVALID_STATE_ERR;
> > +                return;
> > +            }
> > +        }
> 
> No need to initialize "maximum" here.

Removed the code.

> I think it would be better to factor things so that the step, minimum, maximum
> fetching was in separate functions. I don't see any reason to have entirely
> separate code for range and number just because the minimum and maximum are
> computed differently for those two. I would have minimum and maximum functions,
> and return infinity if there is no maximum. Or a getMaximum function that
> returns a boolean.
> 
> Generally speaking it's annoying to have all the logic twice in this function!

I made some changes to unify the code for NUMBER and RANGE.
 - Renamd rangeMinimum() and rangeMaximum() to minimum() and maximum(), and add support for NUMBER.
 - Introduce stepBase().
Now rangeUnderflow(), rangeOverflow() and applyStepForNumberOrRange() get cleaner :-)


> I think you should just call getAllowedValueStep inside the applyStep function,
> and make the stepUp and stepDown functions just pass things through to
> applyStep.

Done.

> > +String HTMLInputElement::formStringFromDouble(double num)
> 
> Please use whole words. Use "number" instead of "num".

Done.

> > +    // Converts the specified number to a string.  This is an implementation of
> > +    // HTML5's "algorithm to convert a number to a string" for NUMBER/RANGE types.
> > +    static String formStringFromDouble(double);
> 
> We use one space after a period.

Done.
Comment 18 Kent Tamura 2009-11-19 17:08:39 PST
Created attachment 43536 [details]
Proposed patch (rev.8)

Oops, I uploaded an old patch.
Comment 19 Adam Barth 2009-11-30 12:32:02 PST
Attachment 43536 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/HTMLInputElement.cpp:307:  One line control clauses should not use braces.  [whitespace/braces] [4]
Done processing WebCore/html/HTMLInputElement.cpp
Done processing WebCore/rendering/RenderSlider.cpp
Done processing WebCore/html/HTMLInputElement.h
Total errors found: 1
Comment 20 Kent Tamura 2009-11-30 16:49:12 PST
Created attachment 44047 [details]
Proposed patch (rev.9)

> WebCore/html/HTMLInputElement.cpp:307:  One line control clauses should not use
> braces.  [whitespace/braces] [4]

Fixed.
Comment 21 WebKit Review Bot 2009-11-30 16:54:16 PST
style-queue ran check-webkit-style on attachment 44047 [details] without any errors.
Comment 22 Darin Adler 2009-12-07 10:22:20 PST
Comment on attachment 44047 [details]
Proposed patch (rev.9)

> +    static const double defaultMinimumForNumber = -DBL_MAX;
>      // The range type's "default minimum" is 0.
> +    static const double defaultMinimumForRange = 0.0;

> +    static const double defaultMaximumForNumber = DBL_MAX;
>      // The range type's "default maximum" is 100.
> +    static const double defaultMaximumForRange = 100.0;

> +        static const double defaultStepBase = 0.0;

I think these constants should go at the top of the file instead of in the various functions that use them.

> +        // A remedy for the inconsistent min/max values for RANGE.
> +        // Sets the maxmimum to the default or the minimum value.
> +        const double min = minimum();

Normally we do not use const for local variables in WebKit.

> +        if (max < min)
> +            max = min < defaultMaximum ? defaultMaximum : min;

This could be written more cleanly using the max function:

    max = std::max(std::max(min, defaultMaximum), max);

The idea is that you use the highest of the three: min, max, and defaultMaximum.

Also, if the local variable was not named max, you would not need the "std::" prefixes either.

r=me as is -- maybe you can adopt my suggestions in a later patch
Comment 23 Kent Tamura 2009-12-07 18:29:26 PST
I modified for Darin's suggestions, and committed manually as r51822.