368248 - regression: getting binding exceptions with repeats

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

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]

Attachment: testcase

Comment 2

[aaronr]

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 (:asurkov)]

(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]

Attachment: patch1

This fixes the regression, but introduces a problem with labels.  I'll look at it more tomorrow.

Comment 5

[aaronr]

(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]

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][[e-mail]]]

Comment on attachment 256903 [details] [diff] [review]
patch1

Nit, shouldn't sentences in comments start with capital letter.

Comment 8

[aaronr]

Attachment: 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)]

Checked into trunk

Comment 10

[[e-mail]]

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

[[e-mail]]

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 (:asurkov)]

(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]

Attachment: patch compile error

fixes 1.8 branch compile error

Comment 14

[[e-mail]]

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]

checked in branch compile fix to trunk

Comment 16

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16