Summary: | Make HTMLLegendElement.form behave according to specification | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Forms | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, adele, ap, eoconnor, haraken, ian, mifenton, mjs, rniwa, syoichi, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | http://dev.w3.org/html5/spec/single-page.html#dom-legend-form | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-11-02 05:35:52 PDT
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. |