291501 - labels don't support the src attribute

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

according to 8.3.3 in the spec, labels need to support the "Linking" attributes
and throw the xforms-link-error event if link traversal fails.  This failure
should also probably be written out to the Console.

Comment 1

[aaronr]

testcase in the URL: field - http://www.digitalfires.com/aaron/labellink.xhtml

Comment 2

[jhp (no longer active)]

Attachment: patch

First attempt at an XForms patch.  This is geared towards the attached
testcase, where 'src' points to a text file containing the label text.	For
that reason, I added a check in |OnStartRequest()| to prevent anything other
than "text/plain" from being read (images, svg, etc).  Not sure if I did the
networking part correctly.  Also, the |ReportError()| code caused an infinite
loop when passing |mElement|, so I have that commented out for now.

Comment 3

[Doron Rosenberg (IBM)]

For some reason, XFormsLabel has no parent:

  nsCOMPtr<nsIDOMNode> parent;
  mElement->GetParentNode(getter_AddRefs(parent));

!parent is true before reporterror is called.  Which is confusing.

Comment 4

[jhp (no longer active)]

Attachment: patch v1.1

Fixes the |ReportError()| calls to be more descriptive.

Comment 5

[aaronr]

Comment on attachment 185589 [details] [diff] [review]
patch v1.1


>+void
>+nsXFormsLabelElement::LoadExternalLabel(const nsAString& aSrc)
>+{
>+  nsresult rv = NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+  if (doc) {
>+    nsCOMPtr<nsIURI> uri;
>+    NS_NewURI(getter_AddRefs(uri), aSrc, doc->GetDocumentCharacterSet().get(),
>+              doc->GetDocumentURI());
>+    if (uri) {
>+      if (nsXFormsUtils::CheckSameOrigin(doc->GetDocumentURI(), uri)) {
>+        nsCOMPtr<nsILoadGroup> loadGroup;
>+        loadGroup = doc->GetDocumentLoadGroup();
>+        NS_WARN_IF_FALSE(loadGroup, "No load group!");
>+
>+        // Using the same load group as the main document and creating
>+        // the channel with LOAD_NORMAL flag delays the dispatching of
>+        // the 'load' event until label data document has been loaded.
>+        nsCOMPtr<nsIChannel> docChannel;
>+        NS_NewChannel(getter_AddRefs(docChannel), uri, nsnull, loadGroup,
>+                      this, nsIRequest::LOAD_NORMAL);
>+
>+        if (docChannel) {
>+          rv = docChannel->AsyncOpen(this, nsnull);
>+          if (NS_FAILED(rv)) {
>+            // URI doesn't exist; report error.
>+            // XXX Passing |mElement| as |aContext| param to ReportError leads
>+            //     to an infinite loop.  Avoid for now.
>+            const PRUnichar *strings[] = { PromiseFlatString(aSrc).get() };
>+            nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink1Error"),
>+                                       strings, 1, mElement, nsnull);
>+            nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkError);

xforms-link-error events are targeted at models, not labels.  GetModel() should
do the trick for you.



>+NS_IMETHODIMP
>+nsXFormsLabelElement::OnDataAvailable(nsIRequest *aRequest,
>+                                      nsISupports *aContext,
>+                                      nsIInputStream *aInputStream,
>+                                      PRUint32 aOffset,
>+                                      PRUint32 aCount)
>+{
>+  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;
>+    mSrcAttrText.Append(buffer, bytesRead);
>+    aCount -= bytesRead;
>+  }

If Read() fails after being partway done, before you return the error,
shouldn't you do mSrcAttrText.Truncate() to get rid of the half-finished data
that could be in there?


>+NS_IMETHODIMP
>+nsXFormsLabelElement::OnStopRequest(nsIRequest *aRequest,
>+                                    nsISupports *aContext,
>+                                    nsresult aStatusCode)
>+{
>+  if (NS_FAILED(aStatusCode)) {
>+    // XXX Passing |mElement| as |aContext| param to ReportError leads
>+    //     to an infinite loop.  Avoid for now.
>+    nsAutoString src;
>+    mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>+    const PRUnichar *strings[] = { src.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink2Error"),
>+                               strings, 1, mElement, nsnull);
>+    nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkError);
>+    mSrcAttrText.Truncate();
>+  }
>+
>+  RefreshLabel();
>+
>+  return NS_OK;
>+}

Here again, xforms-link-error events are targeted at models, not labels.


>+labelLink1Error      = XForms Error (17): Could not find Label element link target: %S
>+labelLink2Error      = XForms Error (18): Failed loading Label element link target: %S

Maybe make these error messages a little easier to read for someone unfamiliar
with the code?	'Label element link target', I'm guessing, wouldn't be easily
interpreted by a form author.  Maybe something like, "... Failed loading Label
element from external file..." or something like that.

Comment 6

[jhp (no longer active)]

> If Read() fails after being partway done, before you return the error,
> shouldn't you do mSrcAttrText.Truncate() to get rid of the half-finished data
> that could be in there?

No.  If |OnDataAvailable()| returns an error, then we receive the error
|OnStopRequest()|, where we truncate the text.

Comment 7

[jhp (no longer active)]

Attachment: patch v1.2

Updated to include Aaron's comments.

Comment 8

[aaronr]

Comment on attachment 185619 [details] [diff] [review]
patch v1.2

looks good.  r=me

please create a follow on bug so that we don't forget to handle different
types.	Maybe the correct thing would be to do this in XBL (when our XBL patch
is in) and then insert different anonymous content based on the mime type of
the external document.

Comment 9

[Doron Rosenberg (IBM)]

Comment on attachment 185619 [details] [diff] [review]
patch v1.2

smaug, can you do 2nd r?  We don't want only-IBM reviewed checkins :)

Comment 10

[jhp (no longer active)]

(In reply to comment #8)
Created bug 297083 for handling different types.

Comment 11

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

Comment on attachment 185619 [details] [diff] [review]
patch v1.2

>+          }
>+        }
>+      } else {
>+        nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceLoadOrigin"),
>+                                   domDoc);
This is wrong error message.

What happens if someone is changing the src attribute in a loop, i.e.
so fast that the previous requests hasn't been finalized?

The patch didn't apply cleanly to current trunk.

Comment 12

[jhp (no longer active)]

Attachment: patch v1.3

Patch to address smaug's comments.  I save the |nsIChannel| from any load
request.  Then if |AttributeSet()| or |AttributeRemove()| are called and there
is a request in progress, then we just cancel it.  I'll see if I can create a
testcase for that tomorrow.

Comment 13

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

Comment on attachment 185826 [details] [diff] [review]
patch v1.3


>+            const PRUnichar *strings[] = { PromiseFlatString(aSrc).get() };
>+            nsXFormsUtils::ReportError(NS_LITERAL_STRING("labelLink1Error"),
>+                                       strings, 1, mElement, nsnull);
See
http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTPromiseFlatStrin
g.h#66

Other than that, looks good.
Are you going to create a testcase?

Comment 14

[jhp (no longer active)]

Attachment: patch v1.4

Fix use of |PromiseFlatString|.

I couldn't create a conventional test case to test the |nsIStreamListener|
functions.  I tried adding an 'onload' Javascript that would change or remove
the 'src' attribute, but they would always get run in order; no conflicts.  So
instead, I just added a call to |AttributeRemoved()| in |AttributeSet()|, and I
saw the channel canceled correctly.

Comment 15

[jhp (no longer active)]

Checked into trunk. -> FIXED

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

+    NS_NewURI(getter_AddRefs(uri), aSrc, doc->GetDocumentCharacterSet().get(),
+              doc->GetDocumentURI());

shouldn't that use doc->GetBaseURI(), or even a node-specific base URI
(nsIContent::GetBaseURI)?

Comment 17

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

(In reply to comment #16)
> +    NS_NewURI(getter_AddRefs(uri), aSrc, doc->GetDocumentCharacterSet().get(),
> +              doc->GetDocumentURI());
> 
> shouldn't that use doc->GetBaseURI(), or even a node-specific base URI
> (nsIContent::GetBaseURI)?

Actually yes. Have to fix that in Bug 297765.