Add min and max input field values to Chromium API

[Anthony Berent]

Add min and max input field values to Chromium API

[Anthony Berent]

Created attachment 163939 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Anthony Berent]

These are needed by Chrome on Android to allow it to limit the range of, for example, dates when using Android native widgets and dialogs for input.

[Anthony Berent]

Created attachment 163964 [details]
Patch

[Darin Adler]

Comment on attachment 163964 [details]
Patch

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

> Source/WebKit/chromium/src/WebTextInputInfo.cpp:45
> +        && min == min
> +        && max == max;

Missing the "other." prefix on both of these lines.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2162
> +        if (input->isNumberField()
> +            || input->isRangeControl()
> +            || input->isDateField()
> +            || input->isDateTimeField()
> +            || input->isDateTimeLocalField()
> +            || input->isMonthField()
> +            || input->isWeekField()
> +            || input->isTimeField()) {
> +            info.min = input->minimum();
> +            info.max = input->maximum();

This long if statement is bad for maintenance of HTMLInputElement. We may add new types or decide not to use this style of boolean getter. Is there some reason you can’t just call minimum and maximum unconditionally?

[Anthony Berent]

(In reply to comment #5)
> (From update of attachment 163964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163964&action=review
> 
> > Source/WebKit/chromium/src/WebTextInputInfo.cpp:45
> > +        && min == min
> > +        && max == max;
> 
> Missing the "other." prefix on both of these lines.

Agreed, will fix.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2162
> > +        if (input->isNumberField()
> > +            || input->isRangeControl()
> > +            || input->isDateField()
> > +            || input->isDateTimeField()
> > +            || input->isDateTimeLocalField()
> > +            || input->isMonthField()
> > +            || input->isWeekField()
> > +            || input->isTimeField()) {
> > +            info.min = input->minimum();
> > +            info.max = input->maximum();
> 
> This long if statement is bad for maintenance of HTMLInputElement. We may add new types or decide not to use this style of boolean getter. Is there some reason you can’t just call minimum and maximum unconditionally?

There are comments in HTMLInputElement saying one should not call them for other types. As such it seemed the check was needed.

    // Returns the minimum value for type=date, number, or range.  Don't call this for other types.
    double minimum() const;
    // Returns the maximum value for type=date, number, or range.  Don't call this for other types.
    // This always returns a value which is >= minimum().
    double maximum() const;

[Anthony Berent]

Created attachment 163982 [details]
Patch

[Kent Tamura]

Comment on attachment 163982 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:8
> +        Required to correctly bound values in native widgets used, for example, for date entry in Chrome on Android.

I'm not sure this should be done in WebTextInputInfo.  Can we obtain them from WebInputElement? Can we expose WebCore/platform/DateTimeChooser.h via WebKit API?

> Source/WebKit/chromium/public/WebTextInputInfo.h:61
> +    double min;
> +
> +    // The maximum value of the field (only meaningful for numbers, ranges,
> +    // and date or time based fields).
> +    double max;
> +

min and max should be initialized in the constructor.
Also, we should have "step" value.

> Source/WebKit/chromium/tests/WebViewTest.cpp:377
> +    EXPECT_EQ((double)1000*24*60*60, info.max);
> +    EXPECT_EQ((double)-1000*24*60*60*2, info.min);

Do not use C-style cast.  In this case, 1000.0 * 24 * 60 * 60 works without the cast.

Comment on attachment 163982 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:2153
> +        if (input->isNumberField()

How about using HTMLInputElement::isSteppable() and HTMLInputElement::createStepRange()?
If we use them, we can write

if (input->isSteppable()) {
  const StepRange stepRange = input->createStepRange();
  info.min = stepRange.minimum();
  info.max = stepRange.maximum();
}

Note: HTMLInputElement::minimum() and maximum() call createStepRange().

[Anthony Berent]

There is a plan to change how date and time are supported in WebKit (hence some of Kent's comments). As such we are suspending this work until that change has been made.

[Stephen Chenney]

Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.