338135 - precedence order of content of hint element is wrong

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.55  [en] (IBM EVV/3.8/EAK01AGF/LE)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060511 Minefield/3.0a1

Appears that inline content always is used even when external is present (src has precedence http://www.w3.org/TR/xforms/index-all.html#ui-commonelems-hint )

This fails W3C's test suite case 8.3.5.b


Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: message file for testcase

Comment 2

[Steve Speicher]

Attachment: test case

Comment 3

[Merle Sterling]

Attachment: Implement precedence for hint content

Read the content from the src attribute for ephemeral messages during OnDataAvailable. Update OnStartRequest and OnStopRequest as well.  Add a method to inline-alert in xforms.xml so we can check if the hint element contains a linking attribute that should be read during OnDataAvailable.

Comment 4

[Steve Speicher]

Appears to be a problem with help element as well, see W3C test suite case:

http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.3/8.3.4/8.3.4.b.xhtml

The inline help text is being shown.

Comment 5

[Merle Sterling]

*** Bug 300870 has been marked as a duplicate of this bug. ***

Comment 6

[Merle Sterling]

Attachment: Precedence for both Hint and Ephemeral messages

The initial patch implemented the proper precedence for Hint messages but bug338135 (Ephemeral messages) is very similar and requires further changes to the same code.  So, bug 338135 has been dup'ed to this bug and this patch resolves both.

Comment 7

[Merle Sterling]

(In reply to comment #6)
> Created an attachment (id=223189) [edit]
> Precedence for both Hint and Ephemeral messages
> The initial patch implemented the proper precedence for Hint messages but
> bug338135 (Ephemeral messages) is very similar and requires further changes to
> the same code.  So, bug 338135 has been dup'ed to this bug and this patch
> resolves both.

I meant bug 300870 in the above comments, but there doesn't seem to be a way to edit your comments to correct them!

Comment 8

[aaronr]

Comment on attachment 223189 [details] [diff] [review]
Precedence for both Hint and Ephemeral messages

>Index: nsXFormsMessageElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v
>retrieving revision 1.21
>diff -u -8 -p -r1.21 nsXFormsMessageElement.cpp
>--- nsXFormsMessageElement.cpp	23 May 2006 08:27:18 -0000	1.21
>+++ nsXFormsMessageElement.cpp	24 May 2006 16:06:44 -0000

>@@ -956,18 +1004,17 @@ nsXFormsMessageElement::TestExternalFile
>   
>   // See if it's an http channel.  We'll look at the http status code more
>   // closely and only request the "HEAD" to keep the response small.
>   // Especially since we are requesting this for every message in the document
>   // and in most cases we pass off the URL to another browser service to
>   // get and display the message so no good to try to look at the contents
>   // anyway.
> 
>-  // XXX ephemeral messages should probably get their full contents here once
>-  // they are set up to handle external resources.
>+  // Ephemeral messages will get their full contents during OnDataAvailable.
>   nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);

nit: I'd just remove the XXX comment.  I don't think that you need to point out in TextExternalFile where ephemeral's will get their contents.

>@@ -990,16 +1037,31 @@ nsXFormsMessageElement::TestExternalFile
>     return NS_ERROR_FAILURE;
>   }
> 
>   // channel should be running along smoothly, increment the count
>   AddRemoveExternalResource(PR_TRUE);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsMessageElement::GetValue(nsAString& aValue)
>+{
>+  // The order of precedence for determining the text of the hint is:
>+  //   single node binding, linking, inline text (8.3.5)
>+

If this code is meant to only be called by hint, then maybe make that comment here.  Or at least comment that mSrcAttrText is only valid for hint.  Maybe make the comment, 'The order of precedence for determining the text of the message is: single node binding, linking, inlinetext.  However, we only cache the external value (via mSrcAttrText) for hints.'  Actually the code now makes it look like inline alert will have problems if it doesn't use inline content.  I'll have to test that.  That might eventually have to use mSrcAttrText, too.

>@@ -1051,16 +1113,26 @@ nsXFormsMessageElement::OnStartRequest(n
>   // this channel from the count until we've set the mStopType to be the
>   // proper value.  Entering this function, mStopType will be eStopType_None,
>   // so if we need mStopType to be any other value (like in an error
>   // condition), please make sure it is set before AddRemoveExternalResource
>   // is called.
>   NS_ASSERTION(aRequest == mChannel, "unexpected request");
>   NS_ASSERTION(mChannel, "no channel");
> 
>+  if (!mElement)
>+    return NS_OK;
>+
>+  // If the type is Hint or Ephemeral we want to return ns_ok so that callbacks
>+  // continue and we can read the contents of the src attribute during
>+  // OnDataAvailable. For all others, we return ns_binding_aborted because we
>+  // don't care to actually read the data at this point.
>+  if (IsEphemeral())
>+    return NS_OK;
>+

I would add to the end of the comment, "The external content for these other elements will be retrieved by the services that actually display the popups".

>@@ -1108,25 +1180,47 @@ nsXFormsMessageElement::OnStartRequest(n
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest,
>                                         nsISupports *aContext,
>                                         nsIInputStream *aInputStream,
>                                         PRUint32 aOffset,
>                                         PRUint32 aCount)
> {
>-  NS_NOTREACHED("nsXFormsMessageElement::OnDataAvailable");
>-  return NS_BINDING_ABORTED;
>+  if (!mElement)
>+    return NS_OK;
>+
>+  if (!IsEphemeral())
>+    return NS_BINDING_ABORTED;
>+
>+  nsresult rv;
>+  PRUint32 size, bytesRead;
>+  char buffer[256];
>+
>+  while (aCount) {
>+    size = PR_MIN(aCount, sizeof(buffer));
>+    rv = aInputStream->Read(buffer, size, &bytesRead);
>+    if (NS_FAILED(rv))
>+      return rv;

nit: I'd change this to NS_ENSURE_SUCCESS(rv, rv)

>+    mSrcAttrText.Append(buffer, bytesRead);
>+    aCount -= bytesRead;
>+  }
>+
>+  return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
>                                       nsISupports *aContext,
>                                       nsresult aStatusCode)
> {
>+  nsCOMPtr<nsIXFormsUIWidget> widget = do_QueryInterface(mElement);
>+  if (widget)
>+    widget->Refresh();
>+
>   return NS_OK;
> }

if we get here due to a binding aborted from above or if we get some other kind of status error, we still want to refresh?  I'd think that you'd at least want to wrap the refresh with IsEphemeral

with those changes, r=me

Comment 9

[Merle Sterling]

Attachment: Final patch

Made the minor changes suggested by Aaron.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 223305 [details] [diff] [review]
Final patch

> NS_IMETHODIMP
>+nsXFormsMessageElement::OnCreated(nsIXTFBindableElementWrapper *aWrapper)
>+{
>+  nsresult rv = nsXFormsBindableStub::OnCreated(aWrapper);
Should be nsXFormsDelegateStub::OnCreated(aWrapper)

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  aWrapper->SetNotificationMask(kStandardNotificationMask |
>+                                nsIXTFElement::NOTIFY_WILL_CHANGE_DOCUMENT |
>+                                nsIXTFElement::NOTIFY_WILL_CHANGE_PARENT |
>+                                nsIXTFElement::NOTIFY_PARENT_CHANGED |
>+                                nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);

nsIXTFElement::NOTIFY_WILL_CHANGE_DOCUMENT and nsIXTFElement::NOTIFY_PARENT_CHANGED 
are already in kStandardNotificationMask.


>+
>+  nsCOMPtr<nsIDOMElement> node;
>+  aWrapper->GetElementNode(getter_AddRefs(node));
>+
>+  // It's ok to keep a pointer to mElement.  mElement will have an
>+  // owning reference to this object, so as long as we null out mElement in
>+  // OnDestroyed, it will always be valid.
>+
>+  mElement = node;
>+  NS_ASSERTION(mElement, "Wrapper is not an nsIDOMElement, we'll crash soon");

This is done already in nsXFormsControlStubBase, no need to re-assign mElement.


> 
> NS_IMETHODIMP
>+nsXFormsMessageElement::DoneAddingChildren()
>+{
>+  TestExternalFile();
>+
>+  return NS_OK;
>+}

Shouldn't you set mDoneAddingChildren to PR_TRUE here?


>+  nsAutoString level;
>+  mElement->GetAttribute(NS_LITERAL_STRING("level"), level);
>+  if (!level.IsEmpty())
>+    if (level.Equals(NS_LITERAL_STRING("ephemeral")))
>+      return PR_TRUE;
>+
>+  return PR_FALSE;

just
return level.Equals(NS_LITERAL_STRING("ephemeral"));
No need for those if()


>+
>+      <method name="hasLinkingAttr">
>+        <body>
>+          if (this.GetAttribute('src')) {
This doesn't work, right? 'G' should be lowercase - .getAttribute('src')
And anyway, why do you need this method? Isn't this.hasAttribute('src') enough
in <method name="refresh">.

Comment 11

[aaronr]

good eyes Olli.  I can't believe that I missed the mDoneAddingChildren one especially!  The ::OnCreated stuff (except the notification flags) came directly from nsXFormsLabelElement, so should Merle change it there, too?

Comment 12

[Merle Sterling]

> > 
> > NS_IMETHODIMP
> >+nsXFormsMessageElement::DoneAddingChildren()
> >+{
> >+  TestExternalFile();
> >+
> >+  return NS_OK;
> >+}
> Shouldn't you set mDoneAddingChildren to PR_TRUE here?
Yes and I can't believe I missed that too.
> >+
> >+      <method name="hasLinkingAttr">
> >+        <body>
> >+          if (this.GetAttribute('src')) {
> This doesn't work, right? 'G' should be lowercase - .getAttribute('src')
> And anyway, why do you need this method? Isn't this.hasAttribute('src') enough
> in <method name="refresh">.
Surprisingly it did work, but you're right, it should be lowercase.  Simply using hasAttribute would have been way too simple. :)

Comment 13

[Merle Sterling]

Attachment: Fixes per Olli's review

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> good eyes Olli.  I can't believe that I missed the mDoneAddingChildren one
> especially!  The ::OnCreated stuff (except the notification flags) came
> directly from nsXFormsLabelElement, so should Merle change it there, too?
> 

nsXFormsLabelElement::OnCreated looks ok to me.

Comment 15

[aaronr]

(In reply to comment #14)
> (In reply to comment #11)
> > good eyes Olli.  I can't believe that I missed the mDoneAddingChildren one
> > especially!  The ::OnCreated stuff (except the notification flags) came
> > directly from nsXFormsLabelElement, so should Merle change it there, too?
> > 
> 
> nsXFormsLabelElement::OnCreated looks ok to me.
> 

doh!  My bad.  Man, vacation can't come soon enough!

Comment 16

[aaronr]

sorry, forgot to say why I r-'d it.  Doron was about to check in the patch and noticed that it didn't fix the attached testcase.  I swear that I saw it working, but he is right...doesn't work on the current trunk.  Maybe another patch broke it?

Comment 17

[Merle Sterling]

I checked out a new tree at 3pm, applied the patch, and did a clean build.  All of the testcases for both 338135 and 300870 work fine. The only thing I can think of is that I always change the src attribute to point to a local file. The original test case points to a file on bugzilla.mozilla.org and that results in a security error in the javascript console and an xforms-link-error popup. I've never been able to figure out the magic entries to put in hostperm.1 to make cross domain loading work, but I don't think the link error is the fault of any of the code from the patch. If you get the security permissions correct and the file does indeed exist, everything should be ok.

Comment 18

[Doron Rosenberg (IBM)]

Ok, something is really weird here.  http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.3/8.3.5/8.3.5.b.xhtml doesn't work as well, unless I first visit hint.txt (http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.3/8.3.5/hint.txt), after which it works.

Bizaare.

Comment 19

[Doron Rosenberg (IBM)]

The reason is, testexternal file is using the "HEAD" request method - which only reads in the head of the file, not the entire file, since it was designed to test if the file exists.

However, if you have the file cached in your browser, the entire file will be loaded.  Clearing the cache will make the problem evident.

Changing "HEAD" to "GET" fixes the problem for me.

Comment 20

[aaronr]

Alex is asking for GetValue to return the inline content if there is no bound node and if there is no @src.

I think that you can get around the problem Doron noticed if you change your logic so that we don't bother with the http channel checking if it is ephemeral (during TestExternalFile).

I also think that even if you are ephemeral, you are going to want to do the status checks during ::OnStartRequest.  The way that your patch looks to me, right now, I believe that you won't properly generate xforms-link-error events if the link address doesn't exist for a hint.  Should probably move the IsEphemeral test in ::OnStartRequest to right before AddRemoveExternalResources and add a test for mStopType so that if it isn't eStopType_None that you still call AddRemoveExternalResources, null out mChannel and return NS_BINDING_ABORTED.

Comment 21

[Merle Sterling]

Attachment: Latest greatest patch

- Don't use HEAD if the message is ephemeral - allows http urls for ephemeral messages to work. 
- Update GetValue to return inline contents if there is no bound node and no src attribute. 
- Refactor OnStartRequest so that we generate link errors for ephemeral messages if the url does not exist.

Comment 22

[alexander :surkov (:asurkov)]

*** Bug 340468 has been marked as a duplicate of this bug. ***

Comment 23

[alexander :surkov (:asurkov)]

*** Bug 338776 has been marked as a duplicate of this bug. ***

Comment 24

[aaronr]

Comment on attachment 225324 [details] [diff] [review]
Latest greatest patch

>Index: nsXFormsMessageElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v
>retrieving revision 1.22
>diff -u -8 -p -r1.22 nsXFormsMessageElement.cpp
>--- nsXFormsMessageElement.cpp	31 May 2006 09:00:24 -0000	1.22
>+++ nsXFormsMessageElement.cpp	12 Jun 2006 21:38:50 -0000

>@@ -193,42 +199,60 @@ private:
> 
>   /**
>    * Either add or subtract from the number of messages with external resources
>    * currently loading in this document.  aAdd == PR_TRUE will add to the total
>    * otherwise we will subtract from the total (as long as it isn't already 0).
>    */
>   void AddRemoveExternalResource(PRBool aAdd);
> 
>+  /** Determine if the message is Ephemeral.
>+   */
>+  PRBool IsEphemeral();
>+

nit: drop the comment a line so that it is on a line with a '*' and not '**'.  Just for consistencies sake.


>@@ -374,47 +398,59 @@ nsXFormsMessageElement::ParentChanged(ns
>   }
> 
>   return nsXFormsDelegateStub::ParentChanged(aNewParent);
> }
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
> {
>-  if (aName == nsXFormsAtoms::src) {
>-    // If we are currently trying to load an external message, cancel the
>-    // request.
>-    if (mChannel) {
>-      mChannel->Cancel(NS_BINDING_ABORTED);
>-    }
>+  if (mDoneAddingChildren) {
>+    if (aName == nsXFormsAtoms::src) {
>+      // If we are currently trying to load an external message, cancel the
>+      // request.
>+      if (mChannel) {
>+        mChannel->Cancel(NS_BINDING_ABORTED);
>+      }
> 
>-    mStopType = eStopType_None;
>-    TestExternalFile();
>+      mStopType = eStopType_None;
>+    }
>   }

don't you still need the call to TestExternalFile here inside the if mDoneAddingChildren section?


>@@ -990,16 +1025,42 @@ nsXFormsMessageElement::TestExternalFile
>     return NS_ERROR_FAILURE;
>   }
> 
>   // channel should be running along smoothly, increment the count
>   AddRemoveExternalResource(PR_TRUE);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsMessageElement::GetValue(nsAString& aValue)
>+{
>+  // The order of precedence for determining the text of the message
>+  // is: single node binding, linking, inline text (8.3.5).  We cache
>+  // the value of the external message (via mSrcAttrText) for hints
>+  // and ephemeral messages.
>+  
>+  nsXFormsDelegateStub::GetValue(aValue);
>+  if (aValue.IsVoid()) {
>+     if (!mSrcAttrText.IsEmpty()) {
>+       // handle linking ('src') attribute
>+       aValue = NS_ConvertUTF8toUTF16(mSrcAttrText);
>+     }

If you are testing mSrcAttrText to determine whether this element contains @src, then you should probably make sure that mSrceAttrText is empty if @src is removed.

>+     else {
>+       // Return inline value
>+       nsCOMPtr<nsIDOM3Node> node = do_QueryInterface(mElement);
>+       if (node) {
>+         node->GetTextContent(aValue);
>+       }
>+     }
>+  }

nit: looks like your indent here in the first 'if' loop is 3, not 2 spaces.

>+
>+  return NS_OK;
>+}
>+
> // nsIInterfaceRequestor
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::GetInterface(const nsIID &aIID, void **aResult)
> {
>   *aResult = nsnull;
>   return QueryInterface(aIID, aResult);
> }
>@@ -1045,16 +1106,19 @@ nsXFormsMessageElement::OnStartRequest(n
>   // this channel from the count until we've set the mStopType to be the
>   // proper value.  Entering this function, mStopType will be eStopType_None,
>   // so if we need mStopType to be any other value (like in an error
>   // condition), please make sure it is set before AddRemoveExternalResource
>   // is called.
>   NS_ASSERTION(aRequest == mChannel, "unexpected request");
>   NS_ASSERTION(mChannel, "no channel");
> 
>+  if (!mElement)
>+    return NS_OK;
>+

are you sure that you want to just return NS_OK here?  Shouldn't mChannel be nulled, AddRemoveExternalResource called, the request aborted?

>@@ -1089,38 +1153,70 @@ nsXFormsMessageElement::OnStartRequest(n
>       mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>       const PRUnichar *strings[] = { tagName.get(), src.get() };
>       nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
>                                  strings, 2, mElement, mElement);
>       mStopType = eStopType_LinkError;
>     }
>   }
> 
>+  // If the type is Hint or Ephemeral we want to return ns_ok so that callbacks
>+  // continue and we can read the contents of the src attribute during
>+  // OnDataAvailable. For all others, we return ns_binding_aborted because we
>+  // don't care to actually read the data at this point. The external content
>+  // for these other elements will be retrieved by the services that actually
>+  // display the popups.
>+  if (IsEphemeral() && mStopType == eStopType_None)
>+    return NS_OK;
>+
>   AddRemoveExternalResource(PR_FALSE);
>   mChannel = nsnull;
> 
>   return NS_BINDING_ABORTED;
> }
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest,
>                                         nsISupports *aContext,
>                                         nsIInputStream *aInputStream,
>                                         PRUint32 aOffset,
>                                         PRUint32 aCount)
> {
>-  NS_NOTREACHED("nsXFormsMessageElement::OnDataAvailable");
>-  return NS_BINDING_ABORTED;
>+  if (!mElement)
>+    return NS_OK;
>+

same here.

>+  if (!IsEphemeral())
>+    return NS_BINDING_ABORTED;
>+
>+  nsresult rv;
>+  PRUint32 size, bytesRead;
>+  char buffer[256];
>+
>+  while (aCount) {
>+    size = PR_MIN(aCount, sizeof(buffer));
>+    rv = aInputStream->Read(buffer, size, &bytesRead);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mSrcAttrText.Append(buffer, bytesRead);
>+    aCount -= bytesRead;
>+  }
>+
>+  return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
>                                       nsISupports *aContext,
>                                       nsresult aStatusCode)
> {
>+  if (IsEphemeral()) {
>+    nsCOMPtr<nsIXFormsUIWidget> widget = do_QueryInterface(mElement);
>+    if (widget)
>+      widget->Refresh();
>+  }
>+
>   return NS_OK;


if it is ephemeral, don't you need to call AddRemoveExternalResource and null out mChannel here?

> }
> 
> void
> nsXFormsMessageElement::AddRemoveExternalResource(PRBool aAdd)
> {
>   // if this message doesn't have a channel established already or it has
>   // already returned, then no sense bumping the counter.
>@@ -1165,16 +1261,26 @@ nsXFormsMessageElement::AddRemoveExterna
>       if (!isPending) {
>         mChannel = nsnull;
>       }
>       modelPriv->MessageLoadFinished();
>     }
>   }
> }
> 
>+PRBool nsXFormsMessageElement::IsEphemeral()
>+{
>+  if (mType == eType_Hint)
>+    return PR_TRUE;
>+
>+  nsAutoString level;
>+  mElement->GetAttribute(NS_LITERAL_STRING("level"), level);
>+  return level.Equals(NS_LITERAL_STRING("ephemeral"));
>+}
>+
> NS_HIDDEN_(nsresult)
> NS_NewXFormsMessageElement(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsMessageElement(nsXFormsMessageElement::eType_Normal);
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult);
>Index: resources/content/xforms.css
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.css,v
>retrieving revision 1.36
>diff -u -8 -p -r1.36 xforms.css
>--- resources/content/xforms.css	6 Jun 2006 13:52:03 -0000	1.36
>+++ resources/content/xforms.css	12 Jun 2006 21:38:50 -0000
>@@ -100,17 +100,17 @@ select itemset, xul|*:root select1 items
> html|*:root select1[appearance='compact'] itemset,
> html|*:root select1[appearance='full'] itemset,
> select choices, xul|*:root select1 choices,
> html|*:root select1[appearance='compact'] choices,
> html|*:root select1[appearance='full'] choices {
>   display: none;
> }
> 
>-message, alert {
>+message, alert, help {
>   display: none;
> }
> 
> action, message[level="ephemeral"], hint {
>   position: absolute;
>   z-index: 2147481647;
>   visibility: hidden;
>   top: 0px;
>Index: resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.37
>diff -u -8 -p -r1.37 xforms.xml
>--- resources/content/xforms.xml	6 Jun 2006 13:52:03 -0000	1.37
>+++ resources/content/xforms.xml	12 Jun 2006 21:38:51 -0000
>@@ -370,17 +370,18 @@
>               getAnonymousElementByAttribute(this, "anonid", "bindingData");
>           }
>           return this._bindingData;
>         </getter>
>       </property>
> 
>       <method name="refresh">
>         <body>
>-          if (this.accessors.hasBoundNode()) {
>+          if (this.accessors.hasBoundNode() ||
>+              this.hasAttribute('src')) {
>             this.bindingData.textContent = this.stringValue;
>             this.inlineData.style.display = "none";
>             this.bindingData.style.display = "inherit";
>           } else {
>             this.bindingData.style.display = "none";
>             this.inlineData.style.display = "inherit";
>           }
>         </body>

Comment 25

[Merle Sterling]

(In reply to comment #24)
> > NS_IMETHODIMP
> > nsXFormsMessageElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
> > {
> >-  if (aName == nsXFormsAtoms::src) {
> >-    // If we are currently trying to load an external message, cancel the
> >-    // request.
> >-    if (mChannel) {
> >-      mChannel->Cancel(NS_BINDING_ABORTED);
> >-    }
> >+  if (mDoneAddingChildren) {
> >+    if (aName == nsXFormsAtoms::src) {
> >+      // If we are currently trying to load an external message, cancel the
> >+      // request.
> >+      if (mChannel) {
> >+        mChannel->Cancel(NS_BINDING_ABORTED);
> >+      }
> > 
> >-    mStopType = eStopType_None;
> >-    TestExternalFile();
> >+      mStopType = eStopType_None;
> >+    }
> >   }
> don't you still need the call to TestExternalFile here inside the if
> mDoneAddingChildren section?

No, TestExternalFile is called after all of the attributes have been added because the order in which the attributes are added is undefined.
> >+NS_IMETHODIMP
> >+nsXFormsMessageElement::GetValue(nsAString& aValue)
> >+{
> >+  // The order of precedence for determining the text of the message
> >+  // is: single node binding, linking, inline text (8.3.5).  We cache
> >+  // the value of the external message (via mSrcAttrText) for hints
> >+  // and ephemeral messages.
> >+  
> >+  nsXFormsDelegateStub::GetValue(aValue);
> >+  if (aValue.IsVoid()) {
> >+     if (!mSrcAttrText.IsEmpty()) {
> >+       // handle linking ('src') attribute
> >+       aValue = NS_ConvertUTF8toUTF16(mSrcAttrText);
> >+     }
> If you are testing mSrcAttrText to determine whether this element contains
> @src, then you should probably make sure that mSrceAttrText is empty if @src is
> removed.
Ok, Truncated mSrcAttrText in AttributeRemoved.

Added AddRemoveExternalResource(PR_FALSE), set mChannel to NULL, and return ns_binding_aborted in the network callbacks if mElement is null. Note that this differs from the way the label element handles it, but it sounds right.

Comment 26

[Merle Sterling]

Attachment: Patch

Comment 27

[Merle Sterling]

Attachment: Patch

Call TestExternalFile from AttributeSet if we are done adding children so that we retest the external file if the src attribute has changed.

Comment 28

[aaronr]

Comment on attachment 226226 [details] [diff] [review]
Patch

looks good.  r=me

Comment 29

[aaronr]

checked into trunk for msterlin

Comment 30

[aaronr]

checked into 1.8.0 branch on 2006/09/21

Comment 31

[aaronr]

checked into 1.8 branch on 2006/11/21