[webkit-reviews] review denied: [Bug 60673] <input type=number> doesn't ignore size="" attribute : [Attachment 102525] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Shinya Kawanaka
<shinyak at google.com>'s request for review:
Bug 60673: <input type=number> doesn't ignore size="" attribute
https://bugs.webkit.org/show_bug.cgi?id=60673

Attachment 102525: Patch
https://bugs.webkit.org/attachment.cgi?id=102525&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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?