select1 event sequence backwards: sel/desel then value change

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: Steve Speicher, Assigned: aaronr)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

4.02 KB, application/xhtml+xml
Details
11.81 KB, patch
Doron Rosenberg (IBM)
: review+
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 216758 [details]
test case

Comment 2

12 years ago
(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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 3

12 years ago
> 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.

(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

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

Comment 6

12 years ago
(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'].
(Assignee)

Comment 7

12 years ago
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.
Attachment #219477 - Flags: review?(doronr)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #219477 - Flags: review?(doronr)
(Assignee)

Comment 8

12 years ago
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.
Attachment #219477 - Attachment is obsolete: true
Attachment #219646 - Flags: review?(doronr)

Updated

12 years ago
Attachment #219646 - Flags: review?(doronr) → review+
(Assignee)

Updated

12 years ago
Attachment #219646 - Flags: superreview?(Olli.Pettay)
(Assignee)

Updated

12 years ago
Attachment #219646 - Flags: superreview?(Olli.Pettay) → review?(Olli.Pettay)

Updated

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

Comment 9

12 years ago
checked it into the trunk yesterday (04/25).  I forgot to mark the bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 10

12 years ago
(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.

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

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.