select1 doesn't suppress value changed event sequence when incremental != true

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: Steve Speicher, Assigned: Steve Speicher)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Windows XP
fixed1.8.0.4, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.55  [en] (IBM EVV/3.8/EAK01AGF/LE)
Build Identifier: 

1.0 Test suite case 4.6.3.b

When incremental != true the value changed events shouldn't be dispatched.

Also, the logic is backwards for the default for incremental (for both select and select1).
'incremental' should be treated as false, unless explicitly set to true or not present.

Reproducible: Always

Steps to Reproduce:
1. Load supplied test cases and observer results




I have a patch for this already, will attach after a little more testing/work.
(Assignee)

Comment 1

12 years ago
Created attachment 215408 [details]
test case shows problem with select1 incremental = false
(Assignee)

Comment 2

12 years ago
Created attachment 215409 [details]
test case that shows incremental = junk

Comment 3

12 years ago
> Also, the logic is backwards for the default for incremental (for both select
> and select1).
> 'incremental' should be treated as false, unless explicitly set to true or not
> present.

Why this way? Why not treating it false only if it is really 'false'. Otherwise 
'true', since it is the default value.
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)

> Why this way? Why not treating it false only if it is really 'false'. Otherwise 
> 'true', since it is the default value.
> 

http://www.w3.org/TR/2006/REC-xforms-20060314/slice4.html#sequence-for-select says (2nd bullet):
  "the form control does not have the incremental="true" setting"

therefore, if not explicity set to "true" (or not set at all), it should only do: 4.6.6 Sequence: Selection Without Value Change

Actually, I can't find anywhere in the spec that says incremental = 'true' is the default.  If I read the 2nd bullet literally, then the default is 'false'.

Comment 5

12 years ago
http://www.w3.org/TR/xforms/slice8.html#ui-selectOne
"incremental

    When true, this form control will generate additional xforms-value-changed events. The default for this form control is true."

Comment 6

12 years ago
Also in the schema:
<xsd:attribute name="incremental" type="xsd:boolean" use="optional" default="true"/>

Updated

12 years ago
Assignee: aaronr → sspeiche
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

12 years ago
Comment on attachment 215409 [details]
test case that shows incremental = junk

since the type is really, xsd:boolean this is invalid.  section 4.6.3 was not clear on that but  section 8 is very clear
Attachment #215409 - Attachment is obsolete: true
(Assignee)

Comment 8

12 years ago
It also appears that the event sequencing is wrong for select1. Should be 'Selection without value change' followed by 'Value change', the 'value change' is happening first.  Will attempt to fix both.
(Assignee)

Comment 9

12 years ago
Created attachment 216757 [details] [diff] [review]
patch, only handles incremental='false'..not sequence of events

Opened bug 332242 for the sequence for select/deselect events before value change
Attachment #216757 - Flags: review?(allan)
(Assignee)

Updated

12 years ago
Attachment #216757 - Flags: review?(doronr)
Comment on attachment 216757 [details] [diff] [review]
patch, only handles incremental='false'..not sequence of events

looks good, I'll let allan deal with the spec concerns if any are left.
Attachment #216757 - Flags: review?(doronr) → review+

Comment 11

12 years ago
Comment on attachment 216757 [details] [diff] [review]
patch, only handles incremental='false'..not sequence of events

>Index: resources/content/select1.xml
>===================================================================
>+      <property name="incremental">

As this is a readonly property, please use readonly="true".

>+        <getter>
>+        <![CDATA[
>+          // default is true
>+          var incremental = true;
>+
>+          if (this.hasAttribute("incremental")) {
>+            if (this.getAttribute("incremental") == "false")
>+              incremental = false;
>+          }
>+
>+          return incremental;

No need to check for the attribute presence first, the get() call will return the empty string if the attribute is not there. So the entire function is just:
return this.getAttribute("incremental") != "false";

With that fixed, r=me
Attachment #216757 - Flags: review?(allan) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 217011 [details] [diff] [review]
patch for checkin, with Allan's comments
Attachment #216757 - Attachment is obsolete: true

Updated

12 years ago
Blocks: 322255

Comment 13

12 years ago
checked in to trunk for sspecihe
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

12 years ago
Blocks: 332853

Updated

12 years ago
Keywords: fixed1.8.0.3, fixed1.8.1

Updated

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