Bug 50617 – ValidityState's exposed functions should check if willValidate() is true before all

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

[Dai Mikurube]

ValidityState's exposed functions (e.g. typeMismatch(), patternMismatch(), ...) should check if willValidate() is true before all.
If willValidate() is false, these functions should return false and it should not call any sub functions (e.g. HTMLInputElement::patternMismatch()).

This bug was discussed at the bug 50380.

[Dai Mikurube]

Created attachment 75801 [details]
Patch

[Dai Mikurube]

(In reply to comment #1)
> Created an attachment (id=75801) [details]
> Patch

It requires rebasing... withdrawn.

[Dai Mikurube]

Created attachment 75862 [details]
Patch

[Kent Tamura]

Comment on attachment 75862 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * fast/forms/ValidityState-patternMismatch-008-expected.txt: Added.
> +        * fast/forms/ValidityState-patternMismatch-008.html: Added.

"008" represents nothing about the content.  Do not add files with such cryptic names.
Existing ValidityState-patternMismatch-00x should be renamed and we can merge them into one.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-date.js:28
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

We don't need to add "disabled" parameter to shouldBeInvalid().  It is not used.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-datetime.js:28
> -function shouldBeInvalid(value)
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

ditto.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-datetimelocal.js:28
> -function shouldBeInvalid(value)
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

ditto.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-month.js:28
> -function shouldBeInvalid(value)
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

ditto.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-time.js:28
> -function shouldBeInvalid(value)
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

ditto.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-url.js:23
> -function expectInvalid(value) {
> -    check(value, true);
> +function expectInvalid(value, disabled) {
> +    check(value, true, disabled);

ditto.

> LayoutTests/fast/forms/script-tests/ValidityState-typeMismatch-week.js:28
> -function shouldBeInvalid(value)
> +function shouldBeInvalid(value, disabled)
>  {
> -    check(value, true);
> +    check(value, true, disabled);

ditto.

> WebCore/ChangeLog:12
> +        * html/ValidityState.cpp:

We need to update ValidityState::customError() in ValidityState.h too.

[Dai Mikurube]

(In reply to comment #4)
Similar fix for customError() causes failure in LayoutTests for <button> and <fieldset>.

It's because their recalcWillValidate() always return false. It results validity.willValidate is always false for <button> and <fieldset>. But it conflicts the test ValidityState-customError-001.html.

Do you know the reason?

[Dai Mikurube]

(In reply to comment #5)
In addition, reading http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#barred-from-constraint-validation , <fieldset> is barred from constraint validation, but <button> is not.

In my understanding, the test should be fixed for <fieldset>, and recalcWillValidate() should be fixed for <button>. What do you think about it?

[Kent Tamura]

Well, please fix the test for <fieldset>.

<button> should support validation only if type=submit.  The current code is incorrect.  Please fix the code.
Also, <input type=submit> should support validation according to the standard.  So please fix SubmitInputType.cpp and add a test.

[Dai Mikurube]

Created attachment 76162 [details]
Patch

[Dai Mikurube]

(In reply to comment #7)
Ok,
- Fixed the test for <fieldset>, <button> and <input type="submit">. Merged ValidityState-customError-*.html into one.
- Fixed willValidate() and related functions for <button>.
- Fixed willValidate() and related functions for <input type="submit">.

[Kent Tamura]

Comment on attachment 76162 [details]
Patch

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

The patch is good enough for r+.  However I'd like to ask splitting the patch into two:
 1. merge existing tests
 2. fix willValidate-related bugs.

> LayoutTests/fast/forms/script-tests/ValidityState-rangeOverflow.js:12
> +    if (disabled)
> +        input.disabled = true;
> +    else
> +        input.disabled = false;

You can write "input.disabled = !!disabled".

[Dai Mikurube]

(In reply to comment #10)
Ok, I'm splitting. Should I make a new bug for the patch merging tests?

[Kent Tamura]

(In reply to comment #11)
> (In reply to comment #10)
> Ok, I'm splitting. Should I make a new bug for the patch merging tests?

Creating another bug is better.

[Dai Mikurube]

(In reply to comment #12)
I've created a new bug: https://bugs.webkit.org/show_bug.cgi?id=50802

[Dai Mikurube]

Created attachment 76205 [details]
Patch

[Kent Tamura]

Comment on attachment 76205 [details]
Patch

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

> LayoutTests/fast/forms/ValidityState-patternMismatch.html:66
> -/><input id="empty-pattern" type="text" pattern="" value="Lorem Ipsum"
> -/><input id="disabled" pattern="[0-9][A-Z]{3}" value="00AA" disabled />
> +
> +<input id="simple" pattern="[0-9][A-Z]{3}" value="0AAA"/>
> +
> +<input id="no-pattern-and-no-value" />
> +
> +<input id="ip-address" type="text" pattern="\b(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b" value="127.0.0.1" />
> +<input id="email-address" type="text" pattern="[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?" value="[e-mail]" />
> +<input id="wrong-email-address" type="text" pattern="[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?" value="Wrong!"/>
> +
> +<input id="match-1" type="text" pattern="\b(word1|word2|word3)(?:\W+\w+){1,6}?\W+(word1|word2|word3)\b" value="word1 near word2" />
> +<input id="match-2" type="text" pattern=".{5,}" value="12345" />
> +<input id="match-3" type="text" pattern=".{5,}" value="*(@$!" />
> +<input id="match-4" type="text" pattern=".{5,}" value="*(@^$!" />
> +<input id="match-5" type="text" pattern="0|1|2|3|4|5|6|7|8|" value="3" />
> +<input id="match-6" type="text" pattern="^(foo).*" value="foo"/>
> +<input id="match-7" type="text" pattern="^(foo).*" value="fooo"/>
> +<input id="match-8" type="text" pattern="\w" value="f"/>
> +<input id="match-9" type="text" pattern="\." value="."/>
> +<input id="match-10" type="text" pattern="\)\(" value=")("/>
> +<input id="match-11" type="text" pattern="ab" value="ab"/>
> +<input id="match-12" type="text" pattern="^ab" value="ab"/>
> +<input id="match-13" type="text" pattern="^\(" value="("/>
> +<input id="match-14" type="text" pattern="...\)\(..." value="aaa)(bbb"/>
> +<input id="match-15" type="text" pattern="^" value=""/>
> +<input id="match-16" type="text" pattern="$" value=""/>
> +<input id="match-17" type="text" pattern="foobar" value=""/>
> +
> +<input id="wrong-gray-or-grey" type="text" pattern="gr[ae]y" value="Wrong!"/>
> +<input id="gray" type="text" pattern="gr[ae]y" value="gray"/>
> +<input id="grey" type="text" pattern="gr[ae]y" value="grey"/>
> +<input id="empty-gray-or-grey" type="text" pattern="gr[ae]y" value=""/>
> +
> +<input id="mismatch-1" type="text" pattern="((4\.[0-3])|(2\.[0-3]))" value="Something 4.0"/>
> +<input id="mismatch-2" type="text" pattern="\w" value="*"/>
> +<input id="mismatch-3" type="email" pattern="[0-9]" value="something"/>
> +<input id="mismatch-4" type="text" pattern=".{5,}" value="1234" />
> +<input id="mismatch-5" type="text" pattern=".{5,}" value="*)$!" />
> +<input id="mismatch-6" type="text" pattern=".{5,}" value="(^$!" />
> +<input id="mismatch-7" type="text" pattern="0|1|2|3|4|5|6|7|8|" value="a" />
> +<input id="mismatch-8" type="text" pattern="^(foo).*" value="a foo"/>
> +<input id="mismatch-9" type="text" pattern="\w" value="134"/>
> +<input id="mismatch-10" type="text" pattern="\." value="\."/>
> +<input id="mismatch-11" type="text" pattern="\)\(" value=") ("/>
> +<input id="mismatch-12" type="text" pattern="ab" value="a"/>
> +<input id="mismatch-13" type="text" pattern="ab" value="b"/>
> +<input id="mismatch-14" type="text" pattern="^ab" value="abc"/>
> +<input id="mismatch-15" type="text" pattern="^\(" value="(something"/>
> +<input id="mismatch-16" type="text" pattern="\)\\" value="something)\"/>
> +<input id="mismatch-17" type="text" pattern="...\)\([b]{3}" value="adf)(bbbTEST"/>
> +<input id="mismatch-18" type="text" pattern="foo\\" value="food"/>
> +<input id="mismatch-19" type="text" pattern="^" value="wrong"/>
> +<input id="mismatch-20" type="text" pattern="$" value="wrong"/>
> +
> +<input id="empty-pattern" type="text" pattern="" value="Lorem Ipsum"/>
> +
> +<input id="disabled" pattern="[0-9][A-Z]{3}" value="00AA" disabled />
> +

You don't need to change this block, do you?

> WebCore/ChangeLog:9
> +        ValidityState's exposed functions should check if willValidate() is true before all
> +        https://bugs.webkit.org/show_bug.cgi?id=50617
> +
> +        Added checking willValidate() to exposed functions.
> +

We should mention the submit button behavior change.

> WebCore/ChangeLog:12
> +        (WebCore::HTMLButtonElement::parseMappedAttribute): Added calling setNeedsWillValidateCheck().
> +        (WebCore::HTMLButtonElement::recalcWillValidate): Added.

It's obvious you added them.  You should write why you needed to add them.

> WebCore/ChangeLog:15
> +        (WebCore::SubmitInputType::supportsValidation): Removed.

You should write the reason.

> WebCore/html/HTMLButtonElement.cpp:188
> +    return !disabled() && !readOnly();

We should use HTMLFormControlElement::recalcWillValidate().
  return m_type == SUBMIT && HTMLFormControlElement::recalcWillValidate();

> WebCore/html/ValidityState.cpp:209
> +    if (!element->willValidate())
> +        return false;
> +
> +    return !m_customErrorMessage.isEmpty();

nit: We can write "return element->willValidate() && !m_customErrorMessage.isEmpty();"

[Dai Mikurube]

Comment on attachment 76205 [details]
Patch

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

>> LayoutTests/fast/forms/ValidityState-patternMismatch.html:66
>> +
> 
> You don't need to change this block, do you?

The block before this change is in an unintended newline style from the bug 50802. Actually, I chose the one with more natural style in rebasing and merging.
I can withdraw this change. It may be better to create another bug for refactoring it.

[Dai Mikurube]

Created attachment 76303 [details]
Patch

[Kent Tamura]

(In reply to comment #16)
> (From update of attachment 76205 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76205&action=review
> 
> >> LayoutTests/fast/forms/ValidityState-patternMismatch.html:66
> >> +
> > 
> > You don't need to change this block, do you?
> 
> The block before this change is in an unintended newline style from the bug 50802. Actually, I chose the one with more natural style in rebasing and merging.
> I can withdraw this change. It may be better to create another bug for refactoring it.

pfeldman made it by http://trac.webkit.org/changeset/73720 .
You should ask his intention.  Anyway, this change should not be in this patch.

[Dai Mikurube]

(In reply to comment #18)
> pfeldman made it by http://trac.webkit.org/changeset/73720 .
> You should ask his intention.  Anyway, this change should not be in this patch.

I misunderstood that this change was made automatically... I removed my change on it.

Then, pfeldman's change looks to remove some space characters in the output. Newline characters and whitespace characters in the test HTML result in some whitespace characters in the LayoutTests output.

[Dai Mikurube]

Created attachment 76357 [details]
Patch

[Kent Tamura]

Comment on attachment 76357 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 76357 [details]
Patch

Rejecting attachment 76357 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
 ....................................................................................................................................................................................................................................................................
fast/css/pseudo-required-optional-005.html -> failed

Exiting early after 1 failures. 6580 tests run.
125.24s total testing time

6579 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7024085

[Dai Mikurube]

Created attachment 76362 [details]
Patch

[Dai Mikurube]

(In reply to comment #22)
Sorry, modified the tests :
- fast/css/pseudo-required-optional-005.html
- fast/css/pseudo-valid-unapplied.html

[Kent Tamura]

Comment on attachment 76362 [details]
Patch

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

> LayoutTests/fast/css/pseudo-required-optional-005.html:28
> +shouldBeFalse('isGreenFor("submit")');

This is wrong.  Submit button doesn't support "required" attribute.  So ":optional" class should be applied to the element.

Also, the name "isGreenFor" is not good.  "isOptional" would be helpful.

> LayoutTests/fast/css/pseudo-valid-unapplied.html:69
> +shouldBe('getBackgroundColor("input-submit")', "invalidColor");
> +shouldBe('getBackgroundColor("input-image")', "normalColor");
> +shouldBe('getBackgroundColor("fieldset")', "normalColor");
> +shouldBe('getBackgroundColor("object")', "normalColor");
> +shouldBe('getBackgroundColor("button")', "invalidColor");

This test file is for "unapplied" cases.  Input-submit and button should be moved to another test file.

[Dai Mikurube]

Created attachment 76490 [details]
Patch

[Dai Mikurube]

(In reply to comment #26)
Sorry, I wrongly pressed enter... It includes unnecessary changes on descriptions and whitespace.

[Kent Tamura]

Comment on attachment 76490 [details]
Patch

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

> LayoutTests/fast/css/pseudo-valid-unapplied.html:-29
> -<button name="button">Lorem ipsum</button>

We should have test cases for <button type=reset> and <button type=button> instead.

[Dai Mikurube]

Created attachment 76492 [details]
Patch

[Kent Tamura]

Comment on attachment 76492 [details]
Patch

Looks good!

[WebKit Review Bot]

Comment on attachment 76492 [details]
Patch

Clearing flags on attachment: 76492

Committed r73999: <http://trac.webkit.org/changeset/73999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.