Implement minlength for <input> and <textarea>.

Implement minlength for <input> and <textarea>.
Add layout tests to test minlength behavior and new tooShort validity property. Enrich maxlength/tooLong layout tests while at it.

BUG=390485
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183454

[keishi, 2014-08-01 03:14:54]

We are the first browsers to support minLength so I think we should be strict so
we don't allow any bad pages with minLength > maxLength. If we don't clamp
minLength to maxLength, the user will only be able to input maxLength characters
and won't be able to submit the form, forcing the web author to fix it.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:265: if (!isTextType())
We don't have WMLInputElement anymore so I think we can move this to
BaseTextInputType
https://bugs.webkit.org/show_bug.cgi?id=42814

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1719: if (m_minLength > maxLength)
We should just apply the rules for "parsing non-negative integers" here and not
care about minLength.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1736: if (minLength > m_maxLength)
We should just parse the minLength value here and not care about maxLength.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:457: int max = maxLength();
We don't need to clamp to maxLength

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:519: // TODO(bartekn): Add a message
for tooShort().
I think you need to land the blink side change first (add to
WebLocalizedString.h)
And then land the chromium side chnges (blink_platform_impl.cc and
webkit_strings.grd)

[Bartek Nowierski, 2014-10-03 12:52:00]

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:265: if (!isTextType())
On 2014/08/01 03:14:54, keishi wrote:
> We don't have WMLInputElement anymore so I think we can move this to
> BaseTextInputType
> https://bugs.webkit.org/show_bug.cgi?id=42814

The code has been moved to forms/BaseTextInputType.cpp and looks this has
disappeared there.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1719: if (m_minLength > maxLength)
On 2014/08/01 03:14:54, keishi wrote:
> We should just apply the rules for "parsing non-negative integers" here and
not
> care about minLength. 

I assume you want me to remove this part. Done.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLInputEl...
Source/core/html/HTMLInputElement.cpp:1736: if (minLength > m_maxLength)
On 2014/08/01 03:14:54, keishi wrote:
> We should just parse the minLength value here and not care about maxLength.

Done.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:457: int max = maxLength();
On 2014/08/01 03:14:54, keishi wrote:
> We don't need to clamp to maxLength

Done.

https://codereview.chromium.org/435753003/diff/1/Source/core/html/HTMLTextAre...
Source/core/html/HTMLTextAreaElement.cpp:519: // TODO(bartekn): Add a message
for tooShort().
On 2014/08/01 03:14:54, keishi wrote:
> I think you need to land the blink side change first (add to
> WebLocalizedString.h)
> And then land the chromium side chnges (blink_platform_impl.cc and
> webkit_strings.grd)

I'll do it in the next patchset. Let's focus on getting this working first :)

[keishi, 2014-10-06 03:58:23]

This needs layout tests. Something like ValidityState-tooLong-input.html and
ValidityState-tooLong-textarea.html.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1318:
exceptionState.throwDOMException(IndexSizeError, "The value provided (" +
String::number(maxLength) + ") is less than minlength" +
String::number(m_minLength) + ".");
nit: Maybe we can use indexExceedsMinimumBound so we have uniform error
messages.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1328:
exceptionState.throwDOMException(IndexSizeError, "The value provided (" +
String::number(minLength) + ") is more than maxlength" +
String::number(m_maxLength) + ".");
Ditto.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:517: return "Too short"; // DO NOT
CHECK IN!!!
Ditto.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:519: //     return
locale().validationMessageTooShortText(computeLengthForSubmission(value()),
minLength());
Ditto.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
File Source/core/html/forms/InputType.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:379: return "Too short"; // DO NOT CHECK
IN!!!
We don't need this comment. "check in" in Blink probably means commit to the
repo so this isn't accurate.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:381: //     return
locale().validationMessageTooShortText(value().length(), element.minLength());
We don't need this line. Blink doesn't keep code in comments.

[Bartek Nowierski, 2014-10-06 13:18:17]

I held off on layout tests until I had confidence that I'm going in the right
direction with this change. I just added one. I'm still working on adding more,
but publishing what I have since it significantly changed since the last time.
Up to you if you want to review now or wait until I'm done.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1318:
exceptionState.throwDOMException(IndexSizeError, "The value provided (" +
String::number(maxLength) + ") is less than minlength" +
String::number(m_minLength) + ".");
On 2014/10/06 03:58:23, keishi wrote:
> nit: Maybe we can use indexExceedsMinimumBound so we have uniform error
> messages.

Done.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:1328:
exceptionState.throwDOMException(IndexSizeError, "The value provided (" +
String::number(minLength) + ") is more than maxlength" +
String::number(m_maxLength) + ".");
On 2014/10/06 03:58:23, keishi wrote:
> Ditto.

Done.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:517: return "Too short"; // DO NOT
CHECK IN!!!
On 2014/10/06 03:58:23, keishi wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/HTMLTex...
Source/core/html/HTMLTextAreaElement.cpp:519: //     return
locale().validationMessageTooShortText(computeLengthForSubmission(value()),
minLength());
On 2014/10/06 03:58:23, keishi wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
File Source/core/html/forms/InputType.cpp (right):

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:379: return "Too short"; // DO NOT CHECK
IN!!!
On 2014/10/06 03:58:23, keishi wrote:
> We don't need this comment. "check in" in Blink probably means commit to the
> repo so this isn't accurate.

That was just a note to self and to the reviewer that this line must not  be
submitted to the main repository

https://codereview.chromium.org/435753003/diff/40001/Source/core/html/forms/I...
Source/core/html/forms/InputType.cpp:381: //     return
locale().validationMessageTooShortText(value().length(), element.minLength());
On 2014/10/06 03:58:23, keishi wrote:
> We don't need this line. Blink doesn't keep code in comments.

Similarly here, this wasn't meant to be submitted to the main repository. That
was just a note to myself to uncomment this line once I implement the localized
message.

[keishi, 2014-10-08 09:17:49]

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-tooLong-input.html (right):

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
Changes to LayoutTests/fast/forms/ValidityState-tooLong-input.html and
LayoutTests/fast/forms/ValidityState-tooLong-textarea.html look unrelated to the
main change. Could they be split into a separate CL?

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:59: // TODO: The
grapheme test below doesn't do its job because initial value is
nit: style guideline says to use FIXME instead of TODO

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:61: // see that the test
would fail, which possibly reveals a code issue.
nit: This looks like a bug so maybe you can file a bug at crbug.com and add a
link here.

[Bartek Nowierski, 2014-10-09 05:32:56]

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-tooLong-input.html (right):

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
On 2014/10/08 09:17:49, keishi wrote:
> Changes to LayoutTests/fast/forms/ValidityState-tooLong-input.html and
> LayoutTests/fast/forms/ValidityState-tooLong-textarea.html look unrelated to
the
> main change. Could they be split into a separate CL?

They kind of are, kind of aren't. My goal was to have minlength and maxlength
tests equivalent. Since I added some extra checks on the minlength side, I
ported them on the maxlength side as well.

I'd prefer to keep this file in, particularly that it mentions adding a grapheme
test to ValidityState-tooShort-input.html once its fixed.

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:59: // TODO: The
grapheme test below doesn't do its job because initial value is
On 2014/10/08 09:17:49, keishi wrote:
> nit: style guideline says to use FIXME instead of TODO

Done.

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:61: // see that the test
would fail, which possibly reveals a code issue.
On 2014/10/08 09:17:49, keishi wrote:
> nit: This looks like a bug so maybe you can file a bug at http://crbug.com and
add a
> link here.

Done. https://code.google.com/p/chromium/issues/detail?id=421727

[keishi, 2014-10-09 05:59:54]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2014-10-09 05:59:55]

LGTM

+tkent

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-tooLong-input.html (right):

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
On 2014/10/09 05:32:56, Bartek Nowierski wrote:
> On 2014/10/08 09:17:49, keishi wrote:
> > Changes to LayoutTests/fast/forms/ValidityState-tooLong-input.html and
> > LayoutTests/fast/forms/ValidityState-tooLong-textarea.html look unrelated to
> the
> > main change. Could they be split into a separate CL?
> 
> They kind of are, kind of aren't. My goal was to have minlength and maxlength
> tests equivalent. Since I added some extra checks on the minlength side, I
> ported them on the maxlength side as well.
> 
> I'd prefer to keep this file in, particularly that it mentions adding a
grapheme
> test to ValidityState-tooShort-input.html once its fixed.

Okay, but could you add a blurb in the CL description then.

[Bartek Nowierski, 2014-10-09 06:25:57]

bartekn@chromium.org changed reviewers:
+ haraken@chromium.org

[Bartek Nowierski, 2014-10-09 06:26:59]

+haraken to approve file changes under platform/text/

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/ValidityState-tooLong-input.html (right):

https://codereview.chromium.org/435753003/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/ValidityState-tooLong-input.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
On 2014/10/09 05:59:55, keishi wrote:
> On 2014/10/09 05:32:56, Bartek Nowierski wrote:
> > On 2014/10/08 09:17:49, keishi wrote:
> > > Changes to LayoutTests/fast/forms/ValidityState-tooLong-input.html and
> > > LayoutTests/fast/forms/ValidityState-tooLong-textarea.html look unrelated
to
> > the
> > > main change. Could they be split into a separate CL?
> > 
> > They kind of are, kind of aren't. My goal was to have minlength and
maxlength
> > tests equivalent. Since I added some extra checks on the minlength side, I
> > ported them on the maxlength side as well.
> > 
> > I'd prefer to keep this file in, particularly that it mentions adding a
> grapheme
> > test to ValidityState-tooShort-input.html once its fixed.
> 
> Okay, but could you add a blurb in the CL description then.

Done.

[haraken, 2014-10-09 07:37:20]

LGTM for platform/.

[Bartek Nowierski, 2014-10-09 07:39:09]

The CQ bit was checked by bartekn@chromium.org

[commit-bot: I haz the power, 2014-10-09 07:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/435753003/120001

[commit-bot: I haz the power, 2014-10-09 08:36:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 08:36:44]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Bartek Nowierski, 2014-10-09 09:37:36]

The CQ bit was checked by bartekn@chromium.org

[commit-bot: I haz the power, 2014-10-09 09:38:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/435753003/120001

[commit-bot: I haz the power, 2014-10-09 10:33:01]

Committed patchset #7 (id:120001) as 183454

[tkent, 2014-10-15 00:50:25]

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.h:362: int m_minLength;
I don't think we need new data member.  We should try to parse minlength
attribute value on demand.  If it has performance problems, we should introduce
m_minLength then.

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:459: int value =
getAttribute(minlengthAttr).string().toInt(&ok);
should use parseHTMLInteger() like HTMLInputElement::parseMinLengthAttribute().

The maxlength() above is not good too.

[Bartek Nowierski, 2014-10-16 04:55:14]

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLIn...
File Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLIn...
Source/core/html/HTMLInputElement.h:362: int m_minLength;
On 2014/10/15 00:50:25, tkent wrote:
> I don't think we need new data member.  We should try to parse minlength
> attribute value on demand.  If it has performance problems, we should
introduce
> m_minLength then.

Someone decided that it's worth doing for maxlength, so just for symmetry we
might as well do it for minlength. The advantage of this approach is that it's
obvious that if one is correct then the other is correct too.

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/435753003/diff/120001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:459: int value =
getAttribute(minlengthAttr).string().toInt(&ok);
On 2014/10/15 00:50:25, tkent wrote:
> should use parseHTMLInteger() like
HTMLInputElement::parseMinLengthAttribute().
> 
> The maxlength() above is not good too.

Good point! I'll change it for both minLength and maxLength.