332211 - Problem updating UI with external instances

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P1)

Description

[Allan Beaufour]

There seems to be a problem with updating the UI when you use external instances. This might be a regression from bug 305060.

Comment 1

[Allan Beaufour]

Attachment: External instance file

Comment 2

[Allan Beaufour]

Attachment: Testcase w/external instance

Comment 3

[Allan Beaufour]

Attachment: Testcase w/inline instance

Comment 4

[aaronr]

Ok, during initialization we call model::rebuild during FinishConstruction.  Rebuild will set mNeedsRefresh based on the test for mDocumentLoaded.  So the result is if we call rebuild after mDocumentLoaded is set, we'll refresh every control in the model.  Looks like we expect mDocumentLoaded to be PR_FALSE during initialization, and PR_TRUE after initialization.  However, mDocumentLoaded could be PR_TRUE if the external instance load doesn't complete until after the xforms document receives DOMContentLoaded.  In this case we'll set mNeedsRefresh.  Thus, the next time the model needs to refresh, it will refresh every control it contains rather than just the ones that depend on the changed node.  In the failing testcase this means that after we select an item from the itemset, we then  refresh all the controls, which will cause the itemset to purge all of its items from the document and clone new copies.  So now the item that select1 thinks is currently selected is no longer in the DOM which will cause problems when the select1 goes to refresh.

Comment 5

[aaronr]

Attachment: first attempt

Well, the fix I propose is to us mReadyHandled instead of mDocumentLoaded.  I'd argue that the form author shouldn't expect that monkeying with and then rebuilding the instance should work before xforms-ready.  And certainly not before xforms-model-construct-done.  There is a gap in between construct done and ready where we could possibly honor it.  But in that case we should probably only refresh the controls that could bind and not refresh those that may still be on the deferredbindlist.  But I'll float the mReadyHandled fix to Allan first to see what he thinks before going through the other complications.

Comment 6

[Allan Beaufour]

Comment on attachment 220203 [details] [diff] [review]
first attempt

(In reply to comment #5)
> Well, the fix I propose is to us mReadyHandled instead of mDocumentLoaded.  I'd
> argue that the form author shouldn't expect that monkeying with and then
> rebuilding the instance should work before xforms-ready.  And certainly not
> before xforms-model-construct-done.  There is a gap in between construct done
> and ready where we could possibly honor it.  But in that case we should
> probably only refresh the controls that could bind and not refresh those that
> may still be on the deferredbindlist.  But I'll float the mReadyHandled fix to
> Allan first to see what he thinks before going through the other complications.

Hmm, it should not. But even if it does, something else is wrong which should be fixed, because your logic seems 100% correct.

r=me

Comment 7

[aaronr]

checked into trunk