Bug 154906 – Align HTMLInputElement.maxLength with the specification

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Align HTMLInputElement.maxLength with the specification:
- https://html.spec.whatwg.org/multipage/forms.html#dom-input-maxlength
- https://html.spec.whatwg.org/multipage/forms.html#attr-input-maxlength

[Chris Dumez]

Created attachment 272632 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 272632 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +

You should probably mention that textarea's maxLength already returned -1 before this change.
Or did you change that recently also?  If so, it would be nice to refer to the revision in the change log.

> Source/WebCore/html/HTMLInputElement.cpp:1260
> +    return std::min(static_cast<unsigned>(m_maxLength), maximumLength);

Should we just check m_maxLength > maximumLength in the above if instead of using std::max here?
That might read better overall.

[Darin Adler]

Comment on attachment 272632 [details]
Patch

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

Even if the public API uses negative numbers as magic values, it seems to me would could do some cleaner stuff inside WebKit itself. Some suggestions below:

> Source/WebCore/html/HTMLInputElement.cpp:-99
> -    , m_maxLength(maximumLength)

I think it’s really confusing to have m_maxLength and maximumLength. Those are the same names, but they are two different concepts. I think maximumLength should be renamed maxEffectiveLength, maximumEffectiveLength, maxEffectiveMaxLength, or maximumEffectiveMaxLength.

> Source/WebCore/html/HTMLInputElement.cpp:1258
> +    if (m_maxLength < 0)
> +        return maximumLength;

A cleaner, albeit less efficient, way to do all of this would be to use Optional<int> for m_maxLength and friends, and implement the -1 magic value meaning "no length" only in the maxLengthForBindings function.

If we want to do the trickier way, then I will note -1 will automatically do the right thing when converted to unsigned, so I suggest removing this code and instead writing a comment. Or you could write a static_assert and a comment, perhaps something like this:

    // The number -1 represents no maximum at all; conveniently it becomes a super-large value when converted to unsigned.
    std::static_assert(std::min<unsigned>(-1, maximumLength) == maximumLength, "-1 means no maximum");

>> Source/WebCore/html/HTMLInputElement.cpp:1260
>> +    return std::min(static_cast<unsigned>(m_maxLength), maximumLength);
> 
> Should we just check m_maxLength > maximumLength in the above if instead of using std::max here?
> That might read better overall.

I personally find the use of std::min a little clearer readable than an if statement. However, I think this is better than static_cast:

    return std::min<unsigned>(m_maxLength, maximumLength);

> Source/WebCore/html/HTMLInputElement.cpp:1743
> -    int maxLength = std::min(parseHTMLNonNegativeInteger(value).valueOr(maximumLength), maximumLength);
>      int oldMaxLength = m_maxLength;
> -    m_maxLength = maxLength;
> -    if (oldMaxLength != maxLength)
> +    m_maxLength = parsedMaxLength();
> +    if (oldMaxLength != m_maxLength)
>          updateValueIfNeeded();
>      setNeedsStyleRecalc();
>      updateValidity();

I think it would be more correct to do work only if effectiveMaxLength() changed. In fact, I am not sure why setNeedsStyleRecalc and updateValidity need to be called if effectiveMaxLength didn’t change, but it seems especially clear that updateValueIfNeeded should be called only if effectiveMaxLength changed, rather than if m_maxLength changed.

> Source/WebCore/html/HTMLTextAreaElement.h:49
> +    int effectiveMaxLength() const { return m_maxLength; }

Seems peculiar to continue using -1 here to indicate there is no maximum. Is that really OK? Might be worth returning to clean that up.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:776
> +    auto optionalMaxLength = parseHTMLNonNegativeInteger(fastGetAttribute(maxlengthAttr));
> +    if (!optionalMaxLength)
> +        return -1;
> +    return optionalMaxLength.value();

This should be a one-liner that uses valueOr(-1) if we are going to keep this.

[Chris Dumez]

Comment on attachment 272632 [details]
Patch

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

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:776
>> +    return optionalMaxLength.value();
> 
> This should be a one-liner that uses valueOr(-1) if we are going to keep this.

Yes, I did it this way for now because the other patch that allows parseHTMLNonNegativeInteger().valueOr(-1) has not landed yet.

[Chris Dumez]

Comment on attachment 272632 [details]
Patch

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

>> Source/WebCore/html/HTMLTextAreaElement.h:49
>> +    int effectiveMaxLength() const { return m_maxLength; }
> 
> Seems peculiar to continue using -1 here to indicate there is no maximum. Is that really OK? Might be worth returning to clean that up.

Yes, this is OK. unlike HTMLInputElement, HTMLTextAreaElement does not clamp the maxLength to a large value. Instead, the call sites of HTMLTextAreaElement::effectiveMaxLength() (that is HTMLTextAreaElement::tooLong() and HTMLTextAreaElement::handleBeforeTextInsertedEvent()) correctly handle -1 as a value indicating no limit.

[Chris Dumez]

Comment on attachment 272632 [details]
Patch

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

>> Source/WebCore/html/HTMLInputElement.cpp:1258
>> +        return maximumLength;
> 
> A cleaner, albeit less efficient, way to do all of this would be to use Optional<int> for m_maxLength and friends, and implement the -1 magic value meaning "no length" only in the maxLengthForBindings function.
> 
> If we want to do the trickier way, then I will note -1 will automatically do the right thing when converted to unsigned, so I suggest removing this code and instead writing a comment. Or you could write a static_assert and a comment, perhaps something like this:
> 
>     // The number -1 represents no maximum at all; conveniently it becomes a super-large value when converted to unsigned.
>     std::static_assert(std::min<unsigned>(-1, maximumLength) == maximumLength, "-1 means no maximum");

It looks like std::min() is only constexpr in C++14, I cannot seem to get this to build.

[Chris Dumez]

Created attachment 272669 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272669 [details]
Patch

Clearing flags on attachment: 272669

Committed r197458: <http://trac.webkit.org/changeset/197458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.