332242 - select1 event sequence backwards: sel/desel then value change

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

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

According to spec section 4.6.3, when select1 incremental=true...xforms-select/deselect should occur before value change events.

Though, the code select1.xml clearly states why _handleSelection() needs to occur before dispatchSelectEvent():

   _handleSelection might deselect this._selected if this user selected item is a copyItem bound to a non-element node.  We don't want to dispatch a xforms-select/deselect in that case since it really isn't considered to be a valid selection to begin with (causes a xforms-binding-exception).

Perhaps a spec issue?

Reproducible: Always



Expected Results:  
Should see xforms-select/xforms-deselect before refresh/revalidate/recalculate

Comment 1

[Steve Speicher]

Attachment: test case

Comment 2

[Allan Beaufour]

(In reply to comment #0)
> According to spec section 4.6.3, when select1
> incremental=true...xforms-select/deselect should occur before value change
> events.

Hmm, yes we do it the wrong way.
 
> Perhaps a spec issue?

I'll leave that to the select wizards for now :)
 
(In reply to comment #1)
> Created an attachment (id=216758) [edit]
> test case

Heh. I like that you use XForms for testing itself! (But it's a bit "dangerous" path to test the tech. with the tech. itself though)

Comment 3

[aaronr]

> Perhaps a spec issue?
> 

Nah, nothing wrong with the spec.  We handle it correctly for selects.  I was too worried about generating the xforms-select and xforms-deselect events accurately that I missed the fact that I set the bound node (and thus generate a value-changed event) before I dispatched the select and deselect events.

Comment 4

[aaronr]

Should be just as simple as moving the event generation into handleSelect before setting the node/value via the accessor.  Best to wait until bug 330825 is in, though, so that we aren't walking on the same code at the same time.  Plus can double check to make sure that the event stuff for select1 is correct by the time both of these are checked in.  Should use both default and incremental="false" testcases.

Comment 5

[aaronr]

we need to reverify this after bug 333619 gets checked in.  I think that patch will fix this problem.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> we need to reverify this after bug 333619 gets checked in.  I think that patch
> will fix this problem.
> 

The bug 333619 fixes issue only for select controls, but this bug is actual for select1[appearance='minimal'].

Comment 7

[aaronr]

Attachment: fix attempt 1

This fixes it and I don't see any regressions in my testcases.  Alex, could you try some of your xul select1 testcases to make sure I didn't regress any of those?  Thanks.

I basically moved the event dispatching inside handleSelection where it made sense to do it there.  Using a flag to tell handleSelection whether to dispatch the events prior to changing the bound node or not.  For example, if non-incremental, we don't want to dispatch select and deselect if we are changing the bound node since those events should have already taken place by the time handleselection is called.

Comment 8

[aaronr]

Attachment: fix attempt 2

added a simple fix to my previous patch.  If you had two select1's bound to the same node they were getting out of sync if one of them changed the value.  Using the wrong variable name but since it was in JS, it didn't complain.  One line change.

Comment 9

[aaronr]

checked it into the trunk yesterday (04/25).  I forgot to mark the bug.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Created an attachment (id=219477) [edit]
> fix attempt 1
> 
> This fixes it and I don't see any regressions in my testcases.  Alex, could you
> try some of your xul select1 testcases to make sure I didn't regress any of
> those?  Thanks.
> 

Sorry, I missed your comment. You did changes only in select1.xml but xul:select1 is based on code for select. I belive xul:select1 works.