Bug 86746

Summary:

Layout broken after cloning and re-inserting a table with a misplaced <form>

Product:

WebKit

Reporter:

dtyschenko <dtyschenko>

Component:

Tables

Assignee:

Pravin D <pravind.2k4>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bdakin, darin, dbates, jchaffraix, kling, pravind.2k4, tkent

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
screenshot	none
test case	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

dtyschenko@gmail.com

Reported 2012-05-17 09:13:13 PDT

Created attachment 142487 [details] screenshot If the page content isn't valid, webkit will tolerantly render document as other engines do, but clonning and appending "invlaid" nodes will result in different (screwed) output. Simple example: <html> <body> <table border="1"> <tr><td width="150px"> </td></tr> <tr><form><td width="150px"> </td></form></tr> <tr><td width="150px"> </td></tr> </table> <script type="text/javascript"> document.body.appendChild( document.getElementsByTagName("TABLE")[0].cloneNode(true)); </script> </body> </html> See the second row does contain <form> tag, but webkit is able to render the table so the <form> doesn't affect the layout. But this isn't applied to the cloned table and it's screwed. Tested in Chrome 19.0.1084.46 (webkit 536.5) and Chromium 18.0.1025.168 (535.19)

Attachments
screenshot (851 bytes, image/png) 2012-05-17 09:13 PDT, dtyschenko@gmail.com	no flags	Details
test case (331 bytes, text/html) 2012-05-17 11:23 PDT, Alexey Proskuryakov	no flags	Details
Patch (7.21 KB, text/plain) 2012-08-27 10:53 PDT, Pravin D	no flags	Details
Patch (7.25 KB, patch) 2012-08-27 12:26 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (7.15 KB, patch) 2012-09-03 07:32 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (11.86 KB, patch) 2012-10-02 08:11 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (11.83 KB, patch) 2012-10-03 07:58 PDT, Pravin D	no flags	Details Formatted Diff Diff
Patch (8.63 KB, patch) 2012-10-03 13:58 PDT, Pravin D	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2012-05-17 11:23:49 PDT

Created attachment 142510 [details] test case Same test as attachment.

Alexey Proskuryakov

Comment 2 2012-05-17 11:34:49 PDT

DOM is the same in both tables, but layout is different. I wonder if there is a reason why <form> is allowed here in the same place - other elements get reparented.

Pravin D

Comment 3 2012-08-27 10:53:32 PDT

Created attachment 160745 [details] Patch

Pravin D

Comment 4 2012-08-27 12:26:57 PDT

Created attachment 160776 [details] Patch

Julien Chaffraix

Comment 5 2012-08-31 13:52:10 PDT

Comment on attachment 160776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160776&action=review > Source/WebCore/ChangeLog:21 > + (WebCore): Let's remove those useless lines. > Source/WebCore/html/HTMLFormElement.cpp:694 > + m_wasDemoted = sourceElement.m_wasDemoted; Shouldn't you copy the other fields instead of just copying m_wasDemoted? > Source/WebCore/html/HTMLFormElement.h:136 > + virtual void copyNonAttributePropertiesFromElement(const Element&); OVERRIDE.

Pravin D

Comment 6 2012-09-03 07:32:38 PDT

Created attachment 161926 [details] Patch

Pravin D

Comment 7 2012-09-03 07:45:44 PDT

(In reply to comment #5) > (From update of attachment 160776 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160776&action=review > > > Source/WebCore/ChangeLog:21 > > + (WebCore): > > Let's remove those useless lines. > Done. > > Source/WebCore/html/HTMLFormElement.cpp:694 > > + m_wasDemoted = sourceElement.m_wasDemoted; > > Shouldn't you copy the other fields instead of just copying m_wasDemoted? > Need to do some digging before we can decide on which are all the other fields that need to be copied. Most of the other fields are filled by the either by form's children or hold its current state(reached by some user interaction, etc) which AFAICT should not be copied when the element is cloned. As this issue req some digging and can be handled independently of this bug, I'd prefer/suggest to take it up in a separate bug. > > Source/WebCore/html/HTMLFormElement.h:136 > > + virtual void copyNonAttributePropertiesFromElement(const Element&); > > OVERRIDE. > Done.

Julien Chaffraix

Comment 8 2012-09-05 16:22:06 PDT

Comment on attachment 161926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161926&action=review > Source/WebCore/html/HTMLFormElement.cpp:694 > + m_wasDemoted = sourceElement.m_wasDemoted; > Need to do some digging before we can decide on which are all the other fields that need to be copied. > [snip] I'd prefer/suggest to take it up in a separate bug. I really would rather that we investigate now which flags should be copied. The rationale being that we likely have other bugs in this code. You have an easy way to reproduce them and the big picture so it's easier to do it now rather than later. I am not a proponent of whack-o-moling bugs as a model for developing.

Pravin D

Comment 9 2012-09-19 05:43:08 PDT

(In reply to comment #8) > (From update of attachment 161926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161926&action=review > > > Source/WebCore/html/HTMLFormElement.cpp:694 > > + m_wasDemoted = sourceElement.m_wasDemoted; > > > Need to do some digging before we can decide on which are all the other fields that need to be copied. > > [snip] I'd prefer/suggest to take it up in a separate bug. > > I really would rather that we investigate now which flags should be copied. The rationale being that we likely have other bugs in this code. You have an easy way to reproduce them and the big picture so it's easier to do it now rather than later. I am not a proponent of whack-o-moling bugs as a model for developing. > Did some digging on the issue. Following are the private members of the class HtmlFormElement with some rationale as to whether they need to be copied or not: FormSubmission::Attributes m_attributes -- Form specific attribute values. This is copied as part of cloneAttributesFromElement() during cloning. CheckedRadioButtons m_checkedRadioButtons Vector<FormAssociatedElement*> m_associatedElements; Vector<HTMLImageElement*> m_imageElements; unsigned m_associatedElementsBeforeIndex; unsigned m_associatedElementsAfterIndex; The above members are updated as and when a new element is added to the form. Thus these need not be copied. bool m_wasUserSubmitted; bool m_isSubmittingOrPreparingForSubmission; bool m_shouldSubmit; These carry user action state. That is various states values to help the form submission event. These also need not be copied as we want a cloned node to in a initial states. bool m_isInResetFunction - Is set when form.reset() is called. Not needed as we are already in the initial state for a cloned node. bool m_wasMalformed -- This member is currently not being set. It is always false. Its used in RenderBlock::layoutBlock() { .... if (n && n->hasTagName(formTag) && static_cast<HTMLFormElement*>(n)->isMalformed()) { // See if this form is malformed (i.e., unclosed). If so, don't give the form // a bottom margin. setMaxMarginAfterValues(0, 0); } .... } So either we have a bug due to m_wasMalformed not being set to true(unable to come up with a test case :( ) or its no more required and we can remove it altogether... Req your opinion in this regard. bool m_wasDemoted -- Set by HTML tree builder when form element is within a table element(other than a table cell). There can be a case when a form element within a table is cloned and attached to non-table element. In this case m_wasDemoted must be false. Required to be copied selectively. Please let know how to proceed from here...

Julien Chaffraix

Comment 10 2012-10-01 10:55:07 PDT

Comment on attachment 161926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161926&action=review >>> Source/WebCore/html/HTMLFormElement.cpp:694 >>> + m_wasDemoted = sourceElement.m_wasDemoted; >> >> > > Did some digging on the issue. Following are the private members of the class HtmlFormElement with some rationale as to whether they need to be copied or not: > > FormSubmission::Attributes m_attributes -- Form specific attribute values. > This is copied as part of cloneAttributesFromElement() during cloning. > > > CheckedRadioButtons m_checkedRadioButtons > Vector<FormAssociatedElement*> m_associatedElements; > Vector<HTMLImageElement*> m_imageElements; > unsigned m_associatedElementsBeforeIndex; > unsigned m_associatedElementsAfterIndex; > > The above members are updated as and when a new element is added to the form. Thus these need not be copied. > > bool m_wasUserSubmitted; > bool m_isSubmittingOrPreparingForSubmission; > bool m_shouldSubmit; > These carry user action state. That is various states values to help the form submission event. These also need not be copied as we want a cloned node to in a initial states. > > > bool m_isInResetFunction - Is set when form.reset() is called. Not needed as we are already in the initial state for a cloned node. > > bool m_wasMalformed -- This member is currently not being set. It is always false. > Its used in > RenderBlock::layoutBlock() { > .... > if (n && n->hasTagName(formTag) && static_cast<HTMLFormElement*>(n)->isMalformed()) { > // See if this form is malformed (i.e., unclosed). If so, don't give the form > // a bottom margin. > setMaxMarginAfterValues(0, 0); > } > .... > } > So either we have a bug due to m_wasMalformed not being set to true(unable to come up with a test case :( ) or its no more required and we can remove it altogether... Req your opinion in this regard. > > bool m_wasDemoted -- Set by HTML tree builder when form element is within a table element(other than a table cell). There can be a case when a form element within a table is cloned and attached to non-table element. In this case m_wasDemoted must be false. Required to be copied selectively. > > Please let know how to proceed from here... I did a search on m_wasMalformed and came with the same conclusion. This boolean is old and it's probably unused following some refactoring. HTML5 doesn't mention that so it should be fine to try removing it in a follow-up change. Your analysis is sound, could you submit an updated patch including a summary comment about why we just need to copy m_wasDemoted in the function.

Pravin D

Comment 11 2012-10-02 08:11:45 PDT

Created attachment 166689 [details] Patch

Pravin D

Comment 12 2012-10-02 08:19:49 PDT

Comment on attachment 166689 [details] Patch The new patch contains the following changes: Added relevant functions to copy member variable m_wasDemoted during cloning. Removed m_wasMalformed member variable and code fragments using it. PS: Not sure if the change log comments on why only m_wasDemoted needs to be copied has come out properly or not :(

Pravin D

Comment 13 2012-10-03 07:58:43 PDT

Created attachment 166895 [details] Patch

Darin Adler

Comment 14 2012-10-03 09:38:39 PDT

Comment on attachment 166895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166895&action=review I’d like this patch better if removing the unused m_wasMalformed was done in a separate patch. There’s no reason to mix removal of the dead code with the bug fix. > Source/WebCore/html/HTMLFormElement.cpp:352 > - else if (firstSuccessfulSubmitButton == 0 && control->isSuccessfulSubmitButton()) > + else if (!firstSuccessfulSubmitButton && control->isSuccessfulSubmitButton()) This reformatting and code style fix of otherwise untouched code is not a good idea for a patch that is not otherwise changing this function. > Source/WebCore/html/HTMLFormElement.cpp:489 > - && (static_cast<Element*>(node)->isFormControlElement() > - || node->hasTagName(objectTag)) > - && toHTMLElement(node)->form() == this) > + && (static_cast<Element*>(node)->isFormControlElement() > + || node->hasTagName(objectTag)) > + && toHTMLElement(node)->form() == this) This reformatting and code style fix of otherwise untouched code is not a good idea for a patch that is not otherwise changing this function. > Source/WebCore/html/HTMLFormElement.cpp:693 > + const HTMLFormElement& sourceElement = static_cast<const HTMLFormElement&>(source); > + > + m_wasDemoted = sourceElement.m_wasDemoted; I don’t think the local variables adds sufficient value here. This would read better as a single line.

Pravin D

Comment 15 2012-10-03 12:47:07 PDT

(In reply to comment #14) > (From update of attachment 166895 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166895&action=review > > I’d like this patch better if removing the unused m_wasMalformed was done in a separate patch. There’s no reason to mix removal of the dead code with the bug fix. > Comment #10 suggests that is will be fine to include the dead code removal in the next patch... Also IMHO it makes more sense removing the dead code here than creating a new bug that wud not have a proper test case, etc... Anyways I'll upload a new patch with only the relevant changes... > > Source/WebCore/html/HTMLFormElement.cpp:352 > > - else if (firstSuccessfulSubmitButton == 0 && control->isSuccessfulSubmitButton()) > > + else if (!firstSuccessfulSubmitButton && control->isSuccessfulSubmitButton()) > > This reformatting and code style fix of otherwise untouched code is not a good idea for a patch that is not otherwise changing this function. > ok... > > Source/WebCore/html/HTMLFormElement.cpp:489 > > - && (static_cast<Element*>(node)->isFormControlElement() > > - || node->hasTagName(objectTag)) > > - && toHTMLElement(node)->form() == this) > > + && (static_cast<Element*>(node)->isFormControlElement() > > + || node->hasTagName(objectTag)) > > + && toHTMLElement(node)->form() == this) > > This reformatting and code style fix of otherwise untouched code is not a good idea for a patch that is not otherwise changing this function. > ok... > > Source/WebCore/html/HTMLFormElement.cpp:693 > > + const HTMLFormElement& sourceElement = static_cast<const HTMLFormElement&>(source); > > + > > + m_wasDemoted = sourceElement.m_wasDemoted; > > I don’t think the local variables adds sufficient value here. This would read better as a single line. > Will do it...

Julien Chaffraix

Comment 16 2012-10-03 12:59:00 PDT

Comment on attachment 166895 [details] Patch > Comment #10 suggests that is will be fine to include the dead code removal in the next patch... Also IMHO it makes more sense removing the dead code here than creating a new bug that wud not have a proper test case, etc... That was not my intent to let your bundle the removal into this patch. I thought the wording of comment #10 was precise on that point, sorry for not being clear: "it should be fine to try removing it in a follow-up change." Atomic (as in one change only) patches are always better as they are usually smaller, easier to review and convey more information when blaming the code.

Pravin D

Comment 17 2012-10-03 13:58:24 PDT

Created attachment 166956 [details] Patch

Pravin D

Comment 18 2012-10-03 14:02:20 PDT

Comment on attachment 166956 [details] Patch @Darin Have uploaded a patch with the suggested changes. @Darin, @Julien, Can I create a new bug for the dead code removal(m_wasMalformed related)? And also cite Comment #9 and Comment #10 in the same?

Julien Chaffraix

Comment 19 2012-10-04 12:37:26 PDT

> @Darin, @Julien, > Can I create a new bug for the dead code removal(m_wasMalformed related)? And also cite Comment #9 and Comment #10 in the same? Please do, you can also link the follow-up bug here for the archive.

WebKit Review Bot

Comment 20 2012-10-04 13:09:33 PDT

Comment on attachment 166956 [details] Patch Clearing flags on attachment: 166956 Committed r130422: <http://trac.webkit.org/changeset/130422>

Pravin D

Comment 21 2012-10-04 13:26:17 PDT

I have created bug 98444 for removing code related to m_wasMalformed.

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