Bug 60673 – <input type=number> doesn't ignore size="" attribute

• Component: Forms
• Status: RESOLVED FIXED

[Ian 'Hixie' Hickson]

<input type=number> is supposed to size according to the min/max/step attributes so that it will be as wide as necessary to represent the longest allowed number:

# These controls are all expected to be about one line high, and about as wide as necessary to show the widest possible value.
 -- http://www.whatwg.org/specs/web-apps/current-work/complete.html#the-input-element-as-domain-specific-widgets

Currently WebKit doesn't size per the min/max/step and does support size="", which should be ignored on type=number elements.

Testcase:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/980

[Shinya Kawanaka]

When the input does not have min or max value, the possible value could be very large. The default max value is currently FLOAT_MAX, i.e. 3.402823466e+38. So the length of possible value could be around 40, because the current implementation does not use scientific format in some situation. This means the input width could be very large.

What do you think about it?

[Kent Tamura]

IMO, We should adjust the width for the longest numer If and only if the number field have both of min= and max=. Otherwise, the number field should have the same width as the default width of type=text.

[Shinya Kawanaka]

Created attachment 102434 [details]
Patch

[Shinya Kawanaka]

Sorry, this patch is not the final version yet.

Kent, could you let me know how to check width property is defined or not?
This should be used in HTMLInputType::shouldAvoidReformat().

The other code should be reviewable though.

[Darin Adler]

Comment on attachment 102434 [details]
Patch

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

> LayoutTests/fast/forms/input-number-size.html:19
> +    text.size = size;
> +    number.step = step;
> +    number.min = min;
> +    number.max = max;
> +    shouldBe('number.offsetWidth', 'text.offsetWidth');

This can be structured so the test output is much nicer! The test output should include the values of size, step, min, and max. That can be done by making a function that sets up the text and number and then computes the width like this:

    shouldBe('numberWidth(' + step + ', ' + min + ', ' + max + ')', 'textWidth(' + size + ')');

Then you just write the numberWidth and textWidth functions:

    function textWidth(size)
    {
        text.size  = size;
        return text.offsetWidth;
    }

If you do that, the test output will be informative instead of just a long row of PASS PASS PASS.

> Source/WebCore/html/HTMLInputElement.cpp:998
> +int HTMLInputElement::possibleContentSize() const

What is "possible content size"? Is it the maximum size that would be needed for the largest possible allowed content? I understand that size means "number of characters", and "content" means "text in the input element", but what does the word "possible" mean here? This name is not clear.

> Source/WebCore/html/NumberInputType.cpp:61
> +    double absValue = fabs(value);

No reason to abbreviate here. How about "absoluteValue" instead of "absValue".

> Source/WebCore/html/NumberInputType.cpp:65
> +    unsigned length = static_cast<int>(log10(floor(absValue))) + 1;

Why cast to an int and then convert to an unsigned?

> Source/WebCore/html/NumberInputType.cpp:145
> +    unsigned minValueDecimalPlaces, maxValueDecimalPlaces, stepValueDecimalPlaces;
> +    double minValueDouble, maxValueDouble, stepValueDouble;

We don’t declare multiple variables on a single line in WebKit code.

> Source/WebCore/html/NumberInputType.cpp:354
> +bool NumberInputType::shouldAvoidReformat()

"should avoid reformat" is not good grammar. And I have no idea what it means. What is a "reformat"?

> Source/WebCore/html/NumberInputType.cpp:356
> +    // TODO:

?

> Source/WebCore/platform/text/LocalizedNumber.h:54
> +String getLocalizedDecimalPoint();

We don’t use get in function names like this in WebKit.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:429
> +    if (RenderBox* spinRenderer = spinButton ? spinButton->renderBox() : 0)
> +        result += spinRenderer->borderLeft() + spinRenderer->borderRight() +
> +                  spinRenderer->paddingLeft() + spinRenderer->paddingRight();

We don’t line up subsequent lines like this. We use braces if there are multiple lines in an if statement body.

I don’t see any coverage of this code change in the test case.

[Kent Tamura]

Comment on attachment 102434 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +
> +        * fast/forms/input-number-size-expected.txt: Added.
> +        * fast/forms/input-number-size.html: Added.
> +

I think we need to update some others tests in fast/forms/.

> Source/WebCore/ChangeLog:8
> +        The input[type=number] element should be as wide as necessary to show the widest possible value.
> +        https://bugs.webkit.org/show_bug.cgi?id=60673
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/forms/input-number-size.html

You should explain behaviors which the patch will introduce.

> Source/WebCore/html/NumberInputType.cpp:358
> +    if (element()->renderStyle() && !element()->renderStyle()->width().isUndefined())
> +        return true;

!element()->renderStyle()->width().isAuto() is probably what you want.

[Shinya Kawanaka]

Created attachment 102518 [details]
Patch

[Shinya Kawanaka]

Created attachment 102525 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #6)
> I think we need to update some others tests in fast/forms/.

I have confirmed that all tests in fast/form/* passes with my patch.

[Kent Tamura]

Comment on attachment 102525 [details]
Patch

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

> LayoutTests/fast/forms/input-number-size.html:60
> +number.size = 10;
> +shouldBe('number.offsetWidth', 'text.offsetWidth');

These lines should be:
  shouldBe('number.size = 10; number.offsetWidth', 'text.offsetWidth');
to show what is tested.

> LayoutTests/fast/forms/input-number-size.html:63
> +number.size = 100;
> +shouldBe('number.offsetWidth', 'text.offsetWidth');

ditto.

> LayoutTests/fast/forms/input-number-size.html:81
> +shouldBe('numberWidth(0, 10, 1, "style1")', 'textWidth(2) + 20');

Why +20?
- You should define variable for the style1 border width.  e.g. var style1Borders = 20;, and use it
- This test case means the text edit area of the number field doesn't have enough width for two letters because of the width of the spin button, right?

> Source/WebCore/ChangeLog:9
> +        The size of input[type=number] is calculated from min/max/step attributes to show the widest possible value.
> +        If min or max attribute is absent, the default size is used.

You had better explain the behavior of setting min/max/step attributes while the field is shown.

> Source/WebCore/html/HTMLInputElement.cpp:1004
> +int HTMLInputElement::preferredSize() const
> +{
> +    if (m_inputType->shouldRespectSizeAttribute())
> +        return size();
> +
> +    return m_inputType->maxNumberOfCharacters(defaultSize);
> +}

It seems we don't need to introduce two new functions for InputType. We can remove shouldRespectSizeAttribute() and maxNumberOfCharacters(), and add InputType::preferredSize().

> Source/WebCore/html/NumberInputType.cpp:149
> +    if (minValueDouble < -limit || limit < minValueDouble)
> +        return defaultSize;

This limit check is already done in parseToDoubleForNumberType*().

> Source/WebCore/html/NumberInputType.cpp:157
> +    if (maxValueDouble < -limit || limit < maxValueDouble)
> +        return defaultSize;

ditto.

> Source/WebCore/html/NumberInputType.cpp:172
> +    if (stepValue.isEmpty()) {
> +        stepValueDouble = 1;
> +        stepValueDecimalPlaces = 0;
> +    } else {
> +        if (!parseToDoubleForNumberTypeWithDecimalPlaces(stepValue, &stepValueDouble, &stepValueDecimalPlaces))
> +            return defaultSize;

if the step attribute value is an invalid string, the effective step value is "1".  Returning defaultSize looks wrong.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:430
> +    HTMLElement* spinButton = innerSpinButtonElement();
> +    if (RenderBox* spinRenderer = spinButton ? spinButton->renderBox() : 0) {
> +        result += spinRenderer->borderLeft() + spinRenderer->borderRight() +
> +                  spinRenderer->paddingLeft() + spinRenderer->paddingRight();
> +    }

Why you add only borders and paddings, and not the content of the spin button?

[Shinya Kawanaka]

Created attachment 103047 [details]
Patch

[Kent Tamura]

Comment on attachment 103047 [details]
Patch

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

> LayoutTests/fast/forms/input-number-size.html:44
> +// The width of spin button should differ by environment. So it should be calculated here.
> +var spinButtonWidth = number.offsetWidth - numberWithoutSpinButton.offsetWidth;

I think spinButtonWidth is 0 because these widths are same.

[WebKit Review Bot]

Comment on attachment 103047 [details]
Patch

Attachment 103047 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306858

New failing tests:
fast/forms/input-appearance-spinbutton-layer.html
fast/speech/input-appearance-numberandspeech.html
fast/forms/input-widths.html
fast/forms/input-appearance-spinbutton-disabled-readonly.html
fast/forms/input-appearance-number-rtl.html
fast/forms/input-number-size.html
fast/forms/input-appearance-spinbutton-visibility.html

[Kent Tamura]

Comment on attachment 103047 [details]
Patch

r- because of regressions.

[Shinya Kawanaka]

Created attachment 103208 [details]
Patch

[Kent Tamura]

Comment on attachment 103208 [details]
Patch

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

> Source/WebCore/html/InputType.h:146
> +    virtual bool sizeShouldIncludeDecoration(int defaultSize, int* preferredSize) const;

We had better change the argument type from int* to int&.  We can remove null-checks by it.

[Shinya Kawanaka]

Created attachment 103221 [details]
Patch

[Kent Tamura]

Comment on attachment 103221 [details]
Patch

ok.
Thank you for woking on this.

[WebKit Review Bot]

Comment on attachment 103221 [details]
Patch

Clearing flags on attachment: 103221

Committed r92589: <http://trac.webkit.org/changeset/92589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.