Closed
Bug 555567
Opened 14 years ago
Closed 14 years ago
Fix the form attribute behavior of the legend element
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: mounir, Assigned: mounir)
References
()
Details
(Keywords: dev-doc-complete, html5)
Attachments
(2 files, 3 obsolete files)
2.32 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
2.44 KB,
patch
|
Details | Diff | Splinter Review |
The spec says: "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." At the moment, the form IDL attribute is returning null only if the legend element is not in a form element (regular behavior).
Assignee | ||
Comment 1•14 years ago
|
||
Those tests need html5 parser because the current parser creates a fieldset element when a legend element has no fieldset. I don't think that's a correct behavior but according to hsivonen that's not worth opening a bug (and the feature i want to test is related to html5 so...). I don't know why but it looks like the test doesn't finish to load correctly. I have "Transferring data from mochi.test..." in the status bar. If someone has an idea ?
Attachment #435600 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•14 years ago
|
||
This patch is inspired from nsHTMLOptionElement which has the exact same behavior for form with select element.
Attachment #435601 -
Flags: review?(bzbarsky)
Comment 3•14 years ago
|
||
Wouldn't we want to walk up the parent chain in the flattened tree instead of the regular parent chain here?
Comment 4•14 years ago
|
||
Not as the HTML spec is defined right now; the property is just part of the DOM with no reference to XBL. Note that we don't really handle mixtures of XBL and <legend> well in any case. ;)
Comment 5•14 years ago
|
||
Comment on attachment 435601 [details] [diff] [review] Patch v0.1 This doesn't look right to me. I would expect to look at only the immediate parent of the <legend>, not up the whole parent chain. Otherwise a figure in a fieldset would have its legend associated with the form, no?
Attachment #435601 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 6•14 years ago
|
||
Updating the tests to not run them when html5 parser isn't enabled (as requested by hsivonen on another bug where the tests were only working with html5 parser).
Attachment #435600 -
Attachment is obsolete: true
Attachment #441779 -
Flags: review?(bzbarsky)
Attachment #435600 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•14 years ago
|
||
I was also wondering why a code I've seen was looking to the entire parent chain. I was thinking it was for something I didn't get. Unfortunately I don't remember the code I've seen doing that.
Attachment #435601 -
Attachment is obsolete: true
Attachment #441780 -
Flags: review?(bzbarsky)
Comment 8•14 years ago
|
||
Comment on attachment 441780 [details] [diff] [review] Patch v2 >diff -r 7d8b63dfd7d8 content/html/content/src/nsHTMLLegendElement.cpp >--- a/content/html/content/src/nsHTMLLegendElement.cpp Tue Apr 27 14:26:00 2010 +0200 >+++ b/content/html/content/src/nsHTMLLegendElement.cpp Tue Apr 27 14:26:37 2010 +0200 >@@ -101,16 +101,24 @@ public: > } > virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, > nsIAtom* aPrefix, const nsAString& aValue, > PRBool aNotify); > virtual nsresult UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute, > PRBool aNotify); > > virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const; >+ >+protected: >+ /** >+ * Get the fieldset content element that contains this legend, this >+ * intentionally does not return nsresult, all we care about is if >+ * there is a fieldset associated with this legend or not. >+ */ >+ nsIContent* GetFieldSet(); > }; > > > NS_IMPL_NS_NEW_HTML_ELEMENT(Legend) > > > nsHTMLLegendElement::nsHTMLLegendElement(nsINodeInfo *aNodeInfo) > : nsGenericHTMLFormElement(aNodeInfo) >@@ -140,33 +148,49 @@ NS_HTML_CONTENT_INTERFACE_TABLE_TAIL_CLA > > > NS_IMPL_ELEMENT_CLONE(nsHTMLLegendElement) > > > NS_IMETHODIMP > nsHTMLLegendElement::GetForm(nsIDOMHTMLFormElement** aForm) > { >- return nsGenericHTMLFormElement::GetForm(aForm); >+ *aForm = nsnull; >+ >+ nsCOMPtr<nsIFormControl> fieldsetControl = do_QueryInterface(GetFieldSet()); >+ >+ return fieldsetControl ? fieldsetControl->GetForm(aForm) : NS_OK; > } > > > NS_IMPL_STRING_ATTR(nsHTMLLegendElement, AccessKey, accesskey) > NS_IMPL_STRING_ATTR(nsHTMLLegendElement, Align, align) > > // this contains center, because IE4 does > static const nsAttrValue::EnumTable kAlignTable[] = { > { "left", NS_STYLE_TEXT_ALIGN_LEFT }, > { "right", NS_STYLE_TEXT_ALIGN_RIGHT }, > { "center", NS_STYLE_TEXT_ALIGN_CENTER }, > { "bottom", NS_STYLE_VERTICAL_ALIGN_BOTTOM }, > { "top", NS_STYLE_VERTICAL_ALIGN_TOP }, > { 0 } > }; > >+nsIContent* >+nsHTMLLegendElement::GetFieldSet() >+{ >+ nsIContent* parent = GetParent(); >+ >+ if (parent && parent->Tag() == nsGkAtoms::fieldset) { >+ return parent; >+ } >+ >+ return nsnull; >+} >+ > PRBool > nsHTMLLegendElement::ParseAttribute(PRInt32 aNamespaceID, > nsIAtom* aAttribute, > const nsAString& aValue, > nsAttrValue& aResult) > { > if (aAttribute == nsGkAtoms::align && aNamespaceID == kNameSpaceID_None) { > return aResult.ParseEnumValue(aValue, kAlignTable, PR_FALSE);
Attachment #441780 -
Flags: review?(bzbarsky) → review+
Comment 9•14 years ago
|
||
Wait. How is that GetFieldSet() function correct? It doesn't make sure the node is HTML... I guess non-HTML ones probably won't QI to nsIFormControl...
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9) > Wait. How is that GetFieldSet() function correct? It doesn't make sure the > node is HTML... I guess non-HTML ones probably won't QI to nsIFormControl... Would you prefer: >+nsIContent* >+nsHTMLLegendElement::GetFieldSet() >+{ >+ nsIContent* parent = GetParent(); >+ >+ if (parent && parent->IsHTML() && parent->Tag() == nsGkAtoms::fieldset) { >+ return parent; >+ } >+ >+ return nsnull; >+} As QI to nsIFormControl shouldn't work for non-html elements, I assume |IsHTML| call isn't needed but if you think it would make this code safer, I see no problem.
Comment 11•14 years ago
|
||
I'd say yes, include that IsHTML() check there, for future proofing if this is ever more widely used if nothing else. And no, I don't know why bugzilla quoted the whole patch when I r+'ed this patch :(
Updated•14 years ago
|
Attachment #441779 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > I'd say yes, include that IsHTML() check there, for future proofing if this is > ever more widely used if nothing else. I will fix that. > And no, I don't know why bugzilla quoted the whole patch when I r+'ed this > patch :( No problem ;) Considering the size of the patch and considering it's not changing the API, is a sr needed ?
Comment 14•14 years ago
|
||
No sr needed here.
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #14) > No sr needed here. Ok, thanks. Marked checkin-needed with r=jst.
Keywords: checkin-needed
Comment 16•14 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/b9f61d766754 and http://hg.mozilla.org/mozilla-central/rev/71b412c3b11d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a5
Updated•14 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•