Bug 47813

Created attachment 73042 [details] Patch V0

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 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.

Created attachment 73059 [details] Patch V1

(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 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.

(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"

Created attachment 73367 [details] Patch V2

(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.

(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 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>

(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 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.

Created attachment 73705 [details] Patch V3

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);

(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:-)

(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.

(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,

Created attachment 73828 [details] Patch V4

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 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.

Created attachment 73870 [details] Patch V5

Comment on attachment 73870 [details] Patch V5 ok. Thanks you!

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 on attachment 73870 [details] Patch V5 Clearing flags on attachment: 73870 Committed r71994: <http://trac.webkit.org/changeset/71994>

All reviewed patches have been landed. Closing bug.