Last Comment Bug 332242 - select1 event sequence backwards: sel/desel then value change
: select1 event sequence backwards: sel/desel then value change
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
http://www.w3.org/TR/2006/REC-xforms-...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-30 08:27 PST by Steve Speicher
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
test case (4.02 KB, application/xhtml+xml)
2006-03-30 08:30 PST, Steve Speicher
no flags Details
fix attempt 1 (11.03 KB, patch)
2006-04-22 18:19 PDT, aaronr
no flags Details | Diff | Splinter Review
fix attempt 2 (11.81 KB, patch)
2006-04-24 13:40 PDT, aaronr
doronr: review+
bugs: review+
Details | Diff | Splinter Review

Description Steve Speicher 2006-03-30 08:27:02 PST
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 2006-03-30 08:30:49 PST
Created attachment 216758 [details]
test case
Comment 2 Allan Beaufour 2006-03-31 00:11:28 PST
(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 2006-03-31 17:16:59 PST
> 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 2006-03-31 17:33:30 PST
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 2006-04-12 11:37:57 PDT
we need to reverify this after bug 333619 gets checked in.  I think that patch will fix this problem.
Comment 6 alexander :surkov 2006-04-12 21:33:56 PDT
(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 2006-04-22 18:19:40 PDT
Created attachment 219477 [details] [diff] [review]
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 2006-04-24 13:40:35 PDT
Created attachment 219646 [details] [diff] [review]
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 2006-04-26 17:35:19 PDT
checked it into the trunk yesterday (04/25).  I forgot to mark the bug.
Comment 10 alexander :surkov 2006-04-26 18:56:38 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.