293597 - itemset does not work with external instance

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

It seems like itemset breaks when it is bound to external instance data

Comment 1

[Allan Beaufour]

Attachment: External instance data

Comment 2

[Allan Beaufour]

Attachment: Testcase

Comment 3

[aaronr]

Just need to link this in with my patch for 283737 and it should work.  But that
means using ProcessNodeBind instead of EvaluateNodeBinding in ::Refresh.  Or
alternatively bringing in the check for the nsXFormsAtoms::readyForBindProperty
on  the document and adding to the deferred list in nsXFormsItemSet::Refresh. 
'Course, either way the itemset needs to be a nsXFormsControlStub with all of
the accompanying changes to make that happen.  Or alter my fix for 283737 to
allow nsIDOMElements onto the deferred list.

Comment 4

[Doron Rosenberg (IBM)]

This still isn't fixed btw, even with those patches

Comment 5

[aaronr]

Sorry, I didn't mean that it would be FIXED by bug 283737, but rather once
283737 was in, that we should be able to hook itemset into that type of logic ->
making sure that the itemset isn't bound until the external instance is loaded.

Comment 6

[Doron Rosenberg (IBM)]

Attachment: patch

Comment 7

[Doron Rosenberg (IBM)]

So the patch does this:

ItemSet's refresh now defers binding.  Since it is a non-visual class, I made
the defer comarray a nsISupports*.  I also added a nsIXFormsBindableElement
class, and made XFormsStubElement (non-visual controls) implement it.  So now
when defered controls are processed, I can QI non-visual controls to a common
class and call refresh on it.  I had to update those classes to make their
refresh an NSIMETHOD and similar XPCOM fun.

Comments welcome ;)

Comment 8

[aaronr]

I haven't read the whole patch, yet, so this may be an ignorant statement.  But
why are only the non-visual elements implementing the nsIXFormsBindableElement?
 I would suggest having both nsXFormsControlStub(Base) and nsXFormsStubElement
implement it.  I also suggest putting the tests that involve
doc->GetProperty(nsXFormsAtoms::readyForBindProperty) into a utils method or
maybe a utility method on model.  Itemset, etc. shouldn't care that this
particular property is set or not specifically, but whether the model is in a
particular mode or not (that in the future may or may not be best represented by
looking for this property).

That is what I got out of my quick glance at the patch.

Comment 9

[Doron Rosenberg (IBM)]

Attachment: comments addressed

Comment 10

[aaronr]

Comment on attachment 186246 [details] [diff] [review]
comments addressed


>-NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsControlStub,
>+NS_IMPL_ISUPPORTS_INHERITED3(nsXFormsControlStub,
>                              nsXFormsXMLVisualStub,
>                              nsIXFormsContextControl,
>-                             nsIXFormsControl)
>+                             nsIXFormsControl, nsIXFormsControlBase)
> 

nit: move nsIXFormsControlBase to the next line since that is the style in this
file.


deferredBindList->ObjectAt(i);
>-      if (control) {
>-        control->Bind();
>-        control->Refresh();
>+      nsIXFormsControlBase *foo = deferredBindList->ObjectAt(i);
>+      if (foo) {
>+        foo->Bind();
>+        foo->Refresh();
>       }
>     }

nit: Don't use a variable called foo!  Descriptive variable names, please.

with those, r=me

Comment 11

[Doron Rosenberg (IBM)]

Attachment: smaug comments addressed

Comment 12

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

Comment on attachment 186516 [details] [diff] [review]
smaug comments addressed

>-NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsControlStub,
>+NS_IMPL_ISUPPORTS_INHERITED3(nsXFormsControlStub,
>                              nsXFormsXMLVisualStub,
>                              nsIXFormsContextControl,
>-                             nsIXFormsControl)
>+                             nsIXFormsControl, nsIXFormsControlBase)
> 

nsIXFormsControlBase to its own line, please.

Comment 13

[Doron Rosenberg (IBM)]

checked in

Comment 14

[Alexey Gaidukov]

With FF 3.6.3 I get the following error

Error: XForms Error (31): Security check failed! Trying to load instance data from a different domain than document
Source: https://bug293597.bugzilla.mozilla.org/attachment.cgi?id=183146
Line: 0
Source code:
<xforms:instance src="https://bugzilla.mozilla.org/attachment.cgi?id=183145"/>

Comment 15

[Alexey Gaidukov]

When I saved testcase's files to local disk and changed to
<xforms:instance src="flavors-external.xml"/>
I get the same error. Looks like regression.

Comment 16

[Alexey Gaidukov]

Attachment: testcase2

Comment 17

[Alexey Gaidukov]

(In reply to comment #15)
> When I saved testcase's files to local disk and changed to
> <xforms:instance src="flavors-external.xml"/>
> I get the same error. Looks like regression.

I was wrong. There is no regression. In the testcase2 I changed src to correct value for current bugzilla behavior.

Comment 18

[Alexey Gaidukov]

Aaron could you explain if in the testcase I select any element from one of two xf:select control I get exception: 
XForms Error (42): output element may not be bound to complex content.

Is it known bug?

Comment 19

[aaronr]

(In reply to comment #18)
> Aaron could you explain if in the testcase I select any element from one of two
> xf:select control I get exception: 
> XForms Error (42): output element may not be bound to complex content.
> 
> Is it known bug?

This isn't a bug.  If you look at: http://www.w3.org/TR/xforms11/#ui-output, you can see:

Data Binding Restrictions: Binds to any simpleContent.

Note:

This control cannot bind to element nodes that have element children. See 8.1.1 Implementation Requirements Common to All Form Controls for user interface processing rules common to all form controls.

If you look at the instance data that the output is bound to after the xf:copy happens, there'll be elements under it.