Last Comment Bug 391586 - bind on an insert changes the in-scope evaluation context
: bind on an insert changes the in-scope evaluation context
Status: RESOLVED FIXED
: fixed1.8.1.12
Product: Core
Classification: Components
Component: XForms (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Merle Sterling
:
Mentors:
Depends on:
Blocks: 398271 410239
  Show dependency treegraph
 
Reported: 2007-08-09 14:00 PDT by John L. Clark
Modified: 2008-01-08 19:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demonstration of bug report (1.34 KB, application/xhtml+xml)
2007-08-09 14:02 PDT, John L. Clark
no flags Details
testcase: using @model (1.39 KB, application/xhtml+xml)
2007-08-16 15:11 PDT, Merle Sterling
no flags Details
patch (12.94 KB, patch)
2007-08-16 15:13 PDT, Merle Sterling
aaronr: review+
Details | Diff | Review
patch 2 (13.10 KB, patch)
2007-09-04 07:15 PDT, Merle Sterling
bugs: review+
Details | Diff | Review

Description John L. Clark 2007-08-09 14:00:42 PDT
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

When an `xf:insert` uses a bind reference to identify the Node Set Binding node-set, then it also changes the in-scope evaluation context, which means that a relative XPath expression in the `origin` attribute will be evaluated with respect to the bind location, not the original in-scope evaluation context.

Reproducible: Always
Comment 1 John L. Clark 2007-08-09 14:02:44 PDT
Created attachment 276031 [details]
Demonstration of bug report

The insert should copy (i.e. insert) a date element with a text node content, but instead it copies a date element with an empty element child, which is only present at the bind location (not the origin location).
Comment 2 aaronr 2007-08-09 14:26:44 PDT
Orbeon and Sidewinder both behave the same, which is different from our processor.  We need to figure out why and who is right.  According to the spec, "1. The insert context is determined. If the bind attribute is present or if the context attribute is not given, the insert context is the in-scope evaluation context".  So to me, that means in this testcase the evaluation context should be: instance('containers')/target/date since that is the nodeset that @bind gives.  So to me we are behaving correctly, but we certainly aren't behaving like the other processors.
Comment 3 aaronr 2007-08-09 16:46:04 PDT
I asked the W3C what the correct behavior is: http://lists.w3.org/Archives/Public/www-forms/2007Aug/0001.html
Comment 4 aaronr 2007-08-10 13:55:06 PDT
(In reply to comment #3)
> I asked the W3C what the correct behavior is:
> http://lists.w3.org/Archives/Public/www-forms/2007Aug/0001.html
> 

The WG has spoken...the result from @bind should NOT provide the context for xpath evaluations on the xf:insert element.  Orbeon and Sidewinder's behaviors are correct.  Merle, please fix this accordingly.  If this points out a flaw in the testsuite testcase, please mention that to Steve or Keith.  Thanks.
Comment 5 Merle Sterling 2007-08-16 15:11:52 PDT
Created attachment 277017 [details]
testcase: using @model

Additional testcase that uses @model instead of @bind.
Comment 6 Merle Sterling 2007-08-16 15:13:18 PDT
Created attachment 277019 [details] [diff] [review]
patch
Comment 7 aaronr 2007-08-28 19:07:39 PDT
Comment on attachment 277019 [details] [diff] [review]
patch

>Index: nsXFormsInsertDeleteElement.cpp
>===================================================================

>+  rv = nsXFormsUtils::GetNodeContext(mElement,
>+                                     nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                     getter_AddRefs(model),
>+                                     getter_AddRefs(bindElement),
>+                                     &outerBind,
>+                                     getter_AddRefs(parentControl),
>+                                     getter_AddRefs(contextNode),
>+                                     nsnull, nsnull, PR_FALSE);

nit: perhaps a comment here to point out that we are passing in PR_FALSE to tell GetNodeContext not to grab the context node from @bind.

>+  // The insert/delete action is terminated with no effect if the context
>+  // is the empty node-set.
>+  //if (!model || !contextNode)

nit: remove this commented out line if you don't need it.

with those, r=me
Comment 8 Merle Sterling 2007-09-04 07:15:45 PDT
Created attachment 279597 [details] [diff] [review]
patch 2

Add comment; remove extraneous commented out code.
Comment 9 Olli Pettay [:smaug] 2007-09-06 02:43:47 PDT
Comment on attachment 279597 [details] [diff] [review]
patch 2

Not sure if XForms patches need approval while in
M8 freeze. XForms is not part of the build.
Comment 10 Olli Pettay [:smaug] 2007-09-06 11:16:48 PDT
Comment on attachment 279597 [details] [diff] [review]
patch 2

According to mozilla.dev.planning, this doesn't need approval
Comment 11 Olli Pettay [:smaug] 2007-09-06 11:26:58 PDT
checked in
Comment 12 aaronr 2008-01-08 19:22:06 PST
checked into 1.8 branch via bug 410239.

Note You need to log in before you can comment on or make changes to this bug.