Bug 60673 - <input type=number> doesn't ignore size="" attribute
Summary: <input type=number> doesn't ignore size="" attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-11 16:24 PDT by Ian 'Hixie' Hickson
Modified: 2011-08-08 02:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.21 KB, patch)
2011-07-30 06:21 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (23.17 KB, patch)
2011-08-01 07:56 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (23.03 KB, patch)
2011-08-01 09:37 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (24.98 KB, patch)
2011-08-05 00:26 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.54 KB, patch)
2011-08-07 23:51 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.45 KB, patch)
2011-08-08 01:38 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian 'Hixie' Hickson 2011-05-11 16:24:06 PDT
<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
Comment 1 Shinya Kawanaka 2011-07-26 01:59:23 PDT
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?
Comment 2 Kent Tamura 2011-07-26 02:16:09 PDT
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.
Comment 3 Shinya Kawanaka 2011-07-30 06:21:55 PDT
Created attachment 102434 [details]
Patch
Comment 4 Shinya Kawanaka 2011-07-30 06:23:57 PDT
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.
Comment 5 Darin Adler 2011-07-30 10:53:45 PDT
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.
Comment 6 Kent Tamura 2011-07-31 23:13:18 PDT
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.
Comment 7 Shinya Kawanaka 2011-08-01 07:56:45 PDT
Created attachment 102518 [details]
Patch
Comment 8 Shinya Kawanaka 2011-08-01 09:37:03 PDT
Created attachment 102525 [details]
Patch
Comment 9 Shinya Kawanaka 2011-08-01 09:47:04 PDT
(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.
Comment 10 Kent Tamura 2011-08-01 22:15:57 PDT
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?
Comment 11 Shinya Kawanaka 2011-08-05 00:26:33 PDT
Created attachment 103047 [details]
Patch
Comment 12 Kent Tamura 2011-08-05 00:42:16 PDT
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.
Comment 13 WebKit Review Bot 2011-08-05 07:14:20 PDT
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
Comment 14 Kent Tamura 2011-08-05 07:15:41 PDT
Comment on attachment 103047 [details]
Patch

r- because of regressions.
Comment 15 Shinya Kawanaka 2011-08-07 23:51:52 PDT
Created attachment 103208 [details]
Patch
Comment 16 Kent Tamura 2011-08-08 00:09:08 PDT
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.
Comment 17 Shinya Kawanaka 2011-08-08 01:38:25 PDT
Created attachment 103221 [details]
Patch
Comment 18 Kent Tamura 2011-08-08 01:42:30 PDT
Comment on attachment 103221 [details]
Patch

ok.
Thank you for woking on this.
Comment 19 WebKit Review Bot 2011-08-08 02:17:07 PDT
Comment on attachment 103221 [details]
Patch

Clearing flags on attachment: 103221

Committed r92589: <http://trac.webkit.org/changeset/92589>
Comment 20 WebKit Review Bot 2011-08-08 02:17:13 PDT
All reviewed patches have been landed.  Closing bug.