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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: mounir, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(2 files, 3 obsolete files)

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).
Attached patch Tests v0.1 (obsolete) — Splinter Review
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)
Attached patch Patch v0.1 (obsolete) — Splinter Review
This patch is inspired from nsHTMLOptionElement which has the exact same behavior for form with select element.
Attachment #435601 - Flags: review?(bzbarsky)
Wouldn't we want to walk up the parent chain in the flattened tree instead of the regular parent chain here?
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 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-
Attached patch Tests v2Splinter Review
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)
Attached patch Patch v2 (obsolete) — Splinter Review
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 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+
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...
(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.
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 :(
Attachment #441779 - Flags: review?(bzbarsky) → review+
(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 ?
Attached patch Patch v2.1Splinter Review
r=jst
Attachment #441780 - Attachment is obsolete: true
No sr needed here.
(In reply to comment #14)
> No sr needed here.

Ok, thanks.

Marked checkin-needed with r=jst.
Keywords: checkin-needed
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
Depends on: 565611
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.