Bug 49240 – Implement formaction, formenctype, formmethod and formtarget attributes for input and button tags

• Component: Forms
• Status: RESOLVED FIXED

[Dai Mikurube]

Implement 4 formaction, formenctype, formmethod and formtarget attributes for <input> tag whose type=submit.

See HTML5 4.10.19.6 Form submission :
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#form-submission-0

[Dai Mikurube]

Created attachment 73354 [details]
Patch

[Kent Tamura]

Add Dimitri and Darin who are familiar with form submission.

[Dimitri Glazkov (Google)]

Comment on attachment 73354 [details]
Patch

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

Just  a few comments.

> LayoutTests/fast/forms/formtarget-attribute.html:8
> +                    layoutTestController.waitUntilDone();

This definitely should be a dumpAsText test.

> WebCore/html/HTMLFormElement.cpp:319
> +    if (event && event->target() && event->target()->toNode())
> +        button = static_cast<HTMLFormControlElement*>(event->target()->toNode());

This seems like a different method from obtaining the button from above (see firstSuccessfulSubmitButton logic). What's the difference?

> WebCore/html/HTMLFormElement.cpp:330
> +    if (button) {
> +        String attribute;
> +        if (!(attribute = button->formAction()).isNull())
> +            copiedAttributes.parseAction(attribute);
> +        if (!(attribute = button->formEnctype()).isNull())
> +            copiedAttributes.parseEncodingType(attribute);
> +        if (!(attribute = button->formMethod()).isNull())
> +            copiedAttributes.parseMethodType(attribute);
> +        if (!(attribute = button->formTarget()).isNull())
> +            copiedAttributes.setTarget(attribute);
> +    }

This whole thing looks like it should just live in FormSubmission::create.

> WebCore/loader/FormSubmission.cpp:121
> +void FormSubmission::Attributes::copyFrom(const Attributes& other)
> +{
> +    m_method = other.m_method;
> +    m_action = other.m_action;
> +    m_target = other.m_target;
> +    m_encodingType = other.m_encodingType;
> +    m_acceptCharset = other.m_acceptCharset;
> +}
> +

And this becomes unnecessary.

[Kent Tamura]

Comment on attachment 73354 [details]
Patch

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

> LayoutTests/fast/forms/formaction-attribute-expected.txt:2
> + 
> +This test: PASSED!

The test result should have a test description.

> LayoutTests/fast/forms/formaction-attribute.html:4
> + if (window.layoutTestController)

Indentation looks strange. 1 space?

> LayoutTests/fast/forms/formaction-attribute.html:11
> +ul.document.getElementById('sourceViewDiv').innerHTML=' This test: FAILED 2'
> +}
> +function test2(ul) {
> +ul.document.getElementById('sourceViewDiv').innerHTML=' This test: PASSED!'

No indentation.
No spaces around "="
We had better use functions in fast/js/resources/js-test-pre.js as possible.

> LayoutTests/fast/forms/formaction-attribute.html:27
> +var x=document.getElementsByTagName('input')[0];

Need spaces around "="

> LayoutTests/fast/forms/formmethod-attribute-expected.txt:2
> +
> +Success

The test result should have a test description.

> LayoutTests/fast/forms/formmethod-attribute.html:3
> +<meta http-equiv="content-type" content="text/html; charset=iso-8859-1">

Do we need this line?

> LayoutTests/fast/forms/formmethod-attribute.html:24
> +        if (document.URL.substring(document.URL.indexOf('?')+1, document.URL.length) == "")

style: need spaces around "+"

> LayoutTests/fast/forms/formmethod-attribute.html:25
> +            document.write("<p>Success</p>");

We had better use functions in fast/js/resources/js-test-pre.js as possible.

> LayoutTests/fast/forms/formmethod-attribute.html:35
> +    document.write("<p>This test doesn't work directly from bugzilla, please save it to a local file first.</p>");

We don't need to mention it.

> LayoutTests/fast/forms/formtarget-attribute.html:6
> +            {

Your JavaScript style is inconsistent:

function name() {
  and
function name()
{

Either is ok because we don't have explicit style rules for JavaScript, but you should be consistent.

> LayoutTests/fast/forms/formtarget-attribute.html:10
> +                // Not dumping as text since the DumpRenderTree does not dump the text content of child frames.                

We can access the content of iframe by iframe.contentDocument.  We might need to call layoutTestContentroller.setAllowUniversalAccessFromFileURLs(true).

> LayoutTests/fast/forms/mailto/formenctype-attribute.html:11
> +  if (window.layoutTestController) {

We usually use 4-space indentation.

> WebCore/html/HTMLFormControlElement.cpp:87
> +String HTMLFormControlElement::formAction() const

I don't think we need these access functions.  JavaScript binding doesn't need them because of [Reflect], and HTMLFormElement can access them by getAttribute().

[Dai Mikurube]

Comment on attachment 73354 [details]
Patch

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

>> LayoutTests/fast/forms/formaction-attribute-expected.txt:2
>> +This test: PASSED!
> 
> The test result should have a test description.

Thanks. Made it more descriptive. These four new tests were variations from existing tests with as small changes as possible. For this formaction-attribute.html, from form-action.html.

>> LayoutTests/fast/forms/formtarget-attribute.html:8
>> +                    layoutTestController.waitUntilDone();
> 
> This definitely should be a dumpAsText test.

Thanks. I chose dumpAsText and changed how to verify.

>> WebCore/html/HTMLFormControlElement.cpp:87
>> +String HTMLFormControlElement::formAction() const
> 
> I don't think we need these access functions.  JavaScript binding doesn't need them because of [Reflect], and HTMLFormElement can access them by getAttribute().

Exactly. Removed.

>> WebCore/html/HTMLFormElement.cpp:319
>> +        button = static_cast<HTMLFormControlElement*>(event->target()->toNode());
> 
> This seems like a different method from obtaining the button from above (see firstSuccessfulSubmitButton logic). What's the difference?

firstSuccessfulSubmitButton was different from this "button" object. In cases of my tests, firstSuccessfulSubmitButton was 0, but "button" pointed the button clicked.

>> WebCore/html/HTMLFormElement.cpp:330
>> +    }
> 
> This whole thing looks like it should just live in FormSubmission::create.

I guess it should be handled here for the same reason as copyFrom() below.

>> WebCore/loader/FormSubmission.cpp:121
>> +
> 
> And this becomes unnecessary.

Copying is still required. Values of attributes form* (like formaction) must be parsed by parser methods (like FormSubmission::Attributes::parseAction()). But HTMLFormElement::m_attribute should not be modified.

[Dai Mikurube]

Created attachment 73579 [details]
Patch

[Kent Tamura]

(In reply to comment #5)
> >> WebCore/html/HTMLFormElement.cpp:330
> >> +    }
> > 
> > This whole thing looks like it should just live in FormSubmission::create.
> 
> I guess it should be handled here for the same reason as copyFrom() below.
> 
> >> WebCore/loader/FormSubmission.cpp:121
> >> +
> > 
> > And this becomes unnecessary.
> 
> Copying is still required. Values of attributes form* (like formaction) must be parsed by parser methods (like FormSubmission::Attributes::parseAction()). But HTMLFormElement::m_attribute should not be modified.

I understand the requirement of copyFrom(), but we can move the HTMLFormElement.cpp change to FormSubmission.cpp because FormSubmission::create() knows all of necessary information.

[Kent Tamura]

Comment on attachment 73579 [details]
Patch

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

> LayoutTests/fast/forms/formaction-attribute.html:12
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

You dont need to do this.  js-test-pre.js does it.

> LayoutTests/fast/forms/formaction-attribute.html:14
> +doneAction = false;

Prepend "var ".

> LayoutTests/fast/forms/formaction-attribute.html:29
> +<script>

Why does this HTML have two <script>s?
I think you can merge them into one.

> LayoutTests/fast/forms/formaction-attribute.html:32
> +var x = document.getElementsByTagName('input')[0];

"x" is not a good name.

> LayoutTests/fast/forms/formaction-attribute.html:36
> +successfullyParsed = true;

Prepend "var ".

> LayoutTests/fast/forms/formmethod-attribute-expected.txt:1
> +PASS The formmethod attribute was successfully used

Please add a test description.  You can use description() defined by js-test-pre.js

> LayoutTests/fast/forms/formmethod-attribute.html:14
> +<script>

add description() call please.

> LayoutTests/fast/forms/formmethod-attribute.html:16
> +    layoutTestController.dumpAsText();

You don't need to call dumpAsText() because of js-test-pre.js.

> LayoutTests/fast/forms/formmethod-attribute.html:35
> +successfullyParsed = true;

add "var ".

> LayoutTests/fast/forms/formtarget-attribute.html:7
> +<script>

You can merge this <script> block into the <script> below.

> LayoutTests/fast/forms/formtarget-attribute.html:12
> +        layoutTestController.dumpAsText();

You don't need to call dumpAsText() because of js-test-pre.js.

> WebCore/ChangeLog:19
> +        (WebCore::HTMLFormControlElement::formAction):
> +        (WebCore::HTMLFormControlElement::formEnctype):
> +        (WebCore::HTMLFormControlElement::setFormEnctype):
> +        (WebCore::HTMLFormControlElement::formMethod):
> +        (WebCore::HTMLFormControlElement::formTarget):

Please update the ChangeLog.  This block refers to non-existent functions.

> WebCore/html/HTMLFormControlElement.cpp:87
> +String HTMLFormControlElement::formEnctype() const

Why didn't you remove formEnctype() and setFormEnctype()?

[Kent Tamura]

BTW, <button> should have these attributes too.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-button-element

[Dai Mikurube]

Comment on attachment 73579 [details]
Patch

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

>> WebCore/html/HTMLFormControlElement.cpp:87
>> +String HTMLFormControlElement::formEnctype() const
> 
> Why didn't you remove formEnctype() and setFormEnctype()?

They are required since formenctype is [ConvertNullToNullString]. It followed the enctype attribute in form tags. (See HTMLFormElement.idl)

[Dai Mikurube]

Created attachment 73588 [details]
Patch

[Kent Tamura]

(In reply to comment #10)
> (From update of attachment 73579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73579&action=review
> 
> >> WebCore/html/HTMLFormControlElement.cpp:87
> >> +String HTMLFormControlElement::formEnctype() const
> > 
> > Why didn't you remove formEnctype() and setFormEnctype()?
> 
> They are required since formenctype is [ConvertNullToNullString]. It followed the enctype attribute in form tags. (See HTMLFormElement.idl)

[Reflect] for DOMString implies ConvertNullToNullString.

[Kent Tamura]

Comment on attachment 73588 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +        Implement formaction, formenctype, formmethod and formtarget attributes for the input tag

Now we support <button>.  The summary should be updated.

> WebCore/ChangeLog:5
> +        Implement formaction, formenctype, formmethod and formtarget attributes for the input tag

ditto.

[Dai Mikurube]

(In reply to comment #12)
When using ConvertNullToNullString, formEnctype() and setFormEnctype() are required to compile DerivedSources/WebCore/DOMHTMLFormControlElement.mm, a generated file from idl. This mm calls formEnctype and setFormEnctype.

[Dai Mikurube]

Created attachment 73591 [details]
Patch

[Kent Tamura]

(In reply to comment #14)
> (In reply to comment #12)
> When using ConvertNullToNullString, formEnctype() and setFormEnctype() are required to compile DerivedSources/WebCore/DOMHTMLFormControlElement.mm, a generated file from idl. This mm calls formEnctype and setFormEnctype.

You don't need to add ConvertNullToNullString.
  attribute [Reflect] DOMString formEnctype;
is enough.  It contains ConvertNullToNullString behavior.

[Kent Tamura]

Comment on attachment 73591 [details]
Patch

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

> WebCore/html/HTMLInputElement.idl:31
> +        attribute [Reflect, URL] DOMString formAction;
> +        attribute [ConvertNullToNullString] DOMString formEnctype;
> +        attribute [Reflect] DOMString formMethod;
>          attribute [Reflect] boolean formNoValidate;
> +        attribute [Reflect] DOMString formTarget;

We need a similar change for HTMLButtonElement.idl.
Also, we need tests for accessing them by JavaScript properties. e.g. input.formAction reflects formaction= HTML attribute correctly?

[Dai Mikurube]

Created attachment 73704 [details]
Patch

[Dai Mikurube]

(In reply to comment #17)
Kent-san,

Thanks for the comments. Revised.

[Kent Tamura]

Comment on attachment 73704 [details]
Patch

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

> LayoutTests/ChangeLog:33
> +        * fast/forms/formmethod-attribute-button-html-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-button-html.html: Added.
> +        * fast/forms/formmethod-attribute-button-js-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-button-js.html: Added.
> +        * fast/forms/formmethod-attribute-input-html-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-input-html.html: Added.
> +        * fast/forms/formmethod-attribute-input-js-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-input-js.html: Added.
> +        * fast/forms/formtarget-attribute-button-html-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-button-html.html: Added.
> +        * fast/forms/formtarget-attribute-button-js-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-button-js.html: Added.
> +        * fast/forms/formtarget-attribute-input-html-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-input-html.html: Added.
> +        * fast/forms/formtarget-attribute-input-js-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-input-js.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-html-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-html.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-js-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-js.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-html-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-html.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-js-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-js.html: Added.

I don't think we should test the same test cases with both of an HTML attribute and the corresponding DOM property.
As for tests of form submission behavior, we should test it with either of the HTML attribute or the DOM property, and we should have additional tests for association of the HTML attribute and the DOM property like LayoutTests/fast/forms/script-tests/input-minmax.html.

[Kent Tamura]

(In reply to comment #20)
> I don't think we should test the same test cases with both of an HTML attribute and the corresponding DOM property.

because running a complex test case twice is not good for test performance.

[Dai Mikurube]

Created attachment 73712 [details]
Patch

[Kent Tamura]

Comment on attachment 73712 [details]
Patch

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

> LayoutTests/ChangeLog:21
> +        * fast/forms/formaction-attribute-expected.txt: Added.
> +        * fast/forms/formaction-attribute.html: Added.
> +        * fast/forms/formmethod-attribute-button-html-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-button-html.html: Added.
> +        * fast/forms/formmethod-attribute-input-html-expected.txt: Added.
> +        * fast/forms/formmethod-attribute-input-html.html: Added.
> +        * fast/forms/formtarget-attribute-button-html-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-button-html.html: Added.
> +        * fast/forms/formtarget-attribute-input-html-expected.txt: Added.
> +        * fast/forms/formtarget-attribute-input-html.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-html-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-button-html.html: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-html-expected.txt: Added.
> +        * fast/forms/mailto/formenctype-attribute-input-html.html: Added.

There is no submit-form-attribute.html.

> LayoutTests/fast/forms/script-tests/submit-form-attributes.js:6
> +input.type="submit";

nit: Need spaces around "=".

> LayoutTests/fast/forms/script-tests/submit-form-attributes.js:100
> +button.type="submit";

ditto.

> WebCore/ChangeLog:20
> +        Tests: fast/forms/formaction-attribute.html
> +               fast/forms/formmethod-attribute-button-html.html
> +               fast/forms/formmethod-attribute-button-js.html
> +               fast/forms/formmethod-attribute-input-html.html
> +               fast/forms/formmethod-attribute-input-js.html
> +               fast/forms/formtarget-attribute-button-html.html
> +               fast/forms/formtarget-attribute-button-js.html
> +               fast/forms/formtarget-attribute-input-html.html
> +               fast/forms/formtarget-attribute-input-js.html
> +               fast/forms/mailto/formenctype-attribute-button-html.html
> +               fast/forms/mailto/formenctype-attribute-button-js.html
> +               fast/forms/mailto/formenctype-attribute-input-html.html
> +               fast/forms/mailto/formenctype-attribute-input-js.html

Looks obsolete.

[Dai Mikurube]

Created attachment 73715 [details]
Patch

[Kent Tamura]

Comment on attachment 73715 [details]
Patch

Looks ok.  Thank you!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73715 [details]:

animations/suspend-resume-animation.html
http/tests/misc/empty-file-formdata.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 73715 [details]
Patch

Rejecting patch 73715 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
tests/inspector ......
http/tests/loading ....................
http/tests/local ........
http/tests/local/blob ...
http/tests/local/formdata ....
http/tests/media .......
http/tests/messaging ..
http/tests/mime .......
http/tests/misc .........................
http/tests/misc/empty-file-formdata.html -> failed

Exiting early after 1 failures. 21119 tests run.
480.33s total testing time

21118 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
11 test cases (<1%) had stderr output

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

[Kent Tamura]

(In reply to comment #27)
> http/tests/misc .........................
> http/tests/misc/empty-file-formdata.html -> failed
> 
> Exiting early after 1 failures. 21119 tests run.
> 480.33s total testing time
> 
> 21118 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout

Mikurube-san,
This commit-queue output means your patch broke empty-file-formdata.html.  Would you investigate it please?

[Dai Mikurube]

(In reply to comment #28)
Yes, I'm checking it. Thanks for your notification.

[Dai Mikurube]

Created attachment 73874 [details]
Patch

[Dai Mikurube]

(In reply to comment #28)
Fixed. Added
>     m_isMultiPartForm = other.m_isMultiPartForm;
in WebCore/loader/FormSubmission.cpp: FormSubmission::Attributes::copyFrom.

[Kent Tamura]

Comment on attachment 73874 [details]
Patch

> Fixed. Added
> >     m_isMultiPartForm = other.m_isMultiPartForm;
> in WebCore/loader/FormSubmission.cpp: FormSubmission::Attributes::copyFrom.

ok, I understand.  Our tests have good coverage :-)

[WebKit Commit Bot]

Comment on attachment 73874 [details]
Patch

Rejecting patch 73874 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 73874]" exit_code: 2
Last 500 characters of output:
th fuzz 3.
patching file WebCore/html/HTMLAttributeNames.in
Hunk #1 FAILED at 94.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/HTMLAttributeNames.in.rej
patching file WebCore/html/HTMLButtonElement.idl
patching file WebCore/html/HTMLInputElement.idl
patching file WebCore/loader/FormSubmission.cpp
patching file WebCore/loader/FormSubmission.h

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1

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

[Dai Mikurube]

Created attachment 73882 [details]
Patch

[Dai Mikurube]

(In reply to comment #33)
Looks like a conflict... Merged and re-submitted.

[Dai Mikurube]

Kent-san,

Thanks!

[WebKit Commit Bot]

Comment on attachment 73882 [details]
Patch

Clearing flags on attachment: 73882

Committed r72003: <http://trac.webkit.org/changeset/72003>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72003 might have broken Qt Linux Release
The following tests are not passing:
fast/forms/formaction-attribute.html
fast/forms/formmethod-attribute-button-html.html
fast/forms/formtarget-attribute-button-html.html
fast/forms/mailto/formenctype-attribute-button-html.html
fast/forms/mailto/formenctype-attribute-input-html.html

[Andy Estes]

http://trac.webkit.org/changeset/72003 caused the test 'fast/forms/submit-form-attributes.html' to crash DumpRenderTree on the Leopard Intel Debug (Tests) bot. Filed https://bugs.webkit.org/show_bug.cgi?id=49562.