regression: getting binding exceptions with repeats

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: aaronr, Assigned: aaronr)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
x86
All
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

891 bytes, application/xhtml+xml
Details
18.47 KB, patch
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
965 bytes, patch
surkov
: review+
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 252849 [details]
testcase
(Assignee)

Comment 2

11 years ago
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?
(Assignee)

Updated

11 years ago
Blocks: 353738

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
Created attachment 256903 [details] [diff] [review]
patch1

This fixes the regression, but introduces a problem with labels.  I'll look at it more tomorrow.
Assignee: xforms → aaronr
Status: NEW → ASSIGNED
(Assignee)

Comment 5

11 years ago
(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.
(Assignee)

Comment 6

11 years ago
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.
Attachment #256903 - Flags: review?(Olli.Pettay)

Comment 7

11 years ago
Comment on attachment 256903 [details] [diff] [review]
patch1

Nit, shouldn't sentences in comments start with capital letter.
Attachment #256903 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 8

11 years ago
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.
Attachment #256903 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #257438 - Flags: review?(doronr)

Updated

11 years ago
Attachment #257438 - Flags: review?(doronr) → review+

Comment 9

10 years ago
Checked into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Whiteboard: xf-to-branch

Comment 10

10 years ago
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

10 years ago
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

10 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

10 years ago
Created attachment 258089 [details] [diff] [review]
patch compile error

fixes 1.8 branch compile error
(Assignee)

Updated

10 years ago
Attachment #258089 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #258089 - Flags: review?(doronr)

Updated

10 years ago
Attachment #258089 - Flags: review?(surkov.alexander) → review+

Comment 14

10 years ago
Comment on attachment 258089 [details] [diff] [review]
patch compile error

>+  return;
> }
You're not *required* to end a void function with a return ;-)

Updated

10 years ago
Attachment #258089 - Flags: review?(doronr) → review+
(Assignee)

Comment 15

10 years ago
checked in branch compile fix to trunk
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.