280366 - Context is not set properly if parent only has model attribute

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

The problem is that mBoundNode which is also used as context node, is only set
if there are node binding attributes (@ref / @bind).

Comment 1

[Allan Beaufour]

Attachment: Testcase

Comment 2

[Allan Beaufour]

*** Bug 299274 has been marked as a duplicate of this bug. ***

Comment 3

[Allan Beaufour]

Attachment: Patch

* Also triggers ResetBoundNode() routine if there is a model attribute
* If a control is not bound but has a model, GetContext() returns the document
element of the model's default instance document as the context node.

Comment 4

[Allan Beaufour]

Have you forgotten this Aaron?

Comment 5

[aaronr]

Comment on attachment 189879 [details] [diff] [review]
Patch


Ooops!	I did forget to review this patch.  Sorry!

>Index: nsXFormsControlStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v


>@@ -647,19 +661,17 @@
>                                               const nsAString &aValue)
> {
>   if (aName == nsXFormsAtoms::model ||
>       aName == nsXFormsAtoms::bind ||
>       aName == nsXFormsAtoms::ref) {
>     if (mModel)
>       mModel->RemoveFormControl(this);
> 
>-    if (aName != nsXFormsAtoms::model) {
>-      AddRemoveSNBAttr(aName, aValue);
>-    }
>+    AddRemoveSNBAttr(aName, aValue);
>   }
> }

I don't get why you want to increment mBindAttrsCount for an element that only
has a model attribute and thus allow it to go through the ProcessNodeBinding()
stuff.	To assign it a mModel and put the control on the control list for
mModel?

rest of the patch looks alright.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> (From update of attachment 189879 [details] [diff] [review] [edit])
> I don't get why you want to increment mBindAttrsCount for an element that only
> has a model attribute and thus allow it to go through the ProcessNodeBinding()
> stuff.	To assign it a mModel and put the control on the control list for
> mModel?

Because of bug 299274. "somebody" finds it useful to go through the process of
finding the model if there's only a mode attribute.

Comment 7

[aaronr]

Comment on attachment 189879 [details] [diff] [review]
Patch

Ugh, that stinks that we'll have to go through the whole binding process
knowing that we'll never have a chance to find a bound node.  I guess if this
turns out to be a semi common case we'll have to think about optimizing it
somehow.

Comment 8

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 189879 [details] [diff] [review]
Patch

Could you use -p with diff.
But anyway, r=me

Comment 9

[Allan Beaufour]

Checked in on trunk