Last Comment Bug 588683 - Implement form attribute
: Implement form attribute
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b5
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 589977 590387 595606
Blocks: html5forms
  Show dependency treegraph
 
Reported: 2010-08-18 20:59 PDT by Mounir Lamouri (:mounir)
Modified: 2010-09-12 02:22 PDT (History)
8 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5+


Attachments
Patch v1 (34.13 KB, patch)
2010-08-19 23:14 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (38.50 KB, patch)
2010-08-20 15:29 PDT, Mounir Lamouri (:mounir)
jonas: review-
Details | Diff | Splinter Review
Patch v2 (39.77 KB, patch)
2010-08-21 13:47 PDT, Mounir Lamouri (:mounir)
jonas: review-
Details | Diff | Splinter Review
Patch v2.1 (43.33 KB, patch)
2010-08-23 16:27 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Patch v2.2 (43.37 KB, patch)
2010-08-23 17:02 PDT, Mounir Lamouri (:mounir)
jst: superreview+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-08-18 20:59:15 PDT
HTML5 lets an element to be associated with an arbitrary form (not an ancestor) with @form (.form only returns the current form owner).
Comment 1 Olli Pettay [:smaug] 2010-08-19 02:19:03 PDT
Do we know what is the use case for this? And does anyone else implement this?
Comment 2 Olli Pettay [:smaug] 2010-08-19 02:19:39 PDT
...because to me this sounds somewhat bad API.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-19 15:13:28 PDT
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.
Comment 4 Mounir Lamouri (:mounir) 2010-08-19 23:12:44 PDT
Asking for blocking2.0: we want this HTML5 Forms feature for ff4.
Comment 5 Mounir Lamouri (:mounir) 2010-08-19 23:14:30 PDT
Created attachment 467687 [details] [diff] [review]
Patch v1

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.
Comment 6 Mounir Lamouri (:mounir) 2010-08-20 15:29:02 PDT
Created attachment 467926 [details] [diff] [review]
Patch v1.1

This is refactoring a bit the code, fixing issues reported by the try server and fixing the memory leak.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-20 19:32:47 PDT
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 8 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-20 19:35:22 PDT
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.
Comment 9 Mounir Lamouri (:mounir) 2010-08-21 13:46:24 PDT
(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.
Comment 10 Mounir Lamouri (:mounir) 2010-08-21 13:47:44 PDT
Created attachment 468069 [details] [diff] [review]
Patch v2
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-23 12:57:39 PDT
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.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-23 13:07:46 PDT
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.
Comment 13 Mounir Lamouri (:mounir) 2010-08-23 13:23:18 PDT
(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.
Comment 14 Mounir Lamouri (:mounir) 2010-08-23 14:18:58 PDT
(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!
Comment 15 Mounir Lamouri (:mounir) 2010-08-23 16:27:11 PDT
Created attachment 468519 [details] [diff] [review]
Patch v2.1

This new patch includes all the requested changes +/- the IRC discussion.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-23 16:50:14 PDT
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.
Comment 17 Mounir Lamouri (:mounir) 2010-08-23 16:53:45 PDT
(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 18 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-23 16:54:56 PDT
Comment on attachment 468519 [details] [diff] [review]
Patch v2.1

Sorry, it's become such a habit to r- this patch.
Comment 19 Mounir Lamouri (:mounir) 2010-08-23 17:02:44 PDT
Created attachment 468538 [details] [diff] [review]
Patch v2.2

r=sicking

jst, if you can review that this afternoon, it would be great so i would be able to land with -moz-placeholder :)
Comment 20 Mounir Lamouri (:mounir) 2010-08-23 17:06:22 PDT
That was quick! Thanks :)
Comment 21 Mounir Lamouri (:mounir) 2010-08-23 22:14:37 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/802c5ae92647

Note You need to log in before you can comment on or make changes to this bug.