[HTML5] "form" attribute support for form control elements

• Component: Forms
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

Adding "form" attribute support for form control elements. This issue is a part of implementing the <output> element. See https://bugs.webkit.org/show_bug.cgi?id=29363 for more details.

[Kenichi Ishibashi]

Created attachment 73042 [details]
Patch V0

[Kenichi Ishibashi]

Hi,

This patch will cover most of form-associated elements, but  <label> and <object> elements are not. For <object>, we need to change its class hierarchy, as https://bugs.webkit.org/show_bug.cgi?id=48821 described and would be addressed on the issue. For <label>, we also might need to change its class inheritance because it isn't a subclass of HTMLFormControlElement. I think it would be better that making another issue to work on it so the patch doesn't include "form" attribute support for <label>. I'll create new issue for <label> if this patch seems to be reasonable.

[Kent Tamura]

Comment on attachment 73042 [details]
Patch V0

We need to support
 - HTMLFormElement::elements contains elements of which form attribute value point the form even if elements are outside of the form
 - Form submission contains name&value pairs for such elements.

[Kenichi Ishibashi]

Created attachment 73059 [details]
Patch V1

[Kenichi Ishibashi]

(In reply to comment #3)
> (From update of attachment 73042 [details])
> We need to support
>  - HTMLFormElement::elements contains elements of which form attribute value point the form even if elements are outside of the form
>  - Form submission contains name&value pairs for such elements.

Kent-san,

Thank you for your review. I didn't realize the requirement. I've revised the patch. It might be somewhat naive, but I'd like ask you to some advice whether I'm not going to wrong way. It would be great if you could give me some advice for this patch.

Thanks,

[Kent Tamura]

Comment on attachment 73059 [details]
Patch V1

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

Yes, we need to update HTMLFormElement::m_associatedElements. However, HTMLFormElement::formElementIndex() doesn't work well with elements outside of the form.  We need to update formElementIndex() implementation, and should add test cases for form.elements[] order.

We need to handle cases that
 - A "form" attribute of a control is added or changed to another form, or removed.
 - An "id" attribute of a form element is added, changed, or removed.

You had better to take a look at the specification again.  It mentions many cases of changing form owner.
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fae-form

Also, we need to take care of multiple form elements with identical ID values.  Document::getElementById() returns the first element in document-order in such case.  So <foo form=id1> should be associated to the first form with id=id1 in document-order.

> WebCore/html/HTMLFormElement.cpp:150
> +        for (Node* node = document()->traverseNextNode(); node; node = node->traverseNextNode()) {

This is not acceptable at all.  This code will introduce significant performance regression for existing pages.

I recommend to store a list (hash?) of form controls with id attribute to Document, like existing Document::registerFormElementWithState() and unregisterFormElementWithState(), and search the list for controls pointing this form element.

[Kent Tamura]

(In reply to comment #6)
> > WebCore/html/HTMLFormElement.cpp:150
> > +        for (Node* node = document()->traverseNextNode(); node; node = node->traverseNextNode()) {
> 
> This is not acceptable at all.  This code will introduce significant performance regression for existing pages.
> 
> I recommend to store a list (hash?) of form controls with id attribute to Document, like existing Document::registerFormElementWithState() and unregisterFormElementWithState(), and search the list for controls pointing this form element.

correction: "form controls with id attribute" -> "form controls with form attribute"

[Kenichi Ishibashi]

Created attachment 73367 [details]
Patch V2

[Kenichi Ishibashi]

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

Thank you for review and detailed comments. I've revised the patch. The patch still might be not good enough, but I'd like you to glance over the current implementation.

Following your suggestion, I've added a list of form-associated elements with form attribute. I also added two variables into HTMLFormElement for avoiding performance regression for existing pages. They are indices of m_associatedElements and denotes the range of elements which are children of the form element. These variables are used when form-associated elements which don't have form attribute are inserted. I think it would have less impact on existing pages.

On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.

[Erik Arvidsson]

(In reply to comment #9)
> On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.

Why don't you keep a centralized table of form associations?

[Kent Tamura]

Comment on attachment 73367 [details]
Patch V2

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

> LayoutTests/fast/forms/script-tests/form-attribute-elements-order.js:5
> +container.innerHTML = '<input name="victim" id="before1" form="owner" />' +

nit: You don't need double-quotes surrounding attribute values in many cases.

> LayoutTests/fast/forms/script-tests/form-attribute.js:34
> +debug('- Ensures that the form attribute points the form owner even if the element is within another form element.');
> +container.innerHTML = '<form id="owner"></form>' +
> +    '<form id="shouldNotBeOwner">' +
> +    '    <input id="inputElement" name="victim" form="owner" />' +
> +    '</form>';

How about the following similar case?

<form id=owner>
 <form id=shouldNotBeOwner>
  <input id=inputElement name=victime form=owner>
 </form>
</form>

> WebCore/html/HTMLFormElement.cpp:406
> +unsigned HTMLFormElement::formElementIndexWithFormAttribute(HTMLFormControlElement* element)

Does this work if the element is inside of the form?

<form id=owner>
  <input>
  <input form=owner>
  <input>
</form>

or


<form id=owner>
  <form>
   <input>
   <input form=owner>
   <input>
  </form>
</form>

[Kent Tamura]

(In reply to comment #9)
> On the other hand, for elements with form attribute, I couldn't come up with efficient ways to handle them. So the current implementation walks through the entire tree of the document to find the right index to be inserted.

We discussed this offline.
 - Controls with form attribute may be rare.  The traversal cost might be acceptable.
 - However, we have an idea of reducing the traversal cost.  Ishibashi-san will try it.

(In reply to comment #10)
> Why don't you keep a centralized table of form associations?

It won't help so much.  We need to detect control order anyway.

[Kenichi Ishibashi]

Comment on attachment 73367 [details]
Patch V2

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

Kent-san,

Thank you for the review and suggestions. I've updated the patch and I'll post it soon.
I've implemented some algorithms for elements which have form attribute, that are binary search algorithm and comparing the position of two elements, to find the right place to be inserted into m_associatedElements of HTMLFormElement. I hope that the patch would be more efficient than the old one.

Regards,

>> LayoutTests/fast/forms/script-tests/form-attribute-elements-order.js:5
>> +container.innerHTML = '<input name="victim" id="before1" form="owner" />' +
> 
> nit: You don't need double-quotes surrounding attribute values in many cases.

Thank you for letting me know that. I've removed these double-quotes.

>> LayoutTests/fast/forms/script-tests/form-attribute.js:34
>> +    '</form>';
> 
> How about the following similar case?
> 
> <form id=owner>
>  <form id=shouldNotBeOwner>
>   <input id=inputElement name=victime form=owner>
>  </form>
> </form>

I've added the case. BTW, it seems that Webkit ignores nested inner form elements so I just added test which ensures each form attribute points the out-most form element. I'm not sure that is correct, so please let me know if I was done something wrong.

>> WebCore/html/HTMLFormElement.cpp:406
>> +unsigned HTMLFormElement::formElementIndexWithFormAttribute(HTMLFormControlElement* element)
> 
> Does this work if the element is inside of the form?
> 
> <form id=owner>
>   <input>
>   <input form=owner>
>   <input>
> </form>
> 
> or
> 
> 
> <form id=owner>
>   <form>
>    <input>
>    <input form=owner>
>    <input>
>   </form>
> </form>

Added some test cases like that.

[Kenichi Ishibashi]

Created attachment 73705 [details]
Patch V3

[Kent Tamura]

Comment on attachment 73705 [details]
Patch V3

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

> WebCore/html/HTMLFormElement.cpp:403
> +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)

- 'n1' 'n2" are not good names.  They should be node1 and node2.
- This function should belong to Document class.
- The "int" return value is not good.  We should introduce new enum.

> WebCore/html/HTMLFormElement.cpp:415
> +    // Keeping the nearest sucessors of each nodes for later use.

typo and grammer: "successor of each node"

> WebCore/html/HTMLFormElement.cpp:420
> +    while (ancestor1 && ancestor2) {
> +        if (ancestor1 == ancestor2)
> +            break;

You can write "while (ancestor1 && ancestor2 && ancestor1 != ancestor2) {"

> WebCore/html/HTMLFormElement.cpp:441
> +    ASSERT(successor1 && successor2);

successor1 or successor2 can be 0 if n1 is an ancestor of n2, or vice versa.  It never happens in the form attribute case, but we should support it because we'd like to move this function to Document.

BTW, you shouldn't use && in ASSERT().  You should have written
ASSERT(successor1);
ASSERT(successor2);

[Kenichi Ishibashi]

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

Thank you for your quick review.

> (From update of attachment 73705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73705&action=review
> 
> > WebCore/html/HTMLFormElement.cpp:403
> > +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)
> 
> - 'n1' 'n2" are not good names.  They should be node1 and node2.
> - This function should belong to Document class.
> - The "int" return value is not good.  We should introduce new enum.

I see. I'll move the function to Document class. So, should we also make nodeDepth() belong to other class, say, Node class?

> > WebCore/html/HTMLFormElement.cpp:415
> > +    // Keeping the nearest sucessors of each nodes for later use.
> 
> typo and grammer: "successor of each node"

Thanks, I'll fix it.

> 
> > WebCore/html/HTMLFormElement.cpp:420
> > +    while (ancestor1 && ancestor2) {
> > +        if (ancestor1 == ancestor2)
> > +            break;
> 
> You can write "while (ancestor1 && ancestor2 && ancestor1 != ancestor2) {"

I'll fix it.

> 
> > WebCore/html/HTMLFormElement.cpp:441
> > +    ASSERT(successor1 && successor2);
> 
> successor1 or successor2 can be 0 if n1 is an ancestor of n2, or vice versa.  It never happens in the form attribute case, but we should support it because we'd like to move this function to Document.

I'll remove it when I moves the function to the Document class.

> BTW, you shouldn't use && in ASSERT().  You should have written
> ASSERT(successor1);
> ASSERT(successor2);

I see. Thank you for suggestion:-)

[Kent Tamura]

(In reply to comment #16)
> > > WebCore/html/HTMLFormElement.cpp:403
> > > +static int compareTreeOrder(Node* n1, int depth1, Node* n2, int depth2)
> > 
> > - 'n1' 'n2" are not good names.  They should be node1 and node2.
> > - This function should belong to Document class.
> > - The "int" return value is not good.  We should introduce new enum.
> 
> I see. I'll move the function to Document class. So, should we also make nodeDepth() belong to other class, say, Node class?

Oops, I have found Node::compareDocumentPosition() now.  So we can use it and compareTreeOrder() is not needed.  I'm sorry for wasting your time.
compareDocumentPosition() looks slower than your compareTreeOrder().  But it would be acceptable.  We don't need nodeDepth() too.

[Kenichi Ishibashi]

(In reply to comment #17)

Kent-san,

> Oops, I have found Node::compareDocumentPosition() now.  So we can use it and compareTreeOrder() is not needed.  I'm sorry for wasting your time.
> compareDocumentPosition() looks slower than your compareTreeOrder().  But it would be acceptable.  We don't need nodeDepth() too.

Thank you for letting me know the function. Before starting implementation, I should investigate existing code more carefully. It's definitely my fault. Sorry for wasting your time. I'll revise the patch.

Regards,

[Kenichi Ishibashi]

Created attachment 73828 [details]
Patch V4

[Kent Tamura]

Comment on attachment 73828 [details]
Patch V4

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

> LayoutTests/fast/forms/form-attribute-expected.txt:7
> +FIXME: <label> and <object> don't support the form attribute for now.

nit: It's ok to add test cases for <label> and <object> now. -expected.txt contains FAIL lines in that case.

> WebCore/dom/Document.cpp:4368
> +void Document::registerFormElementWithFormAttribute(Element* e)

The parameter name "e" is not good.  It should be "control", "formControl", "element" or something.

> WebCore/dom/Document.cpp:4374
> +void Document::unregisterFormElementWithFormAttribute(Element* e)

ditto.

> WebCore/dom/Document.h:487
> +    void registerFormElementWithFormAttribute(Element* e);
> +    void unregisterFormElementWithFormAttribute(Element* e);

The argument name "e" doesn't add any useful information.  So we should remove it.

> WebCore/html/HTMLFormElement.cpp:418
> +        unsigned middle = (left + right) / 2;

Possible integer overflow.  Fix it.

[Kenichi Ishibashi]

Comment on attachment 73828 [details]
Patch V4

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

Kent-san,

Thank you for review. I'll post revised patch soon.

>> LayoutTests/fast/forms/form-attribute-expected.txt:7
>> +FIXME: <label> and <object> don't support the form attribute for now.
> 
> nit: It's ok to add test cases for <label> and <object> now. -expected.txt contains FAIL lines in that case.

I see. I've added test cases for <label> and <object>.

>> WebCore/dom/Document.cpp:4368
>> +void Document::registerFormElementWithFormAttribute(Element* e)
> 
> The parameter name "e" is not good.  It should be "control", "formControl", "element" or something.

Changed to control. Thanks!

>> WebCore/dom/Document.cpp:4374
>> +void Document::unregisterFormElementWithFormAttribute(Element* e)
> 
> ditto.

ditto.

>> WebCore/dom/Document.h:487
>> +    void unregisterFormElementWithFormAttribute(Element* e);
> 
> The argument name "e" doesn't add any useful information.  So we should remove it.

Removed.

>> WebCore/html/HTMLFormElement.cpp:418
>> +        unsigned middle = (left + right) / 2;
> 
> Possible integer overflow.  Fix it.

Done.

[Kenichi Ishibashi]

Created attachment 73870 [details]
Patch V5

[Kent Tamura]

Comment on attachment 73870 [details]
Patch V5

ok.  Thanks you!

[WebKit Commit Bot]

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

fast/workers/storage/interrupt-database-sync.html
webarchive/test-link-rel-icon.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 73870 [details]
Patch V5

Clearing flags on attachment: 73870

Committed r71994: <http://trac.webkit.org/changeset/71994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.