Now XForms use 'id' attribute as ID attribute only. It makes hard to create XBL widgets that use xforms controls. For example, there is no way to work with xforms xpath index() function or xforms:toggle/xforms:case elements. That is nice feature to have. That is looks like common problem. Sometimes for example, I like to use xul:label in anonymous content that is referred on certain control by 'control' attribute. Can we have common way to make 'anonid' attribute as ID attribute in anonymous content scope?

Many xul/xforms/xhtml elements uses 'id' attribute for their work. For example, 'id' attribute is used by xul:label/xhtml:label to be linked with some element, xul:broadcaster, xul:command and so on. If I like use those elements in anonymous content then I can't use 'id' attribute for them. Idea to use 'anonid' attribute that is unique in context of XBL bound element is looks fine for me. But I haven't idea how it can be implemented. Any thoughts?

Comment on attachment 246884 [details] [diff] [review] patch2 >Index: nsXFormsControlStub.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v >retrieving revision 1.54 >diff -u -r1.54 nsXFormsControlStub.cpp >--- nsXFormsControlStub.cpp 8 Oct 2006 14:15:01 -0000 1.54 >+++ nsXFormsControlStub.cpp 28 Nov 2006 10:09:13 -0000 >@@ -377,21 +372,21 @@ > for (PRInt32 i = 0; i < indexesUsed.Count(); ++i) { > // Find the repeat element and add |this| as a listener > nsCOMPtr<nsIDOMElement> repElem; >- domDoc->GetElementById(*(indexesUsed[i]), getter_AddRefs(repElem)); >+ nsXFormsUtils::GetElementByContextId(mElement, *(indexesUsed[i]), >+ getter_AddRefs(repElem)); > nsCOMPtr<nsIXFormsRepeatElement> rep(do_QueryInterface(repElem)); > if (!rep) > continue; > > rv = rep->AddIndexUser(this); >- if (NS_FAILED(rv)) { >- return rv; >- } >+ NS_ENSURE_SUCCESS(rv, rv); >+ > rv = mIndexesUsed.AppendObject(rep); > NS_ENSURE_SUCCESS(rv, rv); > } > } > >- return rv; >+ return NS_OK; > } > If it is ok to return NS_OK here, then we don't need the test for NS_SUCCEEDED anymore, right? Please remove it since you are already changing this file. Thanks. >Index: nsXFormsUtils.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v >retrieving revision 1.57 >diff -u -r1.57 nsXFormsUtils.h >--- nsXFormsUtils.h 21 Sep 2006 16:49:13 -0000 1.57 >+++ nsXFormsUtils.h 29 Nov 2006 05:21:40 -0000 >@@ -551,19 +551,30 @@ > /** > * Retrieve an element by id, handling (cloned) elements inside repeats. > * >- * @param aDoc The document to get element from > * @param aId The id of the element > * @param aOnlyXForms Only search for XForms elements > * @param aCaller The caller (or rather the caller's DOM element), > ignored if nsnull > * @param aElement The element (or nsnull if not found) > */ >- static NS_HIDDEN_(nsresult) GetElementById(nsIDOMDocument *aDoc, >- const nsAString &aId, >+ static NS_HIDDEN_(nsresult) GetElementById(const nsAString &aId, > const PRBool aOnlyXForms, > nsIDOMElement *aCaller, > nsIDOMElement **aElement); >- >+ >+ /** >+ * Retrieve an element by ID attribute. If given node is anonymous node then >+ * element will be searched inside anonymous content of bound element by >+ * 'anonid' attribute that is used as ID attribute. If given node is explicit >+ * node then original nsIDocument::getElementById() is used. >+ * nit: I'd suggest the following wording: "Search for an element with the given ID value. If the given node lives in the DOM, then we'll use nsIDocument::getElementById(). Otherwise, if the given node is a node that lives as anonymous content, then we'll search for the element using @anonid instead of @id." >+ * @param aRefNode Given node that is used to search referenced element. >+ * @param aId The ID attribute of referenced element. nit: should aId comment be "The ID/anonid value to search for"? And shouldn't you mention aElement? Perhaps, "The element we found that has its ID/anonid value equal to aId"? with those, r=me. You might want to check with someone who is really familiar with XBL and mozilla if you haven't already, like bz, and see if he can think of any scenarios where this might not work well or might cause other problems. As a side note, we need to look at the places where we call document->GetElementById prior to this patch and make sure we shouldn't have been using nsXFormsUtils::GetElementById instead. Seems to me that we wouldn't handle items inside repeats very well for most of these cases.

Comment on attachment 246884 [details] [diff] [review] patch2 >Index: nsXFormsUtils.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v >retrieving revision 1.92 >diff -u -r1.92 nsXFormsUtils.cpp >--- nsXFormsUtils.cpp 25 Oct 2006 20:42:03 -0000 1.92 >+++ nsXFormsUtils.cpp 29 Nov 2006 05:20:42 -0000 >@@ -1894,6 +1890,49 @@ > return NS_OK; > } > >+/* static */ >+nsresult >+nsXFormsUtils::GetElementByContextId(nsIDOMElement *aRefNode, >+ const nsAString &aId, >+ nsIDOMElement **aElement) >+{ >+ NS_ENSURE_ARG(aRefNode); >+ NS_ENSURE_ARG_POINTER(aElement); >+ >+ *aElement = nsnull; >+ >+ nsCOMPtr<nsIDOMDocument> document; >+ aRefNode->GetOwnerDocument(getter_AddRefs(document)); >+ >+ // Even if given element is anonymous node then search element by ID attribute >+ // of document because anonymous node can inherit attribute from bound node >+ // that is refID of document. >+ >+ nsresult rv = document->GetElementById(aId, aElement); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (*aElement) >+ return NS_OK; >+ >+ nsCOMPtr<nsIDOMDocumentXBL> xblDoc(do_QueryInterface(document)); >+ if (!xblDoc) >+ return NS_OK; >+ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(aRefNode)); >+ if (!content) >+ return NS_OK; >+ >+ nsIContent *boundContent; >+ for (boundContent = content->GetBindingParent(); boundContent != nsnull && >+ boundContent != boundContent->GetBindingParent() && !*aElement; >+ boundContent = boundContent->GetBindingParent()) { >+ nsCOMPtr<nsIDOMElement> boundElm(do_QueryInterface(boundContent)); >+ xblDoc->GetAnonymousElementByAttribute(boundElm, NS_LITERAL_STRING("anonid"), >+ aId, aElement); >+ } >+ nit: Also please comment this for loop. For example, what does it mean when boundContent = boundContent->GetBindingParent()?

Comment on attachment 246891 [details] [diff] [review] patch for 1.8 >Index: transformiix/source/xpath/nsIXFormsUtilityService.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/transformiix/source/xpath/Attic/nsIXFormsUtilityService.h,v >retrieving revision 1.4.2.1 >diff -u -p -8 -r1.4.2.1 nsIXFormsUtilityService.h >--- transformiix/source/xpath/nsIXFormsUtilityService.h 20 Jun 2006 18:54:46 -0000 1.4.2.1 >+++ transformiix/source/xpath/nsIXFormsUtilityService.h 29 Nov 2006 04:13:36 -0000 >@@ -62,16 +62,17 @@ class nsIXFormsModelElement; /* forward > > /* Use this macro when declaring classes that implement this interface. */ > #define NS_DECL_NSIXFORMSUTILITYSERVICE \ > NS_IMETHOD GetModelFromNode(nsIDOMNode *node, nsIDOMNode **_retval); \ > NS_IMETHOD IsNodeAssocWithModel(nsIDOMNode *aNode, nsIDOMNode *aModel, PRBool *_retval); \ > NS_IMETHOD GetInstanceDocumentRoot(const nsAString & aID, nsIDOMNode *aModelNode, nsIDOMNode **_retval); \ > NS_IMETHOD ValidateString(const nsAString & aValue, const nsAString & aType, const nsAString & aNamespace, PRBool *_retval); \ > NS_IMETHOD GetRepeatIndex(nsIDOMNode *aRepeat, PRInt32 *aIndex); \ >+ NS_IMETHOD GetRepeatIndexById(nsIDOMNode *aResolverNode, const nsAString &aId, PRInt32 *aIndex); \ > NS_IMETHOD GetMonths(const nsAString & aValue, PRInt32 *aMonths); \ > NS_IMETHOD GetSeconds(const nsAString & aValue, double *aSeconds); \ > NS_IMETHOD GetSecondsFromDateTime(const nsAString & aValue, double *aSeconds); \ > NS_IMETHOD GetDaysFromDateTime(const nsAString & aValue, PRInt32 *aDays); > > /** > * Private interface implemented by the nsXFormsUtilityService in XForms extension. > * Defining it here to prevent XPath requiring XForms extension. >@@ -123,16 +124,23 @@ class NS_NO_VTABLE nsIXFormsUtilityServi > > /** > * Function to retrieve the index from the given repeat element. > */ > /* long getRepeatIndex (in nsIDOMNode aRepeat); */ > NS_IMETHOD GetRepeatIndex(nsIDOMNode *aRepeat, PRInt32 *aIndex) = 0; > > /** >+ * Function to retrieve the index from the given ID attribute of repeat >+ * element. >+ */ >+ /* long getRepeatIndexById (in nsIDOMNode aResolverNode, in AString aRepeat); */ >+ NS_IMETHOD GetRepeatIndexById(nsIDOMNode *aResolverNode, const nsAString &aId, PRInt32 *aIndex) = 0; >+ >+ /** nit: perhaps, "Function to retrieve the index from the repeat element with the given id" Also, please look to see if we really need GetRepeatIndex exported like this anymore. If the transformiix guys let us change this code, we might as well remove this call from the interface and just make it a utility function inside nsXFormsUtilityService.cpp. with that and the nits from patch2 fixed, r=me

(In reply to comment #6) > If it is ok to return NS_OK here, then we don't need the test for NS_SUCCEEDED > anymore, right? Please remove it since you are already changing this file. > Thanks. Agree. Fixed. > nit: I'd suggest the following wording: > > "Search for an element with the given ID value. If the given node lives in the > DOM, then we'll use nsIDocument::getElementById(). Otherwise, if the given > node is a node that lives as anonymous content, then we'll search for the > element using @anonid instead of @id." > > >+ * @param aRefNode Given node that is used to search referenced element. > >+ * @param aId The ID attribute of referenced element. > > nit: should aId comment be "The ID/anonid value to search for"? And shouldn't > you mention aElement? Perhaps, "The element we found that has its ID/anonid > value equal to aId"? Aaron, please check comments one more time for new patch. > with those, r=me. You might want to check with someone who is really familiar > with XBL and mozilla if you haven't already, like bz, and see if he can think > of any scenarios where this might not work well or might cause other problems. I'd like to post new bug to DOM component to provide new method for nsIDOMNode. This things is needed already for us and for bug 362365. > As a side note, we need to look at the places where we call > document->GetElementById prior to this patch and make sure we shouldn't have > been using nsXFormsUtils::GetElementById instead. Seems to me that we wouldn't > handle items inside repeats very well for most of these cases. > Do you have in view nsXFormsDispatchElement?

(In reply to comment #8) > nit: perhaps, "Function to retrieve the index from the repeat element with the > given id" > > Also, please look to see if we really need GetRepeatIndex exported like this > anymore. If the transformiix guys let us change this code, we might as well > remove this call from the interface and just make it a utility function inside > nsXFormsUtilityService.cpp. Fixed

diff with -up8N

Comment on attachment 247385 [details] [diff] [review] patch3 > /* static */ > nsresult >-nsXFormsUtils::GetElementById(nsIDOMDocument *aDoc, >- const nsAString &aId, >+nsXFormsUtils::GetElementById(const nsAString &aId, > const PRBool aOnlyXForms, > nsIDOMElement *aCaller, > nsIDOMElement **aElement) > { > NS_ENSURE_TRUE(!aId.IsEmpty(), NS_ERROR_INVALID_ARG); >- NS_ENSURE_ARG_POINTER(aDoc); > NS_ENSURE_ARG_POINTER(aElement); > *aElement = nsnull; > > nsCOMPtr<nsIDOMElement> element; >- aDoc->GetElementById(aId, getter_AddRefs(element)); >+ GetElementByContextId(aCaller, aId, getter_AddRefs(element)); Because this uses nsXFormsUtils::GetElementByContextId, do we actually need both those methods? >+ >+ /** >+ * Search for an element with the given ID value. If the given node lives in >+ * the DOM, then we'll use nsIDOMDocument::getElementById(). Otherwise, if >+ * the given node is a node that lives as anonymous content, then we'll search >+ * for the element using @anonid instead of @id. This is a bit wrong, since the method seems to always call doc->GetElementById So it is not quite clear to me when to call GetElementByContextId and when GetElementById.

(In reply to comment #12) > (From update of attachment 247385 [details] [diff] [review] [edit]) > > > /* static */ > > nsresult > >-nsXFormsUtils::GetElementById(nsIDOMDocument *aDoc, > >- const nsAString &aId, > >+nsXFormsUtils::GetElementById(const nsAString &aId, > > const PRBool aOnlyXForms, > > nsIDOMElement *aCaller, > > nsIDOMElement **aElement) > > { > > NS_ENSURE_TRUE(!aId.IsEmpty(), NS_ERROR_INVALID_ARG); > >- NS_ENSURE_ARG_POINTER(aDoc); > > NS_ENSURE_ARG_POINTER(aElement); > > *aElement = nsnull; > > > > nsCOMPtr<nsIDOMElement> element; > >- aDoc->GetElementById(aId, getter_AddRefs(element)); > >+ GetElementByContextId(aCaller, aId, getter_AddRefs(element)); > > Because this uses nsXFormsUtils::GetElementByContextId, > do we actually need both those methods? > So it is not quite clear to me when to call GetElementByContextId and when > GetElementById. > I guess we need both methods since these are different things. nsXFormsUtils::GetElementByContextId() searches elements by @id and @anonid attributes. nsXFormsUtils::GetElementById performes additional actions if element is inside repeat row. I don't know good repeat code, so probably Aaron knows better?

Comment on attachment 247396 [details] [diff] [review] patch4 At least I want to see better documentation when to use which GetElementBy*Id

(In reply to comment #9) > Created an attachment (id=247385) [edit] > patch3 > > (In reply to comment #6) > > > nit: I'd suggest the following wording: > > > > "Search for an element with the given ID value. If the given node lives in the > > DOM, then we'll use nsIDocument::getElementById(). Otherwise, if the given > > node is a node that lives as anonymous content, then we'll search for the > > element using @anonid instead of @id." > > > > >+ * @param aRefNode Given node that is used to search referenced element. > > >+ * @param aId The ID attribute of referenced element. > > > > nit: should aId comment be "The ID/anonid value to search for"? And shouldn't > > you mention aElement? Perhaps, "The element we found that has its ID/anonid > > value equal to aId"? > > Aaron, please check comments one more time for new patch. > I checked the comments in the new patch. Perhaps change the wording of this, "// Search element with given value of 'anonid' attribute through complete // bindings chain. We must sure that currently traversed element is not // binding parent for itsleft to avoid infinite loop." to: "// Search for the element with the given value in its 'anonid' attribute // throughout the complete bindings chain. We must ensure that the currently // traversed element is not the binding parent itself to avoid an infinite // loop. The binding parent of a binding parent is itself" > > As a side note, we need to look at the places where we call > > document->GetElementById prior to this patch and make sure we shouldn't have > > been using nsXFormsUtils::GetElementById instead. Seems to me that we wouldn't > > handle items inside repeats very well for most of these cases. > > > > Do you have in view nsXFormsDispatchElement? > No, I was just noticing that some of our .cpp files used doc->GetElementById instead of nsXFormsUtils::GetElementById. Like trigger, and send. I've double checked those and most of them are right...we shouldn't use nsXFormsUtils::GetElementById for elements that belong inside the model. The only one that I THINK might be wrong is nsXFormsSetIndex.cpp. But I don't know if it really is unless we test it to make sure it works. I might be wrong.

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 247385 [details] [diff] [review] [edit] [edit]) > > > > > /* static */ > > > nsresult > > >-nsXFormsUtils::GetElementById(nsIDOMDocument *aDoc, > > >- const nsAString &aId, > > >+nsXFormsUtils::GetElementById(const nsAString &aId, > > > const PRBool aOnlyXForms, > > > nsIDOMElement *aCaller, > > > nsIDOMElement **aElement) > > > { > > > NS_ENSURE_TRUE(!aId.IsEmpty(), NS_ERROR_INVALID_ARG); > > >- NS_ENSURE_ARG_POINTER(aDoc); > > > NS_ENSURE_ARG_POINTER(aElement); > > > *aElement = nsnull; > > > > > > nsCOMPtr<nsIDOMElement> element; > > >- aDoc->GetElementById(aId, getter_AddRefs(element)); > > >+ GetElementByContextId(aCaller, aId, getter_AddRefs(element)); > > > > Because this uses nsXFormsUtils::GetElementByContextId, > > do we actually need both those methods? > > > So it is not quite clear to me when to call GetElementByContextId and when > > GetElementById. > > > > I guess we need both methods since these are different things. > nsXFormsUtils::GetElementByContextId() searches elements by @id and @anonid > attributes. nsXFormsUtils::GetElementById performes additional actions if > element is inside repeat row. I don't know good repeat code, so probably Aaron > knows better? > nsXFormsUtils::GetElementById will, like Alex said, search through repeat rows looking for controls in addition to looking through the regular DOM. For example, xf:dispatch dispatches an event to an element with the given ID. If the element is in a repeat, you don't want to dispatch the event to the element in the DOM since we just end up hiding it and treating it as part of the repeat template. So we use nsXFormsUtils::GetElementById to dispatch the event to the contol with that id that is in the repeat row that has the current focus (well, the repeat row that corresponds to the repeat's index). If the element with that ID isn't in a repeat, then it picks the element with that ID from the DOM. But you wouldn't want to use this call for items that you know can't be inside repeats (like instance or submission elements). So for those we should use document->getElementById. But since Alex created his nsXFormsUtils::GetElementByContextId() function, that is just as handy since it calls document->getElementById first anyhow and does the work of querying the document to use, too.

(In reply to comment #15) > No, I was just noticing that some of our .cpp files used doc->GetElementById > instead of nsXFormsUtils::GetElementById. Like trigger, and send. I've double > checked those and most of them are right...we shouldn't use > nsXFormsUtils::GetElementById for elements that belong inside the model. The > only one that I THINK might be wrong is nsXFormsSetIndex.cpp. But I don't know > if it really is unless we test it to make sure it works. I might be wrong. > Looks working. Let me know if my testcase doesn't show exactly that what you meant.

(In reply to comment #15) > I checked the comments in the new patch. Perhaps change the wording of this, > "// Search element with given value of 'anonid' attribute through complete > // bindings chain. We must sure that currently traversed element is not > // binding parent for itsleft to avoid infinite loop." > > to: > > "// Search for the element with the given value in its 'anonid' attribute > // throughout the complete bindings chain. We must ensure that the currently > // traversed element is not the binding parent itself to avoid an infinite > // loop. The binding parent of a binding parent is itself" I use following wording: "// Search for the element with the given value in its 'anonid' attribute // throughout the complete bindings chain. We must ensure that the binding // parent of currently traversed element is not element itself to avoid an // infinite loop." Is it ok? (In reply to comment #12) > >+ /** > >+ * Search for an element with the given ID value. If the given node lives in > >+ * the DOM, then we'll use nsIDOMDocument::getElementById(). Otherwise, if > >+ * the given node is a node that lives as anonymous content, then we'll search > >+ * for the element using @anonid instead of @id. > > This is a bit wrong, since the method seems to always call doc->GetElementById Fixed. > So it is not quite clear to me when to call GetElementByContextId and when > GetElementById. > I used Aaron's example to comment getElementById() method.

Comment on attachment 248063 [details] [diff] [review] patch5 >Index: extensions/xforms/nsXFormsControlStub.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v >retrieving revision 1.54 >diff -u -p -8 -r1.54 nsXFormsControlStub.cpp >--- extensions/xforms/nsXFormsControlStub.cpp 8 Oct 2006 14:15:01 -0000 1.54 >+++ extensions/xforms/nsXFormsControlStub.cpp 9 Dec 2006 07:39:08 -0000 >@@ -358,45 +358,40 @@ nsXFormsControlStub::ProcessNodeBinding( > NS_ENSURE_SUCCESS(rv, rv); > NS_ENSURE_STATE(mModel); > > rv = MaybeAddToModel(oldModel, parentControl); > NS_ENSURE_SUCCESS(rv, rv); > > if (aModel) > NS_ADDREF(*aModel = mModel); >- mUsesModelBinding = usesModelBinding; > >- nsCOMPtr<nsIContent> content(do_QueryInterface(mElement)); >- NS_ENSURE_STATE(content); >- nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc(); >- nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(doc)); >- NS_ENSURE_STATE(domDoc); >+ mUsesModelBinding = usesModelBinding; > >- if (NS_SUCCEEDED(rv) && indexesUsed.Count()) { >+ if (indexesUsed.Count()) { > // add index listeners on repeat elements > > for (PRInt32 i = 0; i < indexesUsed.Count(); ++i) { > // Find the repeat element and add |this| as a listener > nsCOMPtr<nsIDOMElement> repElem; >- domDoc->GetElementById(*(indexesUsed[i]), getter_AddRefs(repElem)); >+ nsXFormsUtils::GetElementByContextId(mElement, *(indexesUsed[i]), >+ getter_AddRefs(repElem)); > nsCOMPtr<nsIXFormsRepeatElement> rep(do_QueryInterface(repElem)); > if (!rep) > continue; > > rv = rep->AddIndexUser(this); >- if (NS_FAILED(rv)) { >- return rv; >- } >+ NS_ENSURE_SUCCESS(rv, rv); >+ > rv = mIndexesUsed.AppendObject(rep); > NS_ENSURE_SUCCESS(rv, rv); > } > } > >- return rv; >+ return NS_OK; > } > > NS_IMETHODIMP > nsXFormsControlStub::BindToModel(PRBool aSetBoundNode) > { > nsCOMPtr<nsIModelElementPrivate> oldModel(mModel); > > nsCOMPtr<nsIXFormsControl> parentControl; >Index: extensions/xforms/nsXFormsDispatchElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsDispatchElement.cpp,v >retrieving revision 1.6 >diff -u -p -8 -r1.6 nsXFormsDispatchElement.cpp >--- extensions/xforms/nsXFormsDispatchElement.cpp 23 Aug 2005 11:00:50 -0000 1.6 >+++ extensions/xforms/nsXFormsDispatchElement.cpp 9 Dec 2006 07:39:08 -0000 >@@ -90,28 +90,27 @@ nsXFormsDispatchElement::HandleAction(ns > if (cancelableStr.IsEmpty() && bubbleStr.IsEmpty()) > nsXFormsUtils::GetEventDefaults(name, cancelable, bubbles); > else if (cancelableStr.IsEmpty()) > nsXFormsUtils::GetEventDefaults(name, cancelable, tmp); > else if (bubbleStr.IsEmpty()) > nsXFormsUtils::GetEventDefaults(name, tmp, bubbles); > } > >- nsCOMPtr<nsIDOMDocument> doc; >- mElement->GetOwnerDocument(getter_AddRefs(doc)); >- if (!doc) >- return NS_OK; >- > nsCOMPtr<nsIDOMElement> el; >- nsXFormsUtils::GetElementById(doc, target, PR_FALSE, mElement, >- getter_AddRefs(el)); >+ nsXFormsUtils::GetElementById(target, PR_FALSE, mElement, getter_AddRefs(el)); > if (!el) > return NS_OK; >- >+ >+ nsCOMPtr<nsIDOMDocument> doc; >+ mElement->GetOwnerDocument(getter_AddRefs(doc)); > nsCOMPtr<nsIDOMDocumentEvent> docEvent = do_QueryInterface(doc); >+ if (!docEvent) >+ return NS_OK; >+ > nsCOMPtr<nsIDOMEvent> event; > docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event)); > event->InitEvent(name, bubbles, cancelable); > > nsCOMPtr<nsIDOMEventTarget> targetEl = do_QueryInterface(el); > if (targetEl) { > nsXFormsUtils::SetEventTrusted(event, el); > PRBool defaultActionEnabled; >Index: extensions/xforms/nsXFormsSendElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSendElement.cpp,v >retrieving revision 1.3 >diff -u -p -8 -r1.3 nsXFormsSendElement.cpp >--- extensions/xforms/nsXFormsSendElement.cpp 8 Apr 2005 15:40:54 -0000 1.3 >+++ extensions/xforms/nsXFormsSendElement.cpp 9 Dec 2006 07:39:08 -0000 >@@ -59,23 +59,19 @@ nsXFormsSendElement::HandleAction(nsIDOM > return NS_OK; > > NS_NAMED_LITERAL_STRING(submission, "submission"); > nsAutoString submissionID; > mElement->GetAttribute(submission, submissionID); > if (submissionID.IsEmpty()) > return NS_OK; > >- nsCOMPtr<nsIDOMDocument> doc; >- mElement->GetOwnerDocument(getter_AddRefs(doc)); >- if (!doc) >- return NS_OK; >- > nsCOMPtr<nsIDOMElement> el; >- doc->GetElementById(submissionID, getter_AddRefs(el)); >+ nsXFormsUtils::GetElementByContextId(mElement, submissionID, >+ getter_AddRefs(el)); > > if (!el || !nsXFormsUtils::IsXFormsElement(el, submission)) { > const PRUnichar *strings[] = { submissionID.get(), submission.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"), > strings, 2, mElement, mElement); > return nsXFormsUtils::DispatchEvent(mElement, eEvent_BindingException); > } > >Index: extensions/xforms/nsXFormsSetFocusElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSetFocusElement.cpp,v >retrieving revision 1.2 >diff -u -p -8 -r1.2 nsXFormsSetFocusElement.cpp >--- extensions/xforms/nsXFormsSetFocusElement.cpp 23 Aug 2005 11:00:50 -0000 1.2 >+++ extensions/xforms/nsXFormsSetFocusElement.cpp 9 Dec 2006 07:39:08 -0000 >@@ -60,24 +60,18 @@ nsXFormsSetFocusElement::HandleAction(ns > if (!mElement) > return NS_OK; > > nsAutoString control; > mElement->GetAttribute(NS_LITERAL_STRING("control"), control); > if (control.IsEmpty()) > return NS_OK; > >- nsCOMPtr<nsIDOMDocument> doc; >- mElement->GetOwnerDocument(getter_AddRefs(doc)); >- if (!doc) >- return NS_OK; >- > nsCOMPtr<nsIDOMElement> el; >- nsXFormsUtils::GetElementById(doc, control, PR_TRUE, mElement, >- getter_AddRefs(el)); >+ nsXFormsUtils::GetElementById(control, PR_TRUE, mElement, getter_AddRefs(el)); > if (!el) > return NS_OK; > > return nsXFormsUtils::DispatchEvent(el, eEvent_Focus); > } > > NS_HIDDEN_(nsresult) > NS_NewXFormsSetFocusElement(nsIXTFElement **aResult) >Index: extensions/xforms/nsXFormsSetIndexElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSetIndexElement.cpp,v >retrieving revision 1.5 >diff -u -p -8 -r1.5 nsXFormsSetIndexElement.cpp >--- extensions/xforms/nsXFormsSetIndexElement.cpp 23 Feb 2006 11:25:07 -0000 1.5 >+++ extensions/xforms/nsXFormsSetIndexElement.cpp 9 Dec 2006 07:39:08 -0000 >@@ -106,28 +106,27 @@ nsXFormsSetIndexElement::HandleAction(ns > mElement); > return NS_OK; > } > double indexDoub; > rv = result->GetNumberValue(&indexDoub); > NS_ENSURE_SUCCESS(rv, rv); > PRUint32 indexInt = indexDoub < 1 ? 0 : (PRUint32) floor(indexDoub); > >- // Find the \<repeat\> with @id == |id| >- nsCOMPtr<nsIDOMDocument> domDoc; >- rv = mElement->GetOwnerDocument(getter_AddRefs(domDoc)); >- NS_ENSURE_SUCCESS(rv, rv); >- > #ifdef DEBUG_XF_SETINDEX > printf("<setindex>: Setting index '%s' to '%d'\n", > NS_ConvertUTF16toUTF8(id).get(), > indexInt); >-#endif >+#endif >+ >+ // Find the \<repeat\> with @id == |id| > nsCOMPtr<nsIDOMElement> repeatElem; >- rv = domDoc->GetElementById(id, getter_AddRefs(repeatElem)); >+ nsXFormsUtils::GetElementByContextId(mElement, id, >+ getter_AddRefs(repeatElem)); >+ > nsCOMPtr<nsIXFormsRepeatElement> repeat = do_QueryInterface(repeatElem); > if (!repeat) { > const PRUnichar *strings[] = { id.get(), repeatStr.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"), > strings, 2, mElement, mElement); > return NS_OK; > } > >Index: extensions/xforms/nsXFormsTriggerElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsTriggerElement.cpp,v >retrieving revision 1.18 >diff -u -p -8 -r1.18 nsXFormsTriggerElement.cpp >--- extensions/xforms/nsXFormsTriggerElement.cpp 2 Oct 2006 14:34:05 -0000 1.18 >+++ extensions/xforms/nsXFormsTriggerElement.cpp 9 Dec 2006 07:39:08 -0000 >@@ -108,24 +108,22 @@ nsXFormsSubmitElement::HandleDefault(nsI > aEvent->GetType(type); > if (!(*aHandled = type.EqualsLiteral("DOMActivate"))) > return NS_OK; > > NS_NAMED_LITERAL_STRING(submission, "submission"); > nsAutoString submissionID; > mElement->GetAttribute(submission, submissionID); > >- nsCOMPtr<nsIDOMDocument> ownerDoc; >- mElement->GetOwnerDocument(getter_AddRefs(ownerDoc)); >- NS_ENSURE_STATE(ownerDoc); >- > nsCOMPtr<nsIDOMElement> submissionElement; >- ownerDoc->GetElementById(submissionID, getter_AddRefs(submissionElement)); >+ nsXFormsUtils::GetElementByContextId(mElement, submissionID, >+ getter_AddRefs(submissionElement)); >+ > nsCOMPtr<nsIXFormsSubmissionElement> xfSubmission(do_QueryInterface(submissionElement)); >- >+ > if (!xfSubmission) { > const PRUnichar *strings[] = { submissionID.get(), submission.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"), > strings, 2, mElement, mElement); > return nsXFormsUtils::DispatchEvent(mElement, eEvent_BindingException); > } > > xfSubmission->SetActivator(this); >Index: extensions/xforms/nsXFormsUtils.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v >retrieving revision 1.92 >diff -u -p -8 -r1.92 nsXFormsUtils.cpp >--- extensions/xforms/nsXFormsUtils.cpp 25 Oct 2006 20:42:03 -0000 1.92 >+++ extensions/xforms/nsXFormsUtils.cpp 9 Dec 2006 09:10:13 -0000 >@@ -38,16 +38,17 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "nsXFormsUtils.h" > #include "nsString.h" > #include "nsXFormsAtoms.h" > #include "nsIDOMElement.h" > #include "nsIDOMNSHTMLElement.h" > #include "nsIDocument.h" >+#include "nsIDOMDocumentXBL.h" > #include "nsINameSpaceManager.h" > #include "nsIDOMNodeList.h" > #include "nsIDOMXPathEvaluator.h" > #include "nsIDOMXPathExpression.h" > #include "nsIDOMXPathResult.h" > #include "nsIDOMXPathNSResolver.h" > #include "nsIXPathEvaluatorInternal.h" > #include "nsIDOMDocument.h" >@@ -307,26 +308,23 @@ nsXFormsUtils::GetNodeContext(nsIDOMElem > > // Set default context size and position > if (aContextSize) > *aContextSize = 1; > if (aContextPosition) > *aContextPosition = 1; > > // Find correct model element >- nsCOMPtr<nsIDOMDocument> domDoc; >- aElement->GetOwnerDocument(getter_AddRefs(domDoc)); >- NS_ENSURE_TRUE(domDoc, NS_ERROR_FAILURE); >- > nsAutoString bindId; > NS_NAMED_LITERAL_STRING(bindStr, "bind"); > aElement->GetAttribute(bindStr, bindId); > if (!bindId.IsEmpty()) { > // CASE 1: Use @bind >- domDoc->GetElementById(bindId, aBindElement); >+ GetElementByContextId(aElement, bindId, aBindElement); >+ > if (!IsXFormsElement(*aBindElement, bindStr)) { > const PRUnichar *strings[] = { bindId.get(), bindStr.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"), > strings, 2, aElement, aElement); > DispatchEvent(aElement, eEvent_BindingException); > return NS_ERROR_ABORT; > } > >@@ -337,19 +335,19 @@ nsXFormsUtils::GetNodeContext(nsIDOMElem > // CASE 2: Use @model > // If bind did not set model, and the element has a model attribute we use this > nsAutoString modelId; > NS_NAMED_LITERAL_STRING(modelStr, "model"); > aElement->GetAttribute(modelStr, modelId); > > if (!modelId.IsEmpty()) { > nsCOMPtr<nsIDOMElement> modelElement; >- domDoc->GetElementById(modelId, getter_AddRefs(modelElement)); >+ GetElementByContextId(aElement, modelId, getter_AddRefs(modelElement)); > nsCOMPtr<nsIModelElementPrivate> model = do_QueryInterface(modelElement); >- >+ > // No element found, or element not a \<model\> element > if (!model) { > const PRUnichar *strings[] = { modelId.get(), modelStr.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("idRefError"), > strings, 2, aElement, aElement); > nsXFormsUtils::DispatchEvent(aElement, eEvent_BindingException); > return NS_ERROR_FAILURE; > } >@@ -1793,29 +1791,27 @@ FindRepeatContext(nsIDOMElement *aElemen > if (context && NS_SUCCEEDED(rv)) { > NS_ADDREF(result = context); > } > return result; > } > > /* static */ > nsresult >-nsXFormsUtils::GetElementById(nsIDOMDocument *aDoc, >- const nsAString &aId, >+nsXFormsUtils::GetElementById(const nsAString &aId, > const PRBool aOnlyXForms, > nsIDOMElement *aCaller, > nsIDOMElement **aElement) > { > NS_ENSURE_TRUE(!aId.IsEmpty(), NS_ERROR_INVALID_ARG); >- NS_ENSURE_ARG_POINTER(aDoc); > NS_ENSURE_ARG_POINTER(aElement); > *aElement = nsnull; > > nsCOMPtr<nsIDOMElement> element; >- aDoc->GetElementById(aId, getter_AddRefs(element)); >+ GetElementByContextId(aCaller, aId, getter_AddRefs(element)); > if (!element) > return NS_OK; > > // Check whether the element is inside a repeat. If it is, find the cloned > // element for the currently selected row. > nsCOMPtr<nsIDOMNode> repeat = FindRepeatContext(element, PR_FALSE); > if (!repeat) { > // Element not inside a repeat, just return the element (eventually >@@ -1889,16 +1885,63 @@ nsXFormsUtils::GetElementById(nsIDOMDocu > } > > NS_ENSURE_STATE(element); > NS_ADDREF(*aElement = element); > > return NS_OK; > } > >+/* static */ >+nsresult >+nsXFormsUtils::GetElementByContextId(nsIDOMElement *aRefNode, >+ const nsAString &aId, >+ nsIDOMElement **aElement) >+{ >+ NS_ENSURE_ARG(aRefNode); >+ NS_ENSURE_ARG_POINTER(aElement); >+ >+ *aElement = nsnull; >+ >+ nsCOMPtr<nsIDOMDocument> document; >+ aRefNode->GetOwnerDocument(getter_AddRefs(document)); >+ >+ // Even if given element is anonymous node then search element by ID attribute >+ // of document because anonymous node can inherit attribute from bound node >+ // that is refID of document. >+ >+ nsresult rv = document->GetElementById(aId, aElement); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (*aElement) >+ return NS_OK; >+ >+ nsCOMPtr<nsIDOMDocumentXBL> xblDoc(do_QueryInterface(document)); >+ if (!xblDoc) >+ return NS_OK; >+ >+ nsCOMPtr<nsIContent> content(do_QueryInterface(aRefNode)); >+ if (!content) >+ return NS_OK; >+ >+ // Search for the element with the given value in its 'anonid' attribute >+ // throughout the complete bindings chain. We must ensure that the binding >+ // parent of currently traversed element is not element itself to avoid an >+ // infinite loop. >+ nsIContent *boundContent; >+ for (boundContent = content->GetBindingParent(); boundContent != nsnull && >+ boundContent != boundContent->GetBindingParent() && !*aElement; >+ boundContent = boundContent->GetBindingParent()) { >+ nsCOMPtr<nsIDOMElement> boundElm(do_QueryInterface(boundContent)); >+ xblDoc->GetAnonymousElementByAttribute(boundElm, NS_LITERAL_STRING("anonid"), >+ aId, aElement); >+ } >+ >+ return NS_OK; >+} > > /* static */ > PRBool > nsXFormsUtils::HandleFatalError(nsIDOMElement *aElement, > const nsAString &aName) > { > if (!aElement) { > return PR_FALSE; >Index: extensions/xforms/nsXFormsUtils.h >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v >retrieving revision 1.57 >diff -u -p -8 -r1.57 nsXFormsUtils.h >--- extensions/xforms/nsXFormsUtils.h 21 Sep 2006 16:49:13 -0000 1.57 >+++ extensions/xforms/nsXFormsUtils.h 9 Dec 2006 09:29:49 -0000 >@@ -544,31 +544,60 @@ public: > * Returns whether the an elements document is ready to bind data (all > * instance documents loaded, etc.). > * > * @param aElement The element to check for > */ > static NS_HIDDEN_(PRBool) IsDocumentReadyForBind(nsIDOMElement *aElement); > > /** >- * Retrieve an element by id, handling (cloned) elements inside repeats. >+ * Search for an element by ID through repeat rows looking for controls in >+ * addition to looking through the regular DOM. >+ * >+ * For example, xf:dispatch dispatches an event to an element with the given >+ * ID. If the element is in a repeat, you don't want to dispatch the event to >+ * the element in the DOM since we just end up hiding it and treating it as >+ * part of the repeat template. So we use nsXFormsUtils::GetElementById to >+ * dispatch the event to the contol with that id that is in the repeat row >+ * that has the current focus (well, the repeat row that corresponds to the >+ * repeat's index). If the element with that ID isn't in a repeat, then it >+ * picks the element with that ID from the DOM. But you wouldn't want to use >+ * this call for items that you know can't be inside repeats (like instance or >+ * submission elements). So for those you should use >+ * nsXFormsUtils::GetElementByContextId. > * >- * @param aDoc The document to get element from > * @param aId The id of the element > * @param aOnlyXForms Only search for XForms elements > * @param aCaller The caller (or rather the caller's DOM element), > ignored if nsnull > * @param aElement The element (or nsnull if not found) > */ >- static NS_HIDDEN_(nsresult) GetElementById(nsIDOMDocument *aDoc, >- const nsAString &aId, >+ static NS_HIDDEN_(nsresult) GetElementById(const nsAString &aId, > const PRBool aOnlyXForms, > nsIDOMElement *aCaller, > nsIDOMElement **aElement); >- >+ >+ /** >+ * Search for an element with the given ID value. First >+ * nsIDOMDocument::getElementById() is used. If it successful then found >+ * element is returned. Second, if the given node is inside anonymous content >+ * then search is performed throughout the complete bindings chain by @anonid >+ * attribute. >+ * >+ * @param aRefNode The node relatively of which search is performed in >+ * anonymous content >+ * @param aId The @id/@anonid value to search for >+ * >+ * @return aElement The element we found that has its ID/anonid value >+ * equal to aId >+ */ >+ static NS_HIDDEN_(nsresult) GetElementByContextId(nsIDOMElement *aRefNode, >+ const nsAString &aId, >+ nsIDOMElement **aElement); >+ > /** > * Shows an error dialog for fatal errors. > * > * The dialog can be disabled via the |xforms.disablePopup| preference. > * > * @param aElement Element the exception occured at > * @param aName The name to use for the new window > * @return Whether handling was successful >Index: extensions/xforms/nsXFormsXPathFunctions.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsXPathFunctions.cpp,v >retrieving revision 1.1 >diff -u -p -8 -r1.1 nsXFormsXPathFunctions.cpp >--- extensions/xforms/nsXFormsXPathFunctions.cpp 13 Jul 2006 14:21:52 -0000 1.1 >+++ extensions/xforms/nsXFormsXPathFunctions.cpp 9 Dec 2006 07:39:08 -0000 >@@ -160,18 +160,18 @@ nsXFormsXPathFunctions::Index(txIFunctio > NS_ENSURE_TRUE(resolverNode, NS_ERROR_FAILURE); > > // here document is the XForms document > nsCOMPtr<nsIDOMDocument> document; > resolverNode->GetOwnerDocument(getter_AddRefs(document)); > NS_ENSURE_TRUE(document, NS_ERROR_FAILURE); > > // aID should be the id of a nsIXFormsRepeatElement >- nsCOMPtr<nsIDOMElement> repeatEle; >- nsresult rv = document->GetElementById(aID, getter_AddRefs(repeatEle)); >+ rv = nsXFormsUtils::GetElementByContextId(resolverNode, aID, >+ getter_AddRefs(repeatEle)); > NS_ENSURE_SUCCESS(rv, rv); > > // now get the index value from the xforms:repeat. > PRInt32 index; > rv = nsXFormsUtils::GetRepeatIndex(repeatEle, &index); > NS_ENSURE_SUCCESS(rv, rv); > > // repeat's index is 1-based. If it is 0, then that is still ok since

oops, sorry. I wasn't going to add the patch to the comment :) Anyway, the comments look good enough, r=me

'patch7' is checked in by smaug on trunk

What should I do to get 'patch4 for 1.8' on 1.8 branch?

(In reply to comment #27) > What should I do to get 'patch4 for 1.8' on 1.8 branch? > Is 'patch4 for 1.8' just for 1.8 or also for 1.8.0? If it is for both, you should probably rename it to 'patch for 1.8.x'. When I build the large single patch I use to update 1.8.0 and 1.8 to trunk-level code, I do it patch by patch out of the xf-to-branch bugs. So if I see a patch for 1.8, I'll take that instead of the patch that was used for trunk checkin.

Originally I plan it for 1.8 only. I didn't try it for 1.8.0. Do we still have plans to support fx 1.5?

(In reply to comment #29) > Originally I plan it for 1.8 only. I didn't try it for 1.8.0. Do we still have > plans to support fx 1.5? > We were going to do one more release for FF 1.5

should this bug be marked xf-to-branch and resolved fixed or is there more work to be done on it?

(In reply to comment #31) > should this bug be marked xf-to-branch and resolved fixed or is there more work > to be done on it? > I didn't try this on FF 1.5 since I haven't 1.8.0 tree. Can you test it?

I tried 1.8 patch on 1.8.0 tree. Worked fine, though I had to do a clean rebuild of xforms. I'm replacing that patch with this one to get rid of the windows line endings.

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 looking for an XPath signoff on this so that we can get it into the branches

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 Don't know enough XForms to review this, but it looks fine from an arch PoV. Also, the patch should be safe to land on branch because none of the code it touches can execute without the xforms extension installed.

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 requesting approval for the branches. This is a change, as noted by Jonas, that will only affect XForms users. This change will allow people who write custom controls in XBL to embed xforms in the anonymous content and run standard xforms xpath functions (which look for xforms control with the given 'id') on these anonymous controls since the xpath functions would now be able to look for elements with the given anonid, too.

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 Well, I can r=me the extensions/transformiix parts. If that's all you need.

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 I thought we weren't approving interface changes on the 1.8 branch, only additions. Or does that not apply to XForms, even when the code is in transformiix?

(In reply to comment #38) > (From update of attachment 256744 [details] [diff] [review]) > I thought we weren't approving interface changes on the 1.8 branch, only > additions. Or does that not apply to XForms, even when the code is in > transformiix? > I'm guessing that the rule wouldn't apply in this case. This is an internal only interface that is kept in a header file (no corresponding .idl or .xpt file) so it could only be used by .cpp which isn't supported outside of XForms.

Comment on attachment 256744 [details] [diff] [review] patch5 for 1.8 thanks for the explanation. approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

checked into 1.8 and 1.8.0 branches