Bug 47813 - [HTML5] "form" attribute support for form control elements
: [HTML5] "form" attribute support for form control elements
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: HTML5
: 29363
: 19264
  Show dependency treegraph
 
Reported: 2010-10-18 04:35 PST by
Modified: 2010-11-14 20:57 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-10-18 04:35:59 PST
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 From 2010-11-05 01:18:32 PST -------
Created an attachment (id=73042) [details]
Patch V0
------- Comment #2 From 2010-11-05 01:19:32 PST -------
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 From 2010-11-05 01:30:27 PST -------
(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.
------- Comment #4 From 2010-11-05 06:34:32 PST -------
Created an attachment (id=73059) [details]
Patch V1
------- Comment #5 From 2010-11-05 06:35:25 PST -------
(In reply to comment #3)
> (From update of attachment 73042 [details] [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 From 2010-11-06 07:57:53 PST -------
(From update of attachment 73059 [details])
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 From 2010-11-06 08:08:50 PST -------
(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 From 2010-11-09 06:21:54 PST -------
Created an attachment (id=73367) [details]
Patch V2
------- Comment #9 From 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 From 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 From 2010-11-09 18:18:30 PST -------
(From update of attachment 73367 [details])
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 From 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 From 2010-11-11 23:43:01 PST -------
(From update of attachment 73367 [details])
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 From 2010-11-11 23:43:36 PST -------
Created an attachment (id=73705) [details]
Patch V3
------- Comment #15 From 2010-11-12 00:32:03 PST -------
(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.

> 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 From 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] [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 From 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 From 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 From 2010-11-13 08:49:43 PST -------
Created an attachment (id=73828) [details]
Patch V4
------- Comment #20 From 2010-11-14 17:38:07 PST -------
(From update of attachment 73828 [details])
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 From 2010-11-14 19:06:37 PST -------
(From update of attachment 73828 [details])
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 From 2010-11-14 19:07:38 PST -------
Created an attachment (id=73870) [details]
Patch V5
------- Comment #23 From 2010-11-14 19:41:32 PST -------
(From update of attachment 73870 [details])
ok.  Thanks you!
------- Comment #24 From 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 From 2010-11-14 20:57:16 PST -------
(From update of attachment 73870 [details])
Clearing flags on attachment: 73870

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