Closed
Bug 588683
Opened 14 years ago
Closed 14 years ago
Implement form attribute
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
43.37 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Do we know what is the use case for this? And does anyone else implement this?
Comment 2•14 years ago
|
||
...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.
Assignee | ||
Comment 4•14 years ago
|
||
Asking for blocking2.0: we want this HTML5 Forms feature for ff4.
blocking2.0: --- → ?
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 6•14 years ago
|
||
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-
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
(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!
Assignee | ||
Comment 15•14 years ago
|
||
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)
Updated•14 years ago
|
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-
Assignee | ||
Comment 17•14 years ago
|
||
(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+
Assignee | ||
Comment 19•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #468538 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 20•14 years ago
|
||
That was quick! Thanks :)
Assignee | ||
Comment 21•14 years ago
|
||
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
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
•