419190 - instance() function not behaving as expected in official Test Suite

Status: RESOLVED WONTFIX

(Core Graveyard :: XForms, defect)

Description

[Koen Heene]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

Hello,

The Mozilla XForms status page
(http://www.mozilla.org/projects/xforms/status-detail.html) states
that the function instance() is supported.

If had difficulties getting that to work. I went to the official
XForms test site and saw that the output is not as predicted by the test suite and also not as you would expect after having read the specs.

I currently work with FireFox 2.0.0.12 and the XForms extension 0.8.4.

Thank you for looking into this.

--
Koen

Reproducible: Always

Steps to Reproduce:
1. go to the URL above

Actual Results:  
The output of <xforms:output/> is not as predicted

Expected Results:  
That is described on the same test page.

Comment 1

[Koen Heene]

Attachment: an adapted version of the w3.org 7.11.1.a Test Page

The official w3.org test page has given the id="defaultInstance" to the instance they want to retrieve with instance() or instance(''). I don't find this possibility in the specs and therefore consider the Test Page erroneous.

The example in the attachment tries to correct that, but shows that the problems persists.

Comment 2

[Koen Heene]

Noteworthy in the attachment is the fact that the 2nd <xforms:output/> shows the label, but not the value. So it seems that instance() effectively retrieves the anonymous instance, but not the value identified by the XPath.

Comment 3

[aaronr]

Thanks for the bug.  We do not correctly handle instance() with no parameter or empty string parameter.

Comment 4

[Koen Heene]

Thank you for replying.

Would it be useful to mention on the Mozilla XForms status page that instance() is only partly supported?

I suppose that the status page is a very important reference for developers.

Thank you,
Koen

Comment 5

[aaronr]

updated the status page to include this bug

Comment 6

[aaronr]

Attachment: patch for 1.8

This fix is for the 1.8 branch.  The trunk will have to do something similar, but it also needs to overcome the fact that xpath extension functions aren't allowed to have optional parameters, yet.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 305630 [details] [diff] [review]
patch for 1.8


>+          rv = document->GetElementById(instanceId, getter_AddRefs(instEle));
>+        }
>+      } else {
>+        instanceId = EmptyString();
>+        useDefaultInst = PR_TRUE;
>+      }
>  
>       PRBool foundInstance = PR_FALSE;
>       nsAutoString localname, namespaceURI;
>       if (instEle) {
>         instEle->GetLocalName(localname);
>         instEle->GetNamespaceURI(namespaceURI);
>         if (localname.EqualsLiteral("instance") && 
>             namespaceURI.EqualsLiteral(NS_NAMESPACE_XFORMS)) {
>           foundInstance = PR_TRUE;
>         }
>       }

this one and

>-      if (!foundInstance) {
>+
>+      // if we were given an instance id and couldn't find it, then bow out
>+      if (!foundInstance && !useDefaultInst) {
>         // We didn't find an instance element with the given id.  Return the
>         //   empty result set.
>         *aResult = resultSet;
>         NS_ADDREF(*aResult);


this one may be moved after rv = document->GetElementById(instanceId, getter_AddRefs(instEle)), right?

>+      if (!useDefaultInst) {
>+        instNode = do_QueryInterface(instEle);
>+        rv = xformsService->GetModelFromNode(instNode, 
>+                                             getter_AddRefs(modelInstance)); 
>+                                   
>+        NS_ENSURE_SUCCESS(rv, rv);
>+   
>+        rv = xformsService->IsNodeAssocWithModel(xfContextNode, 
>+                                                 modelInstance, 
>+                                                 &modelContainsNode);
>+        NS_ENSURE_SUCCESS(rv, rv);

why can't you have common algorithm here? It sounds "else" case is suitable for the "if" case?

>+      } else {
>+        // xforms stores the instance element that was used to create the
>+        // standalone instance document as a property of that document
>+        nsCOMPtr<nsIDOMDocument> instanceDOMDoc;
>+        xfContextNode->GetOwnerDocument(getter_AddRefs(instanceDOMDoc));

please check this, iirc document->GetOwnerDocument should return document.

with comments addressed r=me

Comment 8

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

> >+      } else {
> >+        // xforms stores the instance element that was used to create the
> >+        // standalone instance document as a property of that document
> >+        nsCOMPtr<nsIDOMDocument> instanceDOMDoc;
> >+        xfContextNode->GetOwnerDocument(getter_AddRefs(instanceDOMDoc));
> 
> please check this, iirc document->GetOwnerDocument should return document.
Nope. That returns null. http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#node-ownerDoc

(in gecko we have nsINode::GetOwnerDoc() which always returns the owner document)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> > >+      } else {
> > >+        // xforms stores the instance element that was used to create the
> > >+        // standalone instance document as a property of that document
> > >+        nsCOMPtr<nsIDOMDocument> instanceDOMDoc;
> > >+        xfContextNode->GetOwnerDocument(getter_AddRefs(instanceDOMDoc));
> > 
> > please check this, iirc document->GetOwnerDocument should return document.
> Nope. That returns null.
> http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#node-ownerDoc
> 
> (in gecko we have nsINode::GetOwnerDoc() which always returns the owner
> document)
> 

Thank you. Then I messed them :)

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

RIP xforms