Last Comment Bug 368248 - regression: getting binding exceptions with repeats
: regression: getting binding exceptions with repeats
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
Depends on:
Blocks: 353738
  Show dependency treegraph
 
Reported: 2007-01-25 16:53 PST by aaronr
Modified: 2016-07-15 14:46 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (891 bytes, application/xhtml+xml)
2007-01-25 16:54 PST, aaronr
no flags Details
patch1 (18.47 KB, patch)
2007-03-01 03:41 PST, aaronr
bugs: review+
Details | Diff | Splinter Review
patch2 (18.47 KB, patch)
2007-03-05 15:37 PST, aaronr
doronr: review+
Details | Diff | Splinter Review
patch compile error (965 bytes, patch)
2007-03-09 23:56 PST, aaronr
surkov.alexander: review+
doronr: review+
Details | Diff | Splinter Review

Description aaronr 2007-01-25 16:53:05 PST
After the patch for bug 356190 went in, we are now getting xforms-binding-exceptions when using repeats with children that are relative to the repeat's nodeset.

This is because during nsXFormsModelElement::InitializeControls in addition to binding the children of the contextContainers (cloned from the original repeat content), we are also binding the original repeat content since these controls are also in the nsXFormsControlList.  Since there is no contextContainer containing these repeat children, the context for the binding expressions will be wrong.  If the controls, for example, have @ref=".", this could be evaluated in the context of the root of the instance document.  When IsContentComplex() is called, then if the instance document contains any other element nodes, this will come back as having complex content.  Which is forbidden for almost all XForms controls.

The fix will probably be not putting the children of repeats in the control list.  I'm just wondering if this will cause other problems.  I don't THINK that repeat relies on its contents being bound and refreshed, but I'm not sure.
Comment 1 aaronr 2007-01-25 16:54:22 PST
Created attachment 252849 [details]
testcase
Comment 2 aaronr 2007-01-26 15:13:12 PST
We already have eType_Template to keep XForms controls that are members of the templates for xf:repeats or xf:itemsets from refreshing (see nsXFormsDelegateStubb::Refresh()).  It is logical to think that if a control doesn't need to refresh, it probably doesn't need to bind.  If neither of these are the case, then they probably don't need to be in the model's control list, either.  Right now we assign eType_Template and eType_Generated to a control when the XBL attaches to the widget.  If instead we did this at the time that the template is first used as the source of cloning to create the generated content, then we'd have this state flag available to us when we are in the process of binding them to the model for the first time (and thus inserting them into the model's control list).

There *MIGHT* be a slight performance impact.  Right now, we know if something is going through widgetAttached it is a nsIXFormsControl so all that kind of testing doesn't need to happen.  We just look up the parent chain for an itemset, a repeat or a contextContainer and if we find one of them we set the eType flag appropriately.  In my suggestion we'd have to see if the node being cloned was a XForms node.  And even in a repeat where the contents are only xforms elements, all of those empty text nodes would have to be tested, too.

I guess, perhaps, we could instead put the call to UpdateRepeatState() in BindToModel, but then we might still need the call in WidgetAttached() for any element that doesn't have a binding attribute.

Or perhaps put put something in nsXFormsDelegateStub::ParentChanged so that if the parent node is a xforms element, test to see if it is a template and if so, make the added child a template, too.  Then make any child added to a xf:repeat or a xf:itemset be a template (except in the node cloning functions where the context containers are added, of course).

Any other ideas/suggestions?
Comment 3 alexander :surkov 2007-01-28 03:36:18 PST
(In reply to comment #2)
> We already have eType_Template to keep XForms controls that are members of the
> templates for xf:repeats or xf:itemsets from refreshing (see
> nsXFormsDelegateStubb::Refresh()).  It is logical to think that if a control
> doesn't need to refresh, it probably doesn't need to bind.  If neither of these
> are the case, then they probably don't need to be in the model's control list,
> either.

I think it's reasonable.
Comment 4 aaronr 2007-03-01 03:41:07 PST
Created attachment 256903 [details] [diff] [review]
patch1

This fixes the regression, but introduces a problem with labels.  I'll look at it more tomorrow.
Comment 5 aaronr 2007-03-01 15:25:09 PST
(In reply to comment #4)
> Created an attachment (id=256903) [details]
> patch1
> 
> This fixes the regression, but introduces a problem with labels.  I'll look at
> it more tomorrow.
> 

ugh, there is a problem with labels all right, but not due to this patch.  Some other regression that occurs with or without this patch.
Comment 6 aaronr 2007-03-01 15:42:20 PST
Comment on attachment 256903 [details] [diff] [review]
patch1

basically moved the repeat state code from nsXFormsDelegateStub to nsXFormsControlStub.  Also rather than just running the repeat state code only on widgetAttached, I run it every time a control's parent changes, which keeps the value more accurate.  I am also not allowing a control to bind or be added to a model control list if it is a template control, which is what really fixes the regression.
Comment 7 Olli Pettay [:smaug] 2007-03-05 15:23:49 PST
Comment on attachment 256903 [details] [diff] [review]
patch1

Nit, shouldn't sentences in comments start with capital letter.
Comment 8 aaronr 2007-03-05 15:37:45 PST
Created attachment 257438 [details] [diff] [review]
patch2

fixes smaug's nit.  We really should capitalize the first letter of a comment if it forms a complete sentence and not just a quick thought.  Especially when it is part of a two or three line comment.
Comment 9 Doron Rosenberg (IBM) 2007-03-07 09:47:12 PST
Checked into trunk
Comment 10 neil@parkwaycc.co.uk 2007-03-09 05:22:19 PST
Comment on attachment 257438 [details] [diff] [review]
patch2

>+  void SetRepeatState(nsRepeatState aState);

>+void
>+nsXFormsContextContainer::SetRepeatState(nsRepeatState aState)
>+{
>+  // A context container can have one of two states...eType_Unknown
>+  // (uninitialized) and eType_GeneratedContent.  This assumes
>+  // that one cannot be generated outside of the repeat or itemset code.
>+
>+  nsRepeatState state = aState;
>+  if (state != eType_Unknown) {
>+    state = eType_GeneratedContent;
>+  }
>+
>+  return nsXFormsControlStub::SetRepeatState(state);
>+}

>+  virtual void SetRepeatState(nsRepeatState aState);

Anyone know when it became legal for a void function to return the void value from another void function?
Comment 11 neil@parkwaycc.co.uk 2007-03-09 05:57:33 PST
OK, so apparently C++ now allows it in case you're writing a template.

I wonder what xf-to-branch might mean though; if you mean 1.8 branch,
then watch out, because VC6 won't compile that.
Comment 12 alexander :surkov 2007-03-09 07:59:01 PST
(In reply to comment #11)
> OK, so apparently C++ now allows it in case you're writing a template.
> 
> I wonder what xf-to-branch might mean though; if you mean 1.8 branch,
> then watch out, because VC6 won't compile that.
> 

You're right xf-to-branch means 1.8 branch. Thank you for the catch.
Comment 13 aaronr 2007-03-09 23:56:52 PST
Created attachment 258089 [details] [diff] [review]
patch compile error

fixes 1.8 branch compile error
Comment 14 neil@parkwaycc.co.uk 2007-03-10 04:12:52 PST
Comment on attachment 258089 [details] [diff] [review]
patch compile error

>+  return;
> }
You're not *required* to end a void function with a return ;-)
Comment 15 aaronr 2007-03-12 22:48:43 PDT
checked in branch compile fix to trunk
Comment 16 aaronr 2007-04-23 16:16:28 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

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