Bug 88632 - REGRESSION(r116487?): HTMLFormElement::elements['name'] is empty if the form is detached from the document tree
Summary: REGRESSION(r116487?): HTMLFormElement::elements['name'] is empty if the form ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Rakesh
URL:
Keywords:
Depends on:
Blocks: 88825
  Show dependency treegraph
 
Reported: 2012-06-08 01:43 PDT by Kent Tamura
Modified: 2012-06-11 17:50 PDT (History)
3 users (show)

See Also:


Attachments
patch1 (9.66 KB, patch)
2012-06-08 07:48 PDT, Rakesh
no flags Details | Formatted Diff | Diff
patch_for_landing (9.53 KB, patch)
2012-06-08 13:12 PDT, Rakesh
no flags Details | Formatted Diff | Diff
patch (9.59 KB, patch)
2012-06-08 14:11 PDT, Rakesh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Rakesh 2012-06-08 02:33:02 PDT
will check this issue.
Comment 2 Rakesh 2012-06-08 07:48:37 PDT
Created attachment 146569 [details]
patch1

Root node of radionode list needs to updated to form if detached from dom tree
Comment 3 Ryosuke Niwa 2012-06-08 11:42:15 PDT
Comment on attachment 146569 [details]
patch1

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

Please fix nits in the test before you land.

> Source/WebCore/dom/Node.cpp:2941
> +    ASSERT(hasTagName(formTag));
> +    if (!hasRareData() || !rareData()->nodeLists())
> +        return;
> +
> +    NodeListsNodeData::RadioNodeListCache cache = rareData()->nodeLists()->m_radioNodeListCache;
> +    for (NodeListsNodeData::RadioNodeListCache::iterator it = cache.begin(); it != cache.end(); ++it)
> +        it->second->setRootElement(toElement(this));

It seems like we have the same problem in LabelsNodeList and others that can be rooted at the document node.
We might want to fix them as well in a follow up patch.

> LayoutTests/fast/forms/radionodelist-whose-form-element-detached-from-domtree.html:10
> +    <input type="radio" name="type1" value="rbarrval1">

Why is the value "rbarrval1"? Can we use some other human-understandable value instead?

> LayoutTests/fast/forms/radionodelist-whose-form-element-detached-from-domtree.html:18
> +    <input type="checkbox" name="cb" value="cbval1">

Ditto.
Comment 4 Rakesh 2012-06-08 13:12:55 PDT
Created attachment 146634 [details]
patch_for_landing

Fixed nits suggested.
Comment 5 Rakesh 2012-06-08 13:17:01 PDT
(In reply to comment #3)
Thanks for reviewing this patch.
> 
> It seems like we have the same problem in LabelsNodeList and others that can be rooted at the document node.
> We might want to fix them as well in a follow up patch.
> 
Will check those.

> Why is the value "rbarrval1"? Can we use some other human-understandable value instead?
> 
Fixed.
Comment 6 Ryosuke Niwa 2012-06-08 13:31:02 PDT
Comment on attachment 146634 [details]
patch_for_landing

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

> LayoutTests/fast/forms/radionodelist-whose-form-element-detached-from-domtree-expected.txt:10
> +PASS radioNodeList1[0].value is 'val1'

Please spell out value.
Comment 7 Rakesh 2012-06-08 14:11:01 PDT
Created attachment 146647 [details]
patch

Better element values
Comment 8 WebKit Review Bot 2012-06-08 19:28:35 PDT
Comment on attachment 146647 [details]
patch

Rejecting attachment 146647 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12918649
Comment 9 WebKit Review Bot 2012-06-08 20:50:18 PDT
Comment on attachment 146647 [details]
patch

Clearing flags on attachment: 146647

Committed r119891: <http://trac.webkit.org/changeset/119891>
Comment 10 WebKit Review Bot 2012-06-08 20:50:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Rakesh 2012-06-09 00:08:36 PDT
Thanks rniwa, I did not see that commit failed, I was not available, it was 2AM already.
Comment 12 Ryosuke Niwa 2012-06-11 17:36:39 PDT
On my second thought, this patch doesn't fix the bug completely in some edge cases.
Comment 13 Ryosuke Niwa 2012-06-11 17:38:08 PDT
I'm rolling out the patch because the fix is incorrect.
Comment 14 Ryosuke Niwa 2012-06-11 17:48:45 PDT
Apparently sheriffbot can't roll out this patch. I'll file a new bug and manually revert this change and make the right fix.