Bug 42814 – maxLength should not be applied to non-text types

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

maxLength should not be applied to non-text types

[Kent Tamura]

According to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#input-type-attr-summary ,
maxLength should be applied only to text, search, url, tel, email, and password types.

[Kent Tamura]

Created attachment 62283 [details]
Patch

[Darin Adler]

Comment on attachment 62283 [details]
Patch

> +var input = document.createElement('input');
> +input.maxLength = 2;
> +input.type = 'number';
> +document.body.appendChild(input);
> +input.focus();
> +document.execCommand('insertText', false, '1234');
> +shouldBe('input.value', '"1234"');

Once you've set up the basics like this, it would be better to have the test cover many cases. It's good to go past the minimum and make sure you've covered all the code. Ideally you have a test that will fail if you change anything in your patch, rather than a test that covers just the most important thing you've changed.

It's unfortunate that HTMLInputElement itself calls a virtual function for supportsMaxLength. It's true that it needs to be virtual so that InputElement can call it, but when HTMLInputElement is calling itself the extra cost is not justified. There is a programming technique that eliminates this without any special care at call sites. See Node::localName, Node::virtualLocalName, Element::localName and Element::virtualLocalName, for an example of the technique. Using the technique would make our compiled code smaller and faster, but would make the code a bit more ugly.

Something from the existing code, not new: It seems a little bit unfortunate that when invoking the regular expression engine you can't tell it that you want to match against the entire string. It ends up doing work you don't need. We could consider adding a new operation to RegularExpression.

Otherwise things look good.

[Kent Tamura]

(In reply to comment #3)
> Once you've set up the basics like this, it would be better to have the test cover many cases. It's good to go past the minimum and make sure you've covered all the code. Ideally you have a test that will fail if you change anything in your patch, rather than a test that covers just the most important thing you've changed.

I think the existing tests (not only the new one) cover this change well.

> It's unfortunate that HTMLInputElement itself calls a virtual function for supportsMaxLength. It's true that it needs to be virtual so that InputElement can call it, but when HTMLInputElement is calling itself the extra cost is not justified. There is a programming technique that eliminates this without any special care at call sites. See Node::localName, Node::virtualLocalName, Element::localName and Element::virtualLocalName, for an example of the technique. Using the technique would make our compiled code smaller and faster, but would make the code a bit more ugly.

Ok, I changed supportsMaxLength() call in HTMLInputElement to isTextType() and a comment.

Committed as r63942.