[Webkit-unassigned] [Bug 60673] <input type=number> doesn't ignore size="" attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60673


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #102434|review?                     |review-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2011-07-30 10:53:45 PST ---
(From update of attachment 102434)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.