<fieldset> should be ignored in interactive validation

<fieldset> should be ignored in interactive validation

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

[tanay.c, 2015-06-23 11:15:13]

tanay.c@samsung.com changed reviewers:
+ tkent@chromium.org

[tanay.c, 2015-06-25 05:26:36]

tanay.c@samsung.com changed reviewers:
+ philipj@opera.com

[tanay.c, 2015-06-25 05:26:37]

Please have a look.

[tkent, 2015-06-25 06:16:45]

https://codereview.chromium.org/1202563009/diff/1/Source/core/html/HTMLFormEl...
File Source/core/html/HTMLFormElement.cpp (right):

https://codereview.chromium.org/1202563009/diff/1/Source/core/html/HTMLFormEl...
Source/core/html/HTMLFormElement.cpp:306: if (unhandled->isFocusable() ||
unhandled->formControlType() == "fieldset")
Here is not the correct place to check if it's <fieldset>.

The specification says fieldset is omitted in "statically validate the
constraints" step.
https://html.spec.whatwg.org/multipage/forms.html#statically-validate-the-con...
> 1. Let controls be a list of all the submittable elements whose form owner is
form, in tree order.

In our implementation, it's
HTMLFormElement::checkInvalidControlsAndCollectUnhandled().
We should add isSubmittableElement [1] to HTMLFormControlElement, and
checkInvalidControlsAndCollectUnhandled() should be:

HTMLFormControlElement* control = toHTMLFormControlElement(elements[i].get());
if (control->isSubmittableElement() &&
!control->checkValidity(unhandledInvalidControls, eventBehavior) &&
control->formOwner() == this) {
    ++invalidControlsCount;

[1] https://html.spec.whatwg.org/multipage/forms.html#category-submit

[tanay.c, 2015-06-25 10:48:57]

On 2015/06/25 06:16:45, tkent wrote:
>
https://codereview.chromium.org/1202563009/diff/1/Source/core/html/HTMLFormEl...
> File Source/core/html/HTMLFormElement.cpp (right):
> 
>
https://codereview.chromium.org/1202563009/diff/1/Source/core/html/HTMLFormEl...
> Source/core/html/HTMLFormElement.cpp:306: if (unhandled->isFocusable() ||
> unhandled->formControlType() == "fieldset")
> Here is not the correct place to check if it's <fieldset>.
> 
> The specification says fieldset is omitted in "statically validate the
> constraints" step.
>
https://html.spec.whatwg.org/multipage/forms.html#statically-validate-the-con...
> > 1. Let controls be a list of all the submittable elements whose form owner
is
> form, in tree order.
> 
> In our implementation, it's
> HTMLFormElement::checkInvalidControlsAndCollectUnhandled().
> We should add isSubmittableElement [1] to HTMLFormControlElement, and
> checkInvalidControlsAndCollectUnhandled() should be:
> 
> HTMLFormControlElement* control = toHTMLFormControlElement(elements[i].get());
> if (control->isSubmittableElement() &&
> !control->checkValidity(unhandledInvalidControls, eventBehavior) &&
> control->formOwner() == this) {
>     ++invalidControlsCount;
> 
> [1] https://html.spec.whatwg.org/multipage/forms.html#category-submit

Thanks for the comments. I have uploaded a patch set with the changes. 
Please have a look.

[tkent, 2015-06-25 11:30:03]

https://codereview.chromium.org/1202563009/diff/20001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/1202563009/diff/20001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:529: if (this->formControlType() ==
"fieldset")
No, HTMLFormControlElement should not have knowledge of specific elements.
isSubmittableElement() should be a virtual function. 
HTMLFormControlElement::isSubmittableElement() should return true, and
HTMLFieldSetElement::isSubmittableElement() should return false.

[tanay.c, 2015-06-25 12:04:57]

Thanks. That makes sense. I incorporated the changes and uploaded a patch. PTAL

https://codereview.chromium.org/1202563009/diff/20001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/1202563009/diff/20001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.cpp:529: if (this->formControlType() ==
"fieldset")
On 2015/06/25 11:30:02, tkent wrote:
> No, HTMLFormControlElement should not have knowledge of specific elements.
> isSubmittableElement() should be a virtual function. 
> HTMLFormControlElement::isSubmittableElement() should return true, and
> HTMLFieldSetElement::isSubmittableElement() should return false.

Done.

[tkent, 2015-06-25 12:47:08]

https://codereview.chromium.org/1202563009/diff/40001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/fieldset/validation-message.html (right):

https://codereview.chromium.org/1202563009/diff/40001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/fieldset/validation-message.html:11: <script>
Please add an explanation of this test.

description('A fieldset element should not show a warning message during
interactive form validation.');

https://codereview.chromium.org/1202563009/diff/40001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.h (left):

https://codereview.chromium.org/1202563009/diff/40001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.h:110: 
Do not remove unrelated line.

[tanay.c, 2015-06-25 13:37:30]

Thanks, incorporated the changes.

https://codereview.chromium.org/1202563009/diff/40001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/fieldset/validation-message.html (right):

https://codereview.chromium.org/1202563009/diff/40001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/fieldset/validation-message.html:11: <script>
On 2015/06/25 12:47:08, tkent wrote:
> Please add an explanation of this test.
> 
> description('A fieldset element should not show a warning message during
> interactive form validation.');

Done.

https://codereview.chromium.org/1202563009/diff/40001/Source/core/html/HTMLFo...
File Source/core/html/HTMLFormControlElement.h (left):

https://codereview.chromium.org/1202563009/diff/40001/Source/core/html/HTMLFo...
Source/core/html/HTMLFormControlElement.h:110: 
On 2015/06/25 12:47:08, tkent wrote:
> Do not remove unrelated line.

Done.

[tkent, 2015-06-25 13:56:07]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-06-25 13:56:07]

lgtm

[commit-bot: I haz the power, 2015-06-25 13:56:31]

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

[commit-bot: I haz the power, 2015-06-25 15:06:23]

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

[commit-bot: I haz the power, 2015-06-25 15:06:24]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60751)

[tkent, 2015-06-25 22:50:42]

On 2015/06/25 15:06:24, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60751)

Ah, we already have a test. So, update *-expected.txt for
imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate.html,
and we don't need to add fast/forms/fieldset/validation-message.html.

[tanay.c, 2015-06-26 06:55:28]

On 2015/06/25 22:50:42, tkent wrote:
> On 2015/06/25 15:06:24, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60751)
> 
> Ah, we already have a test. So, update *-expected.txt for
>
imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate.html,
> and we don't need to add fast/forms/fieldset/validation-message.html.

Done.

[tkent, 2015-06-26 06:58:50]

https://codereview.chromium.org/1202563009/diff/80001/LayoutTests/imported/we...
File
LayoutTests/imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate-expected.txt
(right):

https://codereview.chromium.org/1202563009/diff/80001/LayoutTests/imported/we...
LayoutTests/imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate-expected.txt:1:
CONSOLE WARNING: line 13: The specified value 'abc' is not a valid email
address.
Did you produce the result correctly?

https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/60...

[tanay.c, 2015-06-26 13:11:15]

Uploaded the expectations file again. PTAL

https://codereview.chromium.org/1202563009/diff/80001/LayoutTests/imported/we...
File
LayoutTests/imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate-expected.txt
(right):

https://codereview.chromium.org/1202563009/diff/80001/LayoutTests/imported/we...
LayoutTests/imported/web-platform-tests/html/semantics/forms/constraints/form-validation-validate-expected.txt:1:
CONSOLE WARNING: line 13: The specified value 'abc' is not a valid email
address.
On 2015/06/26 06:58:50, tkent wrote:
> Did you produce the result correctly?
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/60...

Sorry, mismatch was due to rebase. The new patch should fix this.

[tanay.c, 2015-06-26 13:12:00]

The CQ bit was checked by tanay.c@samsung.com to run a CQ dry run

[tanay.c, 2015-06-26 13:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1202563009/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-06-26 13:12:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202563009/100001

[commit-bot: I haz the power, 2015-06-26 14:29:25]

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

[commit-bot: I haz the power, 2015-06-26 14:29:27]

Dry run: This issue passed the CQ dry run.

[tkent, 2015-06-26 19:44:19]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-06-26 19:44:20]

lgtm

[commit-bot: I haz the power, 2015-06-26 19:44:29]

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

[commit-bot: I haz the power, 2015-06-26 19:48:17]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197937