Bug 101044 - Make HTMLLegendElement.form behave according to specification
Summary: Make HTMLLegendElement.form behave according to specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://dev.w3.org/html5/spec/single-p...
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-11-02 05:35 PDT by Chris Dumez
Modified: 2012-11-13 17:09 PST (History)
12 users (show)

See Also:


Attachments
Patch (6.41 KB, patch)
2012-11-02 06:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2012-11-02 10:02 PDT, Chris Dumez
tkent: review-
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2012-11-05 00:46 PST, Chris Dumez
tkent: review-
tkent: commit-queue-
Details | Formatted Diff | Diff
Patch (7.59 KB, patch)
2012-11-05 01:43 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-11-02 05:35:52 PDT
As per the HTML5 specification (http://dev.w3.org/html5/spec/single-page.html#dom-legend-form):
"The form IDL attribute's behavior depends on whether the legend element is in a fieldset element or not. If the legend has a fieldset element as its parent, then the form IDL attribute must return the same value as the form IDL attribute on that fieldset element. Otherwise, it must return null."

WebKit currently returns the legend element's form ancestor even if the parent is not a fieldset, which is not according to spec.

- Firefox behaves according to specification here
- Opera does not follow the specification
- I don't know about other browsers
Comment 1 Chris Dumez 2012-11-02 06:52:40 PDT
Created attachment 172051 [details]
Patch
Comment 2 Kentaro Hara 2012-11-02 06:54:47 PDT
tkent will want to look.
Comment 3 Ryosuke Niwa 2012-11-02 09:54:53 PDT
Comment on attachment 172051 [details]
Patch

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

What do other browsers do?

> LayoutTests/ChangeLog:3
> +        HTMLLegendElement.form should be null it does not have a fieldset element as parent

Missing "when" between "null" and "it".

> Source/WebCore/html/HTMLLegendElement.cpp:92
> +    ContainerNode* ancestor = parentNode();
> +    if (!ancestor->hasTagName(fieldsetTag))

What's the point of ancestor variable? You can just do parentNode()->hasTagName(fieldsetTag).
Comment 4 Chris Dumez 2012-11-02 10:02:07 PDT
Created attachment 172083 [details]
Patch

Take rniwa's feedback into consideration.

The bug description already states the behavior of other browsers I could test:
- Firefox: Like spec (and patch)
- Opera: Does not follow spec
- I don't have IE to test.
Comment 5 Ryosuke Niwa 2012-11-02 10:31:16 PDT
(In reply to comment #4)
> Created an attachment (id=172083) [details]
> Patch
> 
> Take rniwa's feedback into consideration.
> 
> The bug description already states the behavior of other browsers I could test:

Ah, I didn’t see that. We should probably test this on IE.
Comment 6 Chris Dumez 2012-11-02 10:53:46 PDT
I got access to a Windows 7 machine, IE 9 does not follow the spec.
I could not try IE 10 because it appears to require Windows 8.
Comment 7 Chris Dumez 2012-11-02 11:25:07 PDT
(In reply to comment #6)
> I got access to a Windows 7 machine, IE 9 does not follow the spec.
> I could not try IE 10 because it appears to require Windows 8.

I have used http://netrenderer.com, it seems IE10 consumer preview does not follow the spec either.

So only Firefox seems to follow the specification here.
Comment 8 Ryosuke Niwa 2012-11-02 11:32:51 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > I got access to a Windows 7 machine, IE 9 does not follow the spec.
> > I could not try IE 10 because it appears to require Windows 8.
> 
> I have used http://netrenderer.com, it seems IE10 consumer preview does not follow the spec either.
> 
> So only Firefox seems to follow the specification here.

I’m worried that the spec’ed behavior might not be Web-compatible, particularly with mobile sites where WebKit has a dominant market share. I’m curious as to know why the spec mandates this behavior given authors don’t use fieldset elements that often.
Comment 9 Kent Tamura 2012-11-04 20:37:31 PST
Comment on attachment 172083 [details]
Patch

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

I don't decide whether this change should be landed or not. Anyway, the code is wrong.

> LayoutTests/fast/forms/form-attribute.html:88
> +debug('- Ensures that the form attribute of legend element depends on whether it is in a fieldset element or not.');
> +container.innerHTML = '<form id=owner>' +
> +    '    <fieldset><legend id=legendElement1 name=victim /></fieldset>' +
> +    '    <legend id=legendElement2 name=victim />' +
> +    '</form>';
> +owner = document.getElementById('owner');
> +var legendElement1 = document.getElementById('legendElement1');
> +var legendElement2 = document.getElementById('legendElement2');
> +shouldBe('legendElement1.form', 'owner');
> +shouldBe('legendElement2.form', 'null');
> +
> +debug('');

Please do not add tests to this file.  form-attribute.html is a test file for "form" content attribute.
You should make LayoutTests/fast/forms/legend/legend-form.html

> Source/WebCore/html/HTMLLegendElement.cpp:94
> +    // As per the specification, we should return null if the
> +    // legend does not have a fieldset element as its parent.
> +    if (!parentNode()->hasTagName(fieldsetTag))
> +        return 0;
> +
> +    return findFormAncestor();

parentNode() can be 0.
Also, this doesn't match to the specification, which says "must return the same value as the form IDL attribute on that fieldset element."
If the parent <fieldset> has "form" content attribute, <fieldset>'s form is not the ancestor form.

> Source/WebCore/html/HTMLLegendElement.h:45
> +    virtual HTMLFormElement* virtualForm() const;

Need to append OVERRIDE.
Comment 10 Chris Dumez 2012-11-04 22:42:42 PST
Thanks for the review Kent. I'll fix the patch and reupload. 

I overlooked the fact that the legend should return the same form as its parent fieldset.
Comment 11 Chris Dumez 2012-11-05 00:46:47 PST
Created attachment 172287 [details]
Patch

Take Kent Tamura's feedback into consideration.
Comment 12 Kent Tamura 2012-11-05 00:50:54 PST
Comment on attachment 172287 [details]
Patch

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

> Source/WebCore/html/HTMLLegendElement.cpp:95
> +    ContainerNode* fieldset = parentNode();
> +    while (fieldset && !fieldset->hasTagName(fieldsetTag))
> +        fieldset = fieldset->parentNode();

We should check only parentNode of this element, not ancestors.
Comment 13 Chris Dumez 2012-11-05 01:43:33 PST
Created attachment 172292 [details]
Patch

Take Kent Tamura's feedback into consideration.
Comment 14 Chris Dumez 2012-11-07 02:38:57 PST
Any feedback on the latest patch?
Comment 15 Kent Tamura 2012-11-07 22:33:41 PST
IMO, it's ok to proceed the behavior change.

* I have no idea of use-cases of HTMLLegendElement.form. I guess it's not used so often.
* The behavior will be changed if
    * <legend> is not in a <fieldset> parent.  (I guess it's really rare.)
    * The parent <fieldset> has "form" content attribute. (ditto.)
So, the impact of the behavior change must be very small.
Comment 16 Kent Tamura 2012-11-13 16:59:35 PST
Comment on attachment 172292 [details]
Patch

Let's go ahead.
Comment 17 WebKit Review Bot 2012-11-13 17:09:18 PST
Comment on attachment 172292 [details]
Patch

Clearing flags on attachment: 172292

Committed r134510: <http://trac.webkit.org/changeset/134510>
Comment 18 WebKit Review Bot 2012-11-13 17:09:23 PST
All reviewed patches have been landed.  Closing bug.