393416 - Insert or delete of one node can trigger xforms-value-changed on a separate form control

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[John L. Clark]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: 2007-08-09-03-mozilla1.8

In some cases, performing an `xf:insert` or an `xf:delete` of one node can cause a form control bound to a separate node to observe an `xforms-value-changed` event.  This can inadvertently trigger events bound to that event even when the corresponding bound node hasn't actually changed.  I will attach a test case that illustrates this issue.

Reproducible: Always

Comment 1

[John L. Clark]

Attachment: Test form demonstrating the bug

First, select a value for "Field 1"; a message will appear saying "xforms-value-changed observed at `field1`".  This is expected (as the value for the bound node has changed).  Now activate the "Delete" trigger, and the same message will appear.  Finally, activate the "New" trigger, and the message will appear again.  These last two messages should not appear (or to be more precise, the `xforms-value-changed` event that led to these messages should not happen), because the value of the `field1` element has not been changed.

There are several interesting things to note about this form.  If you remove the `xf:bind` in the form (lines 31-32), the "extra" `xforms-value-changed` events are no longer fired.  If you uncomment the `xf:rebuild` and `xf:refresh` actions at lines 53 and 63, respectively, then the corresponding `xf:delete` and `xf:insert` actions no longer cause the "extra" `xforms-value-changed` events to be fired.  Finally, if you only uncomment the `xf:refresh` from those pairs, the triggers need to be pressed twice before the corresponding `DOMActivate` action happens.

Comment 2

[John L. Clark]

Attachment: Fix for this bug

I believe that this patch safely fixes the first problem mentioned in this bug report.  The last chunk of the patch is the only one of real interest; the other chunks simply tweak or clean up some debug messages.  The last chunk moves the setting of the `eFlag_DISPATCH_VALUE_CHANGED` flag into the body of the `g->HasExpr()` test.  As a result, an "xforms-value-changed" should only be fired for a dirty calculate node that also has an expression.  Now, a calculate node without an expression is a start node for the dependency graph, so it will not undergo a change itself, and so should not indicate that its value changed.

Upon closer reading of the spec, I don't think the other issue is a bug; manually triggering a refresh using the `xf:refresh` does clear the refresh flag, so even though an insert or a delete was performed, only the rebuild, recalculate, and revalidate sequence will happen at the end of the outer action, so the display will not be updated until the *next* time a refresh is triggered.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 284300 [details] [diff] [review]
Fix for this bug

John you need to ask someone for review, for example, Aaron :)

Comment 4

[aaronr]

Comment on attachment 284300 [details] [diff] [review]
Fix for this bug

makes sense to me.  r=me

Comment 5

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

Comment on attachment 284300 [details] [diff] [review]
Fix for this bug

(Usually when making patches, -u8 -p are good parameters for cvs diff.)

Comment 6

[aaronr]

checked into trunk for jlc

Comment 7

[aaronr]

checked into 1.8 branch via bug 410239.