Closed Bug 588683 Opened 14 years ago Closed 14 years ago

Implement form attribute

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: mounir, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 4 obsolete files)

HTML5 lets an element to be associated with an arbitrary form (not an ancestor) with @form (.form only returns the current form owner).
Do we know what is the use case for this? And does anyone else implement this?
...because to me this sounds somewhat bad API.
It can be convenient for situations where you have a form which is almost conceptually held together but where one or two controls live somewhere else in the DOM.

It's required if you have multiple forms which are mixed up with each other if you look in tree-order. For example a table where each column is a separate form.
Asking for blocking2.0: we want this HTML5 Forms feature for ff4.
blocking2.0: --- → ?
Attached patch Patch v1 (obsolete) — Splinter Review
This patch passes all my tests but I have a leak. Actually, I think it may be related to the patch but I really don't see where it comes from. Jonas and Johnny, let me know if you see something while reviewing.
Attachment #467687 - Flags: superreview?(jst)
Attachment #467687 - Flags: review?(jonas)
Attached patch Patch v1.1 (obsolete) — Splinter Review
This is refactoring a bit the code, fixing issues reported by the try server and fixing the memory leak.
Attachment #467687 - Attachment is obsolete: true
Attachment #467926 - Flags: superreview?(jst)
Attachment #467926 - Flags: review?(jonas)
Attachment #467687 - Flags: superreview?(jst)
Attachment #467687 - Flags: review?(jonas)
Comment on attachment 467926 [details] [diff] [review]
Patch v1.1

>@@ -2581,16 +2555,21 @@ nsGenericHTMLFormElement::BeforeSetAttr(
>       // Removing the element from the form can make it not be the default
>       // control anymore.  Go ahead and notify on that change, though we might
>       // end up readding and becoming the default control again in
>       // AfterSetAttr.
>       if (doc && aNotify) {
>         doc->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_DEFAULT);
>       }
>     }
>+
>+    if (aName == nsGkAtoms::form) {
>+      // The form id observer is no longer needed.
>+      RemoveFormIdObserver();
>+    }

The comment isn't entirely correct. Say something like:
The form id observer may no longer needed. If it is needed it will be re-added in AfterSetAttr.

>@@ -2629,16 +2608,28 @@ nsGenericHTMLFormElement::AfterSetAttr(P
>       // Adding the element to the form can make it be the default control .
>       // Go ahead and notify on that change.
>       // Note: no need to notify on CanBeDisabled(), since type attr
>       // changes can't affect that.
>       if (doc && aNotify) {
>         doc->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_DEFAULT);
>       }
>     }
>+
>+    if (aName == nsGkAtoms::form) {
>+      // We need a new form id observer.
>+      nsIDocument* doc = GetCurrentDoc();
>+      if (doc) {
>+        AddFormIdObserver();
>+
>+        // Because we have a now @form value, we have to update our form owner.
>+        ClearForm(PR_TRUE, aNotify);
>+        UpdateFormOwner(false);

It might make sense to do these things as part of AddFormIdObserver since the observer API returns the current element.

Also, shouldn't you only do this if aValue is non-null and non-empty, right?

>+nsGenericHTMLFormElement::AddFormIdObserver()
>+{
>+  NS_ASSERTION(GetCurrentDoc(), "When adding a form id observer, \
>+we should be in a document!");

Do

NS_ASSERTION(GetCurrentDoc(), "When adding a form id observer,"
                              "we should be in a document!");
or
NS_ASSERTION(GetCurrentDoc(),
             "When adding a form id observer, we should be in a document!");

(not sure if the latter is too long)


>+  nsAutoString formId;
>+  nsIDocument* doc = GetOwnerDoc();
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId);
>+  nsCOMPtr<nsIAtom> atom = do_GetAtom(formId);

Assert that 'formId' is non-empty.

>+nsGenericHTMLFormElement::RemoveFormIdObserver()
>+{
>+  /**
>+   * We are using GetOwnerDoc() because we don't really care about having the
>+   * element actually being in the tree. If it is not and @form value changes,
>+   * this method will be called for nothing but removing an observer which does
>+   * not exist doesn't cost so much (no entry in the hash table) so having a
>+   * boolean for GetCurrentDoc()/GetOwnerDoc() would make everything look more
>+   * complex for nothing.
>+   */
>+
>+  nsIDocument* doc = GetOwnerDoc();
>+
>+  // At this point, we may not have a document anymore. In that case, we can't
>+  // remove the observer. The document did that for us.
>+  if (!doc) {
>+    return;
>+  }
>+
>+  nsAutoString formId;
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId);

It's probably worth checking that formId isn't empty before doing anything else here given that this is called every time we're unbound from the document. The two hash-lookups (atom and id-hash) aren't *that* cheap.

>+PRBool
>+nsGenericHTMLFormElement::FormIdUpdated(Element* aOldElement,
>+                                        Element* aNewElement,
>+                                        void* aData)
>+{
>+  NS_ASSERTION(static_cast<nsIContent*>(aData)->IsHTML(),
>+               "aData should be an HTML element!");
>+
>+  nsGenericHTMLFormElement* element =
>+    static_cast<nsGenericHTMLFormElement*>(aData);

Somewhat scary that you're casting to two different classes here. I.e. you're assuming that reinterpret_cast-ing nsGenericHTMLFormElement to nsIContent is safe. Would be better to put the assertion after this cast and assert that element->IsHTML() returns true.

>+  element->ClearForm(PR_TRUE, PR_FALSE);
>+  element->UpdateFormOwner(false);

Move the call to ClearForm to inside the UpdateFormOwner function. You always call ClearForm whenever UpdateFormOwner(false) is called.

>+nsGenericHTMLFormElement::UpdateFormOwner(bool aBindToTree)
>+{
>+  bool hadForm = mForm;
>+
>+  if (!mForm) {
>+    // If @form is set, we need to found the form this way.

"If @form is set, we'll use that to find the form."

>+    if (HasAttr(kNameSpaceID_None, nsGkAtoms::form)) {
>+      nsIDocument* doc = GetCurrentDoc();
>+
>+      if (doc) {
>+        nsAutoString formId;
>+        GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId);

Instead of a GetAttr inside a HasAttr, just call GetAttr in the outer |if|. You should probably also ignore the @form if it's empty. So something like

nsAutoString formId;
if (GetAttr(..., formId) && !formId.IsEmpty()) {
  ...


Still reviewing...
Comment on attachment 467926 [details] [diff] [review]
Patch v1.1

Looks good otherwise, but given the number of comments I think I'd like to see another patch.
Attachment #467926 - Flags: review?(jonas) → review-
(In reply to comment #7)
> >@@ -2629,16 +2608,28 @@ nsGenericHTMLFormElement::AfterSetAttr(P
> >       // Adding the element to the form can make it be the default control .
> >       // Go ahead and notify on that change.
> >       // Note: no need to notify on CanBeDisabled(), since type attr
> >       // changes can't affect that.
> >       if (doc && aNotify) {
> >         doc->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_DEFAULT);
> >       }
> >     }
> >+
> >+    if (aName == nsGkAtoms::form) {
> >+      // We need a new form id observer.
> >+      nsIDocument* doc = GetCurrentDoc();
> >+      if (doc) {
> >+        AddFormIdObserver();
> >+
> >+        // Because we have a now @form value, we have to update our form owner.
> >+        ClearForm(PR_TRUE, aNotify);
> >+        UpdateFormOwner(false);
> 
> It might make sense to do these things as part of AddFormIdObserver since the
> observer API returns the current element.

Calling UpdateFormOwner inside AddFormIdObserver would be a bad idea IMO.
However, I found a way to prevent getting two times the element.

> >+nsGenericHTMLFormElement::AddFormIdObserver()
> >+{
> >+  NS_ASSERTION(GetCurrentDoc(), "When adding a form id observer, \
> >+we should be in a document!");
> 
> Do
> 
> NS_ASSERTION(GetCurrentDoc(), "When adding a form id observer,"
>                               "we should be in a document!");

I didn't know it was a valid syntax! :)

> >+PRBool
> >+nsGenericHTMLFormElement::FormIdUpdated(Element* aOldElement,
> >+                                        Element* aNewElement,
> >+                                        void* aData)
> >+{
> >+  NS_ASSERTION(static_cast<nsIContent*>(aData)->IsHTML(),
> >+               "aData should be an HTML element!");
> >+
> >+  nsGenericHTMLFormElement* element =
> >+    static_cast<nsGenericHTMLFormElement*>(aData);
> 
> Somewhat scary that you're casting to two different classes here. I.e. you're
> assuming that reinterpret_cast-ing nsGenericHTMLFormElement to nsIContent is
> safe. Would be better to put the assertion after this cast and assert that
> element->IsHTML() returns true.

I'm not sure I got what you asked. Have a look at the new patch and let me know if I missed something.
FWIW, the assert is much more to prevent future bad usage of the callback, that's why I was only checking for static_cast<nsIContent*>(aData)->IsHTML(). Considering this call back can't be used by untrusted callers, we are pretty safe.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #467926 - Attachment is obsolete: true
Attachment #468069 - Flags: superreview?(jst)
Attachment #468069 - Flags: review?(jonas)
Attachment #467926 - Flags: superreview?(jst)
Comment on attachment 468069 [details] [diff] [review]
Patch v2

There seems to be a bug if @form is changed from one value to another. In AfterSetAttr we call UpdateFormOwner with aBindToTree=false, which means that we'll never call ClearForm to unregister from the old form. And if the @form attribute is changed from being set, to not being set, we don't even call into UpdateFormOwner.

The fix might be to change BeforeSetAttr to call ClearForm as appropriate.

Also, when you're calling ClearForm, you're always passing aNotify=PR_TRUE. This isn't always correct if AfterSetAttr/BeforeSetAttr was called with aNotify=false. In fact, it might be a security problem to do so since it might cause crashes.

Sorry for not finding these things earlier :(

Minor comments below:

>@@ -2535,16 +2503,24 @@ nsGenericHTMLFormElement::UnbindFromTree
>+  // We have to remove the form id observer if there was one.
>+  // We will re-add one later if needed (during bind to tree).
>+  nsAutoString formId;
>+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId) &&
>+      !formId.IsEmpty()) {
>+    RemoveFormIdObserver();
>+  }

Use nsContentUtils::HasNonEmptyAttr

>@@ -2581,16 +2557,28 @@ nsGenericHTMLFormElement::BeforeSetAttr(
>+    if (aName == nsGkAtoms::form) {
>+      // If @form isn't set or set to the empty string, there were no observer
>+      // so we don't have to remove it.
>+      nsAutoString formId;
>+      if (GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId) &&
>+          !formId.IsEmpty()) {
>+        // The current form id observer is no longer needed.
>+        // A new one may be added in AfterSetAttr.
>+        RemoveFormIdObserver();
>+      }
>+    }

Same here

>+nsGenericHTMLFormElement::UpdateFormOwner(bool aBindToTree,
>+                                          Element* aFormIdElement)
>+{

This function seems to assume aFormIdElement is null when aBindToTree is true. You should check this using
NS_PRECONDITION(!aBindToTree || !aFormIdElement, ...) and also document this in the header file.
Attachment #468069 - Flags: review?(jonas) → review-
Hmm.. another thing I realized. It would make sense to implement form="" to
mean that the element doesn't belong to *any* <form>. Even if it has a <form>
ancestor.

That would allow putting a <input> that isn't intended for submission to be put
inside the DOM without worrying about messing up ancestor <form>s.
(In reply to comment #12)
> Hmm.. another thing I realized. It would make sense to implement form="" to
> mean that the element doesn't belong to *any* <form>. Even if it has a <form>
> ancestor.
> 
> That would allow putting a <input> that isn't intended for submission to be put
> inside the DOM without worrying about messing up ancestor <form>s.

It's already the case actually. If @form is set, the form owner will be the form with the id in @form. If none, the element will have no form at all.
(In reply to comment #11)
> Comment on attachment 468069 [details] [diff] [review]
> Patch v2
> 
> There seems to be a bug if @form is changed from one value to another. In
> AfterSetAttr we call UpdateFormOwner with aBindToTree=false, which means that
> we'll never call ClearForm to unregister from the old form. And if the @form
> attribute is changed from being set, to not being set, we don't even call into
> UpdateFormOwner.

I don't follow. ClearForm is always called in UpdateFormOwner _if_ aBindToTree=false so, in that case, ClearForm would be called as expected. In addition, there is a test checking that.

> Also, when you're calling ClearForm, you're always passing aNotify=PR_TRUE.
> This isn't always correct if AfterSetAttr/BeforeSetAttr was called with
> aNotify=false. In fact, it might be a security problem to do so since it might
> cause crashes.

Yeah, actually, I've added aNotify then removed it because ClearForm aNotify argument seems a bit weird. I can add it back. I guess in BindToTree it should be "FALSE" (actually, we don't really care because ClearForm will not be called) but in FormIdUpdate... do we want to notify? I really don't know what should be done :)

> >@@ -2535,16 +2503,24 @@ nsGenericHTMLFormElement::UnbindFromTree
> >+  // We have to remove the form id observer if there was one.
> >+  // We will re-add one later if needed (during bind to tree).
> >+  nsAutoString formId;
> >+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::form, formId) &&
> >+      !formId.IsEmpty()) {
> >+    RemoveFormIdObserver();
> >+  }
> 
> Use nsContentUtils::HasNonEmptyAttr

Oh cool. Didn't know that!
Depends on: 589977
Attached patch Patch v2.1 (obsolete) — Splinter Review
This new patch includes all the requested changes +/- the IRC discussion.
Attachment #468069 - Attachment is obsolete: true
Attachment #468519 - Flags: superreview?(jst)
Attachment #468519 - Flags: review?(jonas)
Attachment #468069 - Flags: superreview?(jst)
blocking2.0: ? → beta5+
Comment on attachment 468519 [details] [diff] [review]
Patch v2.1

>+  // We have to remove the form id observer if there was one.
>+  // We will re-add one later if needed (during bind to tree).
>+  if (nsContentUtils::HasNonEmptyAttr(this, kNameSpaceID_None,
>+                                     nsGkAtoms::form)) {

Fix whitespace.

>-  nsHTMLFormElement* FindForm(nsHTMLFormElement* aCurrentForm = nsnull);
>+  nsHTMLFormElement* FindParentForm(nsHTMLFormElement* aCurrentForm = nsnull);

Name this FindAncestorForm. You're not just looking at the parent, but its parent and so on as well.

r=me with that.
Attachment #468519 - Flags: review?(jonas) → review-
(In reply to comment #16)
> r=me with that.

Are you sure? It looks like Mr. Sicking doesn't agree with you Jonas because he gives me a r-. You two should probably talk about that... :)
Comment on attachment 468519 [details] [diff] [review]
Patch v2.1

Sorry, it's become such a habit to r- this patch.
Attachment #468519 - Flags: review- → review+
Attached patch Patch v2.2Splinter Review
r=sicking

jst, if you can review that this afternoon, it would be great so i would be able to land with -moz-placeholder :)
Attachment #468519 - Attachment is obsolete: true
Attachment #468538 - Flags: superreview?(jst)
Attachment #468519 - Flags: superreview?(jst)
Attachment #468538 - Flags: superreview?(jst) → superreview+
That was quick! Thanks :)
Pushed:
http://hg.mozilla.org/mozilla-central/rev/802c5ae92647
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Depends on: 590387
Depends on: 595606
You need to log in before you can comment on or make changes to this bug.