Bug 47813 - [HTML5] "form" attribute support for form control elements
: [HTML5] "form" attribute support for form control elements
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
: HTML5
Depends on: 29363
Blocks: HTML5Forms
  Show dependency treegraph
 
Reported: 2010-10-18 04:35 PDT by Kenichi Ishibashi
Modified: 2010-11-14 20:57 PST (History)
4 users (show)

See Also:


Attachments
Patch V0 (10.30 KB, patch)
2010-11-05 01:18 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (16.96 KB, patch)
2010-11-05 06:34 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V2 (38.72 KB, patch)
2010-11-09 06:21 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V3 (49.76 KB, patch)
2010-11-11 23:43 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V4 (47.61 KB, patch)
2010-11-13 08:49 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V5 (48.08 KB, patch)
2010-11-14 19:07 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2010-10-18 04:35:59 PDT
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.
Comment 1 Kenichi Ishibashi 2010-11-05 01:18:32 PDT
Created attachment 73042 [details]
Patch V0
Comment 2 Kenichi Ishibashi 2010-11-05 01:19:32 PDT
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.
Comment 3 Kent Tamura 2010-11-05 01:30:27 PDT
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.
Comment 4 Kenichi Ishibashi 2010-11-05 06:34:32 PDT
Created attachment 73059 [details]
Patch V1
Comment 5 Kenichi Ishibashi 2010-11-05 06:35:25 PDT
(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,
Comment 6 Kent Tamura 2010-11-06 07:57:53 PDT
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.
Comment 7 Kent Tamura 2010-11-06 08:08:50 PDT
(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"
Comment 8 Kenichi Ishibashi 2010-11-09 06:21:54 PST
Created attachment 73367 [details]
Patch V2
Comment 9 Kenichi Ishibashi 2010-11-09 06:23:21 PST
(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.
Comment 10 Erik Arvidsson 2010-11-09 10:13:04 PST
(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?
Comment 11 Kent Tamura 2010-11-09 18:18:30 PST
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>
Comment 12 Kent Tamura 2010-11-09 18:23:17 PST
(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.
Comment 13 Kenichi Ishibashi 2010-11-11 23:43:01 PST
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.
Comment 14 Kenichi Ishibashi 2010-11-11 23:43:36 PST
Created attachment 73705 [details]
Patch V3
Comment 15 Kent Tamura 2010-11-12 00:32:03 PST
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);
Comment 16 Kenichi Ishibashi 2010-11-12 02:55:56 PST
(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:-)
Comment 17 Kent Tamura 2010-11-12 04:10:51 PST
(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.
Comment 18 Kenichi Ishibashi 2010-11-12 15:18:11 PST
(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,
Comment 19 Kenichi Ishibashi 2010-11-13 08:49:43 PST
Created attachment 73828 [details]
Patch V4
Comment 20 Kent Tamura 2010-11-14 17:38:07 PST
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.
Comment 21 Kenichi Ishibashi 2010-11-14 19:06:37 PST
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.
Comment 22 Kenichi Ishibashi 2010-11-14 19:07:38 PST
Created attachment 73870 [details]
Patch V5
Comment 23 Kent Tamura 2010-11-14 19:41:32 PST
Comment on attachment 73870 [details]
Patch V5

ok.  Thanks you!
Comment 24 WebKit Commit Bot 2010-11-14 20:21:38 PST
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 ddkilzer@webkit.org and dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 25 WebKit Commit Bot 2010-11-14 20:57:16 PST
Comment on attachment 73870 [details]
Patch V5

Clearing flags on attachment: 73870

Committed r71994: <http://trac.webkit.org/changeset/71994>
Comment 26 WebKit Commit Bot 2010-11-14 20:57:23 PST
All reviewed patches have been landed.  Closing bug.