We should not dispatch events on form load

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
11 months ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

({fixed1.8.0.4, fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.4, fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Form construction does not include an xforms-refresh:
http://www.w3.org/TR/2005/PER-xforms-20051006/slice4.html#evt-modelConstruct
so we should not dispatch events on load.
(Assignee)

Comment 1

11 years ago
Created attachment 214318 [details]
Testcase
(Assignee)

Comment 2

11 years ago
Created attachment 214320 [details] [diff] [review]
Work in progress

Hmmm, we might, internally, rely on the xforms-refresh for external instances, and I actually think that the mModel->SetStates() is wrong. The states should be set during the xforms-refresh cycle, not if just Bind() gets called.
(Assignee)

Comment 3

11 years ago
Created attachment 214411 [details] [diff] [review]
Patch

This removes the xforms-refresh event on form construction.

I've done some minimal testing of external instances, misc. controls, and dynamically changing attributes and inserting elements. It seems to work for me.

There is still something wrong with out Reval/Refresh setup, and when the SetStates gets triggered by controls, but that should be fixed on the (long awaited) fix to our entire RRR routine, which is bug 300591.
Assignee: aaronr → allan
Attachment #214320 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #214411 - Flags: review?(aaronr)
(Assignee)

Updated

11 years ago
Blocks: 326556

Comment 4

11 years ago
Comment on attachment 214411 [details] [diff] [review]
Patch

its a potentially huge change where I would expect *something* to fail, but Allan is right...everything I tested worked.  The only hole I see is things that are added to the model list that aren't bound to an instance node (and thus on the deferred bind list) may not get refreshed during load now.  Do we have anything anymore that fits this bill?
Attachment #214411 - Flags: review?(aaronr) → review+
(Assignee)

Comment 5

11 years ago
Comment on attachment 214411 [details] [diff] [review]
Patch

(In reply to comment #4)
> (From update of attachment 214411 [details] [diff] [review] [edit])
>The only hole I see is things
> that are added to the model list that aren't bound to an instance node (and
> thus on the deferred bind list) may not get refreshed during load now.  Do we
> have anything anymore that fits this bill?

I haven't found anything, no.
Attachment #214411 - Flags: review?(smaug)

Comment 6

11 years ago
Comment on attachment 214411 [details] [diff] [review]
Patch

I wonder if we'll need that aAllStates at some point. But r=me
Attachment #214411 - Flags: review?(smaug) → review+
(Assignee)

Comment 7

11 years ago
Checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Blocks: 332853
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.3, fixed1.8.1
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
(Assignee)

Comment 8

11 years ago
This regressed on trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

11 years ago
This is probably to blame.
http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsControlStub.cpp#212

I keep getting issues over and over again because it's a bit "transparent" when we are initializing and when we do normal processing. It would be nice with a more explicit process. Repeats also suffer from this.
(Assignee)

Comment 10

11 years ago
Created attachment 219872 [details] [diff] [review]
Fix

Check for mReadyHandled before sending events.

The reason for this problem occuring is that the responsibility for setting the states (and sending the events) was moved from the model to the controls, and I forgot about inhibiting the events on load.
Attachment #219872 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #219872 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

11 years ago
Attachment #219872 - Flags: review?(aaronr)

Updated

11 years ago
Attachment #219872 - Flags: review?(aaronr) → review+
(Assignee)

Comment 11

11 years ago
(In reply to comment #10)
> Created an attachment (id=219872) [edit]

Fixed on trunk.
/cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v  <--  nsXFormsModelElement.cpp
new revision: 1.113; previous revision: 1.112
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.