Bug 50617 - ValidityState's exposed functions should check if willValidate() is true before all
Summary: ValidityState's exposed functions should check if willValidate() is true befo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 50802
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-07 00:36 PST by Dai Mikurube
Modified: 2010-12-13 21:19 PST (History)
2 users (show)

See Also:


Attachments
Patch (55.57 KB, patch)
2010-12-07 05:05 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (55.92 KB, patch)
2010-12-07 19:30 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (102.86 KB, patch)
2010-12-09 22:45 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (70.45 KB, patch)
2010-12-10 08:27 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (70.80 KB, patch)
2010-12-11 05:22 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (63.20 KB, patch)
2010-12-13 01:08 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (69.95 KB, patch)
2010-12-13 02:40 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (71.12 KB, patch)
2010-12-13 20:44 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (71.26 KB, patch)
2010-12-13 20:57 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dai Mikurube 2010-12-07 00:36:03 PST
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.
Comment 1 Dai Mikurube 2010-12-07 05:05:32 PST
Created attachment 75801 [details]
Patch
Comment 2 Dai Mikurube 2010-12-07 05:14:57 PST
(In reply to comment #1)
> Created an attachment (id=75801) [details]
> Patch

It requires rebasing... withdrawn.
Comment 3 Dai Mikurube 2010-12-07 19:30:47 PST
Created attachment 75862 [details]
Patch
Comment 4 Kent Tamura 2010-12-07 20:08:18 PST
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.
Comment 5 Dai Mikurube 2010-12-09 00:25:41 PST
(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?
Comment 6 Dai Mikurube 2010-12-09 00:31:09 PST
(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?
Comment 7 Kent Tamura 2010-12-09 01:12:21 PST
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.
Comment 8 Dai Mikurube 2010-12-09 22:45:35 PST
Created attachment 76162 [details]
Patch
Comment 9 Dai Mikurube 2010-12-09 22:48:09 PST
(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">.
Comment 10 Kent Tamura 2010-12-09 23:00:12 PST
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".
Comment 11 Dai Mikurube 2010-12-09 23:09:54 PST
(In reply to comment #10)
Ok, I'm splitting. Should I make a new bug for the patch merging tests?
Comment 12 Kent Tamura 2010-12-09 23:13:22 PST
(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.
Comment 13 Dai Mikurube 2010-12-09 23:18:16 PST
(In reply to comment #12)
I've created a new bug: https://bugs.webkit.org/show_bug.cgi?id=50802
Comment 14 Dai Mikurube 2010-12-10 08:27:38 PST
Created attachment 76205 [details]
Patch
Comment 15 Kent Tamura 2010-12-11 03:59:01 PST
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="someone@somewhere.com" />
> +<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();"
Comment 16 Dai Mikurube 2010-12-11 05:20:18 PST
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.
Comment 17 Dai Mikurube 2010-12-11 05:22:21 PST
Created attachment 76303 [details]
Patch
Comment 18 Kent Tamura 2010-12-12 17:59:08 PST
(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.
Comment 19 Dai Mikurube 2010-12-13 01:01:00 PST
(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.
Comment 20 Dai Mikurube 2010-12-13 01:08:46 PST
Created attachment 76357 [details]
Patch
Comment 21 Kent Tamura 2010-12-13 01:16:00 PST
Comment on attachment 76357 [details]
Patch

ok
Comment 22 WebKit Review Bot 2010-12-13 01:41:28 PST
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
Comment 23 Dai Mikurube 2010-12-13 02:40:58 PST
Created attachment 76362 [details]
Patch
Comment 24 Dai Mikurube 2010-12-13 02:41:50 PST
(In reply to comment #22)
Sorry, modified the tests :
- fast/css/pseudo-required-optional-005.html
- fast/css/pseudo-valid-unapplied.html
Comment 25 Kent Tamura 2010-12-13 03:21:40 PST
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.
Comment 26 Dai Mikurube 2010-12-13 20:44:14 PST
Created attachment 76490 [details]
Patch
Comment 27 Dai Mikurube 2010-12-13 20:46:43 PST
(In reply to comment #26)
Sorry, I wrongly pressed enter... It includes unnecessary changes on descriptions and whitespace.
Comment 28 Kent Tamura 2010-12-13 20:51:10 PST
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.
Comment 29 Dai Mikurube 2010-12-13 20:57:10 PST
Created attachment 76492 [details]
Patch
Comment 30 Kent Tamura 2010-12-13 21:00:22 PST
Comment on attachment 76492 [details]
Patch

Looks good!
Comment 31 WebKit Review Bot 2010-12-13 21:19:47 PST
Comment on attachment 76492 [details]
Patch

Clearing flags on attachment: 76492

Committed r73999: <http://trac.webkit.org/changeset/73999>
Comment 32 WebKit Review Bot 2010-12-13 21:19:55 PST
All reviewed patches have been landed.  Closing bug.