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
Created attachment 172051 [details] Patch
tkent will want to look.
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).
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.
(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.
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.
(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.
(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 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.
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.
Created attachment 172287 [details] Patch Take Kent Tamura's feedback into consideration.
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.
Created attachment 172292 [details] Patch Take Kent Tamura's feedback into consideration.
Any feedback on the latest patch?
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 on attachment 172292 [details] Patch Let's go ahead.
Comment on attachment 172292 [details] Patch Clearing flags on attachment: 172292 Committed r134510: <http://trac.webkit.org/changeset/134510>
All reviewed patches have been landed. Closing bug.