Bug 101044 – Make HTMLLegendElement.form behave according to specification

• Component: Forms
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 172051 [details]
Patch

[Kentaro Hara]

tkent will want to look.

[Ryosuke Niwa]

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

[Chris Dumez]

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.

[Ryosuke Niwa]

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

[Chris Dumez]

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.

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Kent Tamura]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 172287 [details]
Patch

Take Kent Tamura's feedback into consideration.

[Kent Tamura]

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.

[Chris Dumez]

Created attachment 172292 [details]
Patch

Take Kent Tamura's feedback into consideration.

[Chris Dumez]

Any feedback on the latest patch?

[Kent Tamura]

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.

[Kent Tamura]

Comment on attachment 172292 [details]
Patch

Let's go ahead.

[WebKit Review Bot]

Comment on attachment 172292 [details]
Patch

Clearing flags on attachment: 172292

Committed r134510: <http://trac.webkit.org/changeset/134510>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.