293082 - Double types on instance node does not work

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

I simplified down a testcase that was causing relevance not to work.  Yet the
same testcase works fine in Novell's plugin and XSmiles.  So we need to
determine if it is a testcase problem or a scenario that we should support.  Or
even both! :=)

It seems to be related to the fact that this testcase defines a type on the
instance node via a xsi:type attribute and also via @type on a bind to that
node.  Even though both types are the same, it is causing a problem.  A control
that should be 'disabled' and irrelevant is not rendering that way.

Comment 1

[aaronr]

Attachment: testcase

input and its label should not show by default and should toggle between hidden
and shown when the checkbox is clicked.

Comment 2

[Allan Beaufour]

(In reply to comment #0)
> I simplified down a testcase that was causing relevance not to work.  Yet the
> same testcase works fine in Novell's plugin and XSmiles.  So we need to
> determine if it is a testcase problem or a scenario that we should support.  Or
> even both! :=)
>
> It seems to be related to the fact that this testcase defines a type on the
> instance node via a xsi:type attribute and also via @type on a bind to that
> node.  Even though both types are the same, it is causing a problem.  A control
> that should be 'disabled' and irrelevant is not rendering that way.

I've recently added some error messages, so you actually get this error:
"XForms Error (7): Multiple defined model item property on element". So yes, it
is because it is defined twice. Removing any one of the two type definitions
make it work.

Defining the same MIP twice is not allowed, but we could make an exception for
types if we want. I guess that would be ok. Should be close to a onliner, but
depend on bug 283004.

Comment 3

[Allan Beaufour]

Attachment: Patch

Here's a patch removing the multiple MIP check for @type.

Comment 4

[Allan Beaufour]

Actually, because of bug 283004, this also means that a rebuild fails when you
bind a type to an instance node (because the type attribute is already there)...

Comment 5

[aaronr]

chatted with Allan about this and I think that we are handling it right.  At
least according to 4.2.1.c.  If we encounter such a condition we should halt
processing and throw the xforms-binding-exception event which is exactly what we
do.  Will send a note to David to see why Novell's plugin seems to handle it so
well.

Comment 6

[Allan Beaufour]

We handle it correctly, but when we start storing the types as properties (bug
283004) we need to handle types set directly on the instance node. This bug
should fix that.

Comment 7

[Allan Beaufour]

Attachment: Patch

This patch checks @type, if there's no MIP type stored for the instance node.

Comment 8

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

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

> 
>   nsAutoString *typeVal = nsnull;
>+
>+  // Get MIP type bound to node
>   nsCOMPtr<nsIContent> nodeContent(do_QueryInterface(aInstanceData));
>   if (nodeContent) {
>     typeVal =
>       NS_STATIC_CAST(nsAutoString*,
>                      nodeContent->GetProperty(nsXFormsAtoms::type, &rv));
>   } else {
>     nsCOMPtr<nsIAttribute> nodeAttribute(do_QueryInterface(aInstanceData));
>     if (!nodeAttribute)
>       // node is neither content or attribute!
>       return NS_ERROR_FAILURE;
> 
>     typeVal =
>       NS_STATIC_CAST(nsAutoString*,
>                      nodeAttribute->GetProperty(nsXFormsAtoms::type, &rv));
>   }
> 
>+  // Get type stored directly on instance node
>+  nsAutoString typeAttribute;
>   if (NS_FAILED(rv) || !typeVal) {
>+    nsCOMPtr<nsIDOMElement> nodeElem(do_QueryInterface(aInstanceData));
>+    if (nodeElem) {
>+      nodeElem->GetAttributeNS(NS_LITERAL_STRING(NS_NAMESPACE_XML_SCHEMA_INSTANCE),
>+                               NS_LITERAL_STRING("type"), typeAttribute);
>+      if (!typeAttribute.IsEmpty()) {
>+        typeVal = &typeAttribute;

Shouldn't you create a copy of typeAttribute here?
Or who owns the memory pointed by &typeAttribute after the method returns?

Comment 9

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

(In reply to comment #8)
> (From update of attachment 186100 [details] [diff] [review] [edit])

bah, nm. Sorry.

Comment 10

[aaronr]

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

Patch looks fine, but the order seems wrong.  Check out:
http://www.w3.org/TR/xforms/index-all.html#model-using-atomic

Looks like the check for xsi:type should be before the bound type check.

Comment 11

[Allan Beaufour]

Attachment: Patch w/Aaron's comment fixed

Good catch. I was searching for that in the spec. but didn't find it...

Comment 12

[Allan Beaufour]

Checked in