310276 - output isn't refreshed in some cases

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P2)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

Element 'output' has attribute value:
<xf:output value="/example/first"/>
<xf:input bind="first"/>

Model has two 'bind' elements like shown below:

<xf:model id="model">
	<xf:instance>
		<example xmlns="">
			<first>true</first>
			<second>true</second>
		</example>
	</xf:instance>

	<xf:bind nodeset="/example/first" id="first" type="xsd:boolean"/>
	<xf:bind nodeset="/example/second" id="second" type="xsd:boolean"/>
</xf:model>

In this case when I click on checkbox then 'refresh' event isn't posted for output.

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 2

[Allan Beaufour]

Mighty weird. The first time or two you click the input, the output is not
refreshed. But after a few clicks it works...

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Mighty weird. The first time or two you click the input, the output is not
> refreshed. But after a few clicks it works...

Only first time. At second time refresh event is posted.

Comment 4

[Doron Rosenberg (IBM)]

allan: in the test case, when I click on the checkbox, I get 2 changed nodes when this bug happens: the 2 instance data nodes, even though only one of them is bound to.  Is that a bug?

I can "fix" that (the second node in the change nodes list) by removing the type attribute off the 2nd bind.

Comment 5

[aaronr]

Attachment: patch

Doron debugged this and helped with the patch, so if it sux, its his fault :-)

The problem turned out to be that in nsXFormsModelElement::Refresh we were breaking out of the 'check for dirty dependencies' for loop when we detected that we needed to rebind the control, but we were still in the for loop that was checking for changed nodes.  So we'd check the next changed node and enter the  for loop for dirty dependency checking and that would change the rebind flag back to false.  So in the end, we wouldn't end up rebinding the output when we  should have.  As far as I can tell, we should jump out of both for loops once we determine that we need to rebind the control.

During this debugging Doron also noticed that we could never have mBoundNode set up if output only had a value attribute.  I also thought it would be more efficient not to call ProcessNodeBinding under Bind if it couldn't set us up with a bound node anyhow.  So I added the parts of ProcessNodeBinding that assigns mModel and adds the control to the deferred bind list to output's Bind code.  The rest of the services that ProcessNodeBinding provides can come with the call to it during output's Refresh (refresh already follows almost immediately after every call to bind)

Comment 6

[Allan Beaufour]

Comment on attachment 215944 [details] [diff] [review]
patch

oh, correct. change the output changes to an early return (if (!mHasBinding) ... return).

With that r=me

Comment 7

[Doron Rosenberg (IBM)]

Comment on attachment 215944 [details] [diff] [review]
patch

> 
>-        for (PRInt32 j = 0; j < mChangedNodes.Count(); ++j) {
>+        for (PRInt32 j = 0; j < mChangedNodes.Count() && !rebind; ++j) {
>           curChanged = do_QueryInterface(mChangedNodes[j]);
> 
>           // Check whether the bound node is dirty. If so, we need to refresh the
>           // control (get updated node value from the bound node)
>           if (!refresh && boundNode) {

perhaps add a comment about why we are doing a !rebind?

Comment 8

[aaronr]

Attachment: patch with nits fixed

fixes nits by Allan and Doron

Comment 9

[aaronr]

checked into the trunk.