308106 - controls not bound to external instance data in xul

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1

The bug like a bug 279021. When I try to use xforms in xul then I get an exception:

Error: XForms Error (16): Could not bind control to instance data
Source File: chrome://xfcalc/content/protocol2.xul
Line: 0
Source Code:
<xforms:input enabled="1" bind="firstnameemployee1" class="editField"/>

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: xml data

Comment 2

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 3

[Allan Beaufour]

This is funky stuff...

Bug 302915 should have fixed Begin/DoneAddingChildren() for XUL. I do not think
it did the job 100%. If I have a test form with an <xforms:instance src="..."/>
only BeginAddingChildren() are called. That means that a lazy instance is
created if a control tries to bind to anything. The lazy instance has _both_
Begin/Done() called... but only the first time. A refresh of the page only
triggers the Begin() function on the DOM instance.

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> The lazy instance has _both_  Begin/Done() called... but only the first time.
> A refresh of the page only triggers the Begin() function on the DOM instance.

This is the XUL cache doing tricks on us. Setting
"nglayout.debug.disable_xul_cache" to true gives consistent behaviour.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Argh, my bug.
But it helps if you add just any content to the instance element,
for example <xforms:instance src="externalInstance.xml"> </xforms:instance>

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

This should be fixed now in trunk.
Bug 312743

Comment 7

[Allan Beaufour]

Confirmed, bug 312743 helped. Problem is that we missed Firefox 1.5 :(. But the
work around in comment 6 can be used.

*** This bug has been marked as a duplicate of 312743 ***

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Reopening, because we can make this work also in branch.
Atm nsXFormsInstanceElement uses begin/doneAddingChildren to initialize itself,
but it could be the nsXFormsModelElement which initilizes instances.
So adding Init() to nsIInstanceElementPrivate and
iterating child nodes in nsXFormsModelElement::DoneAddingChildren()
(that one is called)  and if the child is a nsIInstanceElementPrivate,
call the Init() (which would do the same thing
as nsXFormsInstanceElement::DoneAddingChildren() atm). 
Hmm, there is already nsXFormsInstanceElement::InitializeLazyInstance(),
that could be removed and the functionality moved to the normal Init().

There is one case this change doesn't handle. It is the empty <xforms:model/> in 
a XUL document (<xforms:model> </xforms:model> does work).
Otherwise the DoneAddingChildren bug shouldn't affect XForms.
In those UI elements which are using DoneAddingChildren,
there should anyway be some child nodes.

And it is even possible to do a small hack to fix the <xforms:model/>
problem. Model element is anyway listening DOMContentLoaded, so when that
event occurs, it can be sure that no child nodes will be added by the content 
sink

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test case, XUL + <xforms:model/> + lazy instance

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Model initializes instances

This merges nsXFormsInstanceElement::DoneAddingChildren and  
nsXFormsInstanceElement::InitializeLazyInstance() to
nsXFormsInstanceElement::Initialize(). Model initializes instances.
The hack which is needed for 1.8 branch (but shouldn't hurt trunk)
is the InitializeInstances() call while handling DOMContentLoaded.

Tested using trunk with and without the fix for DoneAddingChildren.

Comment 11

[Allan Beaufour]

Comment on attachment 200122 [details] [diff] [review]
Model initializes instances

> -NS_IMETHODIMP
> -nsXFormsInstanceElement::ParentChanged(nsIDOMElement *aNewParent)
> -{
> -  if (!aNewParent || mAddingChildren || mLazy)
> -    return NS_OK;
> -
> -  // Once we are set up in the DOM, can find the model and make sure that this
> -  // instance is on the list of instance elements that model keeps
> -  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
> -  if (model) {
> -    model->AddInstanceElement(this);
> -  }
> -  
> -  return NS_OK;
> -}
> -

What if the parent is changed, then? Not terribly important to me though.

> @@ -866,16 +898,21 @@ nsXFormsModelElement::HandleEvent(nsIDOM
>    if (!nsXFormsUtils::EventHandlingAllowed(aEvent, mElement))
>      return NS_OK;

>    nsAutoString type;
>    aEvent->GetType(type);
>    if (!type.EqualsLiteral("DOMContentLoaded"))
>      return NS_OK;

> +  if (!mInstancesInitialized) {
> +    // XXX This is for Bug 308106.

Please add a more descriptive comment.

I'd like a handler for ParentChanged(), but it's not important r=me

Comment 12

[aaronr]

Comment on attachment 200122 [details] [diff] [review]
Model initializes instances

I'm fine with it as it is.   Nice cleanup.  But if you do go Allan's way with
keeping ::ParentChanged and having it add instance elements to the model, then
you should do ::WillParentChanged to remove instances from models if removing
the instance element from the model.  And that could cause some issues.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I'll check in this patch. (just up-to-date with trunk and a better comment).
I didn't add the WillChangeParent/ParentChanged because that
wasn't working earlier either and it needs more changes...
and I'm not sure if we wan't that at all.
(When should we dispatch events related to model construction if instances can
be added dynamically. And what should happen to the lazy instance. etc.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Checked in

Comment 15

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 16

[aaronr]

verified fixed in MOZILLA_1_8_BRANCH