Closed Bug 353880 Opened 18 years ago Closed 17 years ago

Enhancements to spinbuttons and non-numeric range control types

Categories

(Core Graveyard :: XForms, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: msterlin, Assigned: msterlin)

References

Details

(Keywords: fixed1.8.0.12, fixed1.8.1.4)

Attachments

(3 files, 12 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; formsPlayer 1.4; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060921 Minefield/3.0a1

The patch for Bug 316353 implements range controls for non-numeric types using a new spinbutton widget. The patch covers the required functionality but a number of enhancements should be done after the basic functionality is checked in:

- The appearance of the spinbuttons could be improved via css or we might be able to wrap the XUL spinbutton implementation and use that instead.
- The spinbuttons do not spin rapidly when the mouse is held down.  The XUL spinbuttons do because they have an attribute 'type=repeat'; nothing similar is available for XHTML.
- The range types do implement error checking but that could be further improved to deal with missing attributes or non-sensical values supplied by the forms author.


Reproducible: Always

Steps to Reproduce:
1. Load any of the testcases attached to bug 316353 and observe the behavior.
Depends on: 316353
Attached file example
Here is an example of what the spin button control looks like now and what we'd like it to eventually look like.  Written entirely in xhtml and xbl so nothing xforms specific about it.
(In reply to comment #1)
> Created an attachment (id=242457) [edit]
> example
> 
> Here is an example of what the spin button control looks like now and what we'd
> like it to eventually look like.  Written entirely in xhtml and xbl so nothing
> xforms specific about it.
> 

schnitz is very kindly taking a look at getting the styling right for us in xhtml.
Blocks: 353738
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=242457) [edit]
> > example
> > 
> > Here is an example of what the spin button control looks like now and what we'd
> > like it to eventually look like.  Written entirely in xhtml and xbl so nothing
> > xforms specific about it.
> > 
> 
> schnitz is very kindly taking a look at getting the styling right for us in
> xhtml.
> 

Heh. Toolkit has already styled spinbuttons. More that spinbuttons use style rule -moz-appearance that means certain OS will draw this widget by itself. It leads if we will style spinbuttons by yourself then we get incompatible styling for OS.
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=242457) [edit]
> > > example
> > > 
> > > Here is an example of what the spin button control looks like now and what we'd
> > > like it to eventually look like.  Written entirely in xhtml and xbl so nothing
> > > xforms specific about it.
> > > 
> > 
> > schnitz is very kindly taking a look at getting the styling right for us in
> > xhtml.
> > 
> 
> Heh. Toolkit has already styled spinbuttons. More that spinbuttons use style
> rule -moz-appearance that means certain OS will draw this widget by itself. It
> leads if we will style spinbuttons by yourself then we get incompatible styling
> for OS.
> 


only on trunk and mostly just for xul.  We need something that will work in 1.8.x (and .xhtml), too.  If we use the same class names and include the trunk chrome css file in the xbl:resource tag in the xbl, then when it is there it will pick up styles from there and when not, it will pick them up from our xforms.css file, right?
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > Created an attachment (id=242457) [edit]
> > > > example
> > > > 
> > > > Here is an example of what the spin button control looks like now and what we'd
> > > > like it to eventually look like.  Written entirely in xhtml and xbl so nothing
> > > > xforms specific about it.
> > > > 
> > > 
> > > schnitz is very kindly taking a look at getting the styling right for us in
> > > xhtml.
> > > 
> > 
> > Heh. Toolkit has already styled spinbuttons. More that spinbuttons use style
> > rule -moz-appearance that means certain OS will draw this widget by itself. It
> > leads if we will style spinbuttons by yourself then we get incompatible styling
> > for OS.
> > 
> 
> 
> only on trunk and mostly just for xul.  We need something that will work in
> 1.8.x (and .xhtml), too.  If we use the same class names and include the trunk
> chrome css file in the xbl:resource tag in the xbl, then when it is there it
> will pick up styles from there and when not, it will pick them up from our
> xforms.css file, right?
> 


nope, I was wrong.  some spin button styling in 1.8.x, too.  Hold off on this schnitz and I'll look closer at it tomorrow.  I might need to update the example.  Too late to do it now.
Attached patch spinbuttons (obsolete) — Splinter Review
Patch includes xul spinbuttons and numberbox and XHTML spinbutton widget that uses them.
Attachment #243500 - Flags: review?(surkov.alexander)
Did you forget to add new files into patch?
Assignee: xforms → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
(In reply to comment #7)
> Did you forget to add new files into patch?
Yes I did.  Will add them and recreate the patch.
Attached patch spinbuttons (obsolete) — Splinter Review
Attachment #243500 - Attachment is obsolete: true
Attachment #243505 - Flags: review?(surkov.alexander)
Attachment #243500 - Flags: review?(surkov.alexander)
Comment on attachment 243505 [details] [diff] [review]
spinbuttons

Aaron, please ensure my requests are going with you.

1) I don't think either of us will like new files. I guess it's better to put spinbuttons and numberbox into widgets.xml (because they are common both for xhtml and xul). and then please specify where you steal these widgets from by comment before binding.
2) I didn't get why do you need #spinbutton binding of widgets-xhtml file. The difference between your #spinbutton and numberbox is only your widget has xul:label and you resend new events. Why?
Attachment #243505 - Flags: review?(surkov.alexander) → review-
(In reply to comment #10)
> (From update of attachment 243505 [details] [diff] [review] [edit])
> > 2) I didn't get why do you need #spinbutton binding of widgets-xhtml file. The difference between your #spinbutton and numberbox is only your widget has
 xul:label and you resend new events. Why?

Exactly because we want to put labels next to the spinbuttons. The other choices would have been to modify the numberbox to include a label or extend the numberbox and add the label. 

In order to handle increment and decrement of the range you need to know which spinbutton was clicked.  The spinbutton with label in widgets-xhtml catches the up/down events but then resends spinup/spindown so you can identify which spinbutton. Extending the numberbox leads to other problems. 

Alex, would you like to take over this bug as well as 316353? If you would try to implement xsd:date, xsd:time, and xsd:dateTime you would then perhaps have a better understanding of all the issues and complications. I cannot explain them all and do not want to keep going back and forth on things that we have already discussed.
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 243505 [details] [diff] [review] [edit] [edit])
> > > 2) I didn't get why do you need #spinbutton binding of widgets-xhtml file. The difference between your #spinbutton and numberbox is only your widget has
>  xul:label and you resend new events. Why?
> 
> The other
> choices would have been to modify the numberbox to include a label or extend
> the numberbox and add the label. 

I don't like idea to extend numberbox too because in future it will be difficult to use toolkit's one.

> Exactly because we want to put labels next to the spinbuttons. 
It's clear with me you like to have labels but it's not clear why we should have additional binding for this. Will it be simplier to keep this in range bindings?

> In order to handle increment and decrement of the range you need to know which
> spinbutton was clicked.  The spinbutton with label in widgets-xhtml catches the
> up/down events but then resends spinup/spindown so you can identify which
> spinbutton.

I assume they will be indentified by range binding. Right? Then why should range binding indentify spinbuttons?

> Alex, would you like to take over this bug as well as 316353? If you would try
> to implement xsd:date, xsd:time, and xsd:dateTime you would then perhaps have a
> better understanding of all the issues and complications.

I just hope you can tell me more than 90kb of code :). Especially because I don't share entirely current approach of bug 316353.

If we will not achive agreement about widget-xhtml spinbuttons then I think we can leave them for bug 316353. But if they are needed then I'd like to have them here.
(In reply to comment #12)
> I just hope you can tell me more than 90kb of code :). Especially because I
> don't share entirely current approach of bug 316353.

You don't like any approach thus far and the reason is because you are not aware of all the issues. Every change requires rearchitecting the whole thing.

I suggest applying this patch for xul type spinbuttons and trying to implement one of the more complex types like xsd:date. You will quickly discover why extending the base range binding and being forced to implement 'getControlElement' is a problem.  You will see that you cannot push down the increment/decrement functionality to the xul spinbuttons because whether or not you can increment one of them depends on the current values of the others. You will see that the 'set' and 'value' methods you inherit from the base range complicate matters because the 'value' has to be assembled from the contents of multiple spinbuttons.

Try xsd:date and keep in mind that xsd:date may also have a time (although we can choose not to support that) which makes it the same as xsd:dateTime. You can struggle with whether or not xsd:date should extend xsd:dateTime or whether there should be separate date and time widgets with dateTime containing one of each (that will complicate event handling even further).

I have rewritten this stuff multiple different ways and am very aware of all the issues.  Until you have tried yourself and are on the same page we will get nowhere. Let me know when you've got a working xsd:date, xsd:time, and xsd:dateTime implementation and we will go from there.
Okey, I guess we can leave widget-xhtml spinbuttons for bug 316353. Here we can add only toolkit's spinbuttons and numberbox. Does it work for you? Also, you didn't modify in any way toolkit's spinbuttons and numberbox. Right?

Btw, if day field of range[date] is 31 and click increment then what's happen? Is it disabled? Or day field value will be is 1, but month field value will be incremented?
(In reply to comment #14)
> Also, you didn't modify in any way toolkit's spinbuttons and numberbox. Right?
I modified the command handler of xforms-spinbuttons. It sends up/down events but then also creates a function onup/ondown and calls it.  The onup/ondown functions change the value in the numberbox. To work with that, we have to catch the up/down events and decide if the value may be incremented. If not, then the handler has to call preventDefault so dispatchEvent returns false.  I added a test for that so that if the event handler returns false, we don't increment/decrement the value. The idea is that the range widget, such as range-date decides in its event handler if the value may be changed; if it can, then we just let the numberbox go ahead and change it.

I didn't modify numberbox yet but it will need changes as well.  The keypress handlers directly call the methods to modify the value.  That will have to be change to fire up/down events because again we need to be able to decide if increment/decrement is allowed.

> Btw, if day field of range[date] is 31 and click increment then what's happen? Is it disabled? Or day field value will be is 1, but month field value will be incremented?
We can set the wraparound property to let the spinbuttons automatically wrap from 31 to 1 or we can choose to disable it.  I think wraparound would be appropriate.  The month will not change - each spinbutton is independent - BUT here is another reason the event handlers need to decide if the numberbox can inc/dec.  If the month is February and the year is a leap year, the day value can be 29; otherwise it must be limited to 28. You see why the 'max' attribute on the spinbutton is not useful here? We can't just set it to 31 because not all months have 31 days and for leap years not only do you have to consider the month but the year also and that is why we need to be able to look at the values of all the spinbuttons in the up/down event handlers.



(In reply to comment #15)
> (In reply to comment #14)
> > Also, you didn't modify in any way toolkit's spinbuttons and numberbox. Right?
> I modified the command handler of xforms-spinbuttons. It sends up/down events
> but then also creates a function onup/ondown and calls it.  The onup/ondown
> functions change the value in the numberbox. To work with that, we have to
> catch the up/down events and decide if the value may be incremented. If not,
> then the handler has to call preventDefault so dispatchEvent returns false.  I
> added a test for that so that if the event handler returns false, we don't
> increment/decrement the value. The idea is that the range widget, such as
> range-date decides in its event handler if the value may be changed; if it can,
> then we just let the numberbox go ahead and change it.

>+        var evt = document.createEvent("Events");
>+        evt.initEvent(eventname, true, true);
>+        var cancel = this.dispatchEvent(evt);
>+        if (cancel) {
>+          if (this.hasAttribute("on" + eventname)) {
>+            var fn = new Function("event", this.getAttribute("on" + eventname));
>+            if (fn.call(this, event) == false)
>+              cancel = true;
>+          }
>+        }

That's not right way of using of preventDefault() method. If someone adds event listeners by attribute with prefix 'on' then this event listener should be called event if default action was prevented. If you don't want this event listeners you should use stopPropagation(). Also, can you explain me the magic 0when _modifyDown/_modifyUp are registered twice, one is by xbl:handler, other is by onup/ondown attributes? Why?

> 
> > Btw, if day field of range[date] is 31 and click increment then what's happen? Is it disabled? Or day field value will be is 1, but month field value will be incremented?
> We can set the wraparound property to let the spinbuttons automatically wrap
> from 31 to 1 or we can choose to disable it.  I think wraparound would be
> appropriate.  The month will not change - each spinbutton is independent - BUT
> here is another reason the event handlers need to decide if the numberbox can
> inc/dec.  If the month is February and the year is a leap year, the day value
> can be 29; otherwise it must be limited to 28. You see why the 'max' attribute
> on the spinbutton is not useful here? We can't just set it to 31 because not
> all months have 31 days and for leap years not only do you have to consider the
> month but the year also and that is why we need to be able to look at the
> values of all the spinbuttons in the up/down event handlers.
> 

My point is we should skip right now support of complex ranges, ie. ranges for datatypes like date, datetime because it's not clear for me how these widgets should look and behave. I think we should find analogues for different OS and/or be advised by somebody, for instance, by neil/enn here. So, do you need your widget-xthml's spinbuttons for "simple" datatypes?
(In reply to comment #16)
> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent(eventname, true, true);
> >+        var cancel = this.dispatchEvent(evt);
> >+        if (cancel) {
> >+          if (this.hasAttribute("on" + eventname)) {
> >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> >+            if (fn.call(this, event) == false)
> >+              cancel = true;
> >+          }
> >+        }
> That's not right way of using of preventDefault() method. If someone adds event
> listeners by attribute with prefix 'on' then this event listener should be
> called event if default action was prevented. If you don't want this event
> listeners you should use stopPropagation(). Also, can you explain me the magic
> 0when _modifyDown/_modifyUp are registered twice, one is by xbl:handler, other
> is by onup/ondown attributes? Why?
> > 
Good question and I have no idea why. It seems redundant to me to send up/down events and then also to call a function onup/ondown but I didn't write the numberbox code.

If the event handlers don't want the value to be incremented/decremented the only way to get dispatchEvent to return false is to use preventDefault; stopPropogation won't do it.

> My point is we should skip right now support of complex ranges, ie. ranges for datatypes like date, datetime because it's not clear for me how these widgets should look and behave. I think we should find analogues for different OS and/or be advised by somebody, for instance, by neil/enn here. So, do you need your widget-xthml's spinbuttons for "simple" datatypes?

That is not an option. We have to implement something for all of these datatypes and after much discussion and 3 or 4 different implementations from me, it is settled that we are using the xul spinbuttons.

Note that other than adding a label to the xul spinbuttons, the reason I have another spinbutton in widgets-xhtml is so that all of the logic is contained in one place.  If we ever decide on a different representation, we just replace the one spinbutton widget and not all 10 widgets for the various types.

I am eagerly awaiting YOUR implementation of xsd:date, xsd:dateTime, and xsd:time.

(In reply to comment #17)

> If the event handlers don't want the value to be incremented/decremented the
> only way to get dispatchEvent to return false is to use preventDefault;
> stopPropogation won't do it.

preventDefault() doesn't cancel any event listener. But you do it.

> > My point is we should skip right now support of complex ranges, ie. ranges for datatypes like date, datetime because it's not clear for me how these widgets should look and behave. I think we should find analogues for different OS and/or be advised by somebody, for instance, by neil/enn here. So, do you need your widget-xthml's spinbuttons for "simple" datatypes?
> 
> That is not an option. We have to implement something for all of these
> datatypes and after much discussion and 3 or 4 different implementations from
> me, it is settled that we are using the xul spinbuttons.

Please let me clear. I don't talk about xul spinbuttons and xul numberbox. I talk about your additional widget-xhtml's 'spinbuttons' binding.

> Note that other than adding a label to the xul spinbuttons, the reason I have
> another spinbutton in widgets-xhtml is so that all of the logic is contained in
> one place.  If we ever decide on a different representation, we just replace
> the one spinbutton widget and not all 10 widgets for the various types.

I'm agree it's nice to have common code. But I don't see a reason why do you need your 'spinbuttons' binding for simple datatypes. Probably it will be useless for complex datatypes, but I'm not sure because I don't know how range widgets for complex datatypes should look.

> I am eagerly awaiting YOUR implementation of xsd:date, xsd:dateTime, and
> xsd:time.
> 

I didn't want to implement complex datatype, but simple it's truth. But if you care we should write common code for complex and simple ranges right now then I'd like to skip your 'spinbuttons' widget in this bug and to save it for following up bug. Also we must decide about presentation of complex datatypes. It is important requirement to have common code.
(In reply to comment #18)
> Also we must decide about presentation of complex datatypes.

It's been decided and the decision is spinbuttons.  Now whether we wrap the xul spinbuttons in another 'spinbutton' in widgets-xhtml as I have done or we just use the xul spinbuttons directly does not matter to me. Go ahead and remove the xhtml spinbutton and use the xul spinbuttons/numberbox to implement one of the range types.

You wanted to attempt one of the new types yourself and I think you should because you need to know first-hand all of the issues.  Once you have one of the complex types like xsd:date implemented to your satisfaction I will take a look at it and make sure it meets the requirements. If it does, I'll do all of the others based on it.  I am not going to keep 'guessing' what you would like to see and submit patch after patch.
(In reply to comment #19)
> (In reply to comment #18)
> > Also we must decide about presentation of complex datatypes.
> 
> It's been decided and the decision is spinbuttons.

How will range for xsd:date look? Three numberboxes? Two numberboxes and one menulist with spinbuttons for months? One numberbox and three spinbuttons? Something else? Until I'm sure how complex ranges should look then I'm not going to do any complex range widget.

>  Now whether we wrap the xul
> spinbuttons in another 'spinbutton' in widgets-xhtml as I have done or we just
> use the xul spinbuttons directly does not matter to me.

Ok. That's fine. I think you should to do the next to fix this bug:
1) move xul spinbuttons and numberbox to widgets.xml
2) fix canceling event listeners if default action was prevented
3) remove widget-xhmtl's spinbuttons binding.

Aaron, please take a look at requirements I did.
(In reply to comment #20)
>> How will range for xsd:date look? Three numberboxes? Two numberboxes and one
> menulist with spinbuttons for months? One numberbox and three spinbuttons?
> Something else? Until I'm sure how complex ranges should look then I'm not
> going to do any complex range widget.

It should be one numberbox with spinbuttons for each component of the date, but if you want to try other representations do so and then you will be 'sure' of what you want.
(In reply to comment #21)
> (In reply to comment #20)
> >> How will range for xsd:date look? Three numberboxes? Two numberboxes and one
> > menulist with spinbuttons for months? One numberbox and three spinbuttons?
> > Something else? Until I'm sure how complex ranges should look then I'm not
> > going to do any complex range widget.
> 
> It should be one numberbox with spinbuttons for each component of the date, but
> if you want to try other representations do so and then you will be 'sure' of
> what you want.
> 

I have not expirence how they should look. Therefore I said we should find analogues for this in different OS and/or be advised by neil/enn or somebody else. Did you see similiar controls?
(In reply to comment #22)
XSmiles implements xsd:date as a calendar control and arbitrarily pops up the calendar in the middle of the range. We discussed using calendars but the calendar control is not sufficient for implementing the needed functionality so we decided on spinbuttons rather than attempting to write an entirely new calendar control. XSmiles does not implement any of the other types, including xsd:dateTime which for all intents and purposes is the same as xsd:date.

None of the other XForms processors implement any of the non-numeric range types.
Attached patch patch (obsolete) — Splinter Review
Spinbuttons and Numberbox in widgets.xml.
Attachment #243505 - Attachment is obsolete: true
Attachment #244249 - Flags: review?(surkov.alexander)
(In reply to comment #23)
> (In reply to comment #22)
> XSmiles implements xsd:date as a calendar control and arbitrarily pops up the
> calendar in the middle of the range. We discussed using calendars but the
> calendar control is not sufficient for implementing the needed functionality so
> we decided on spinbuttons rather than attempting to write an entirely new
> calendar control. XSmiles does not implement any of the other types, including
> xsd:dateTime which for all intents and purposes is the same as xsd:date.
> 
> None of the other XForms processors implement any of the non-numeric range
> types.
> 


I believe that Alex was referring to somewhere on the web, in Java, in Linux, in Windows, etc. that dates and times are displayed using spinbuttons.  Not necessarily another XForms implementation that used it.  I have seen controls in OS/2 and Win 3.1 that did exactly what we are trying to do but I couldn't find any good pictures/examples on the web.  The closest I could find was Neil's XUL Planet work and the date time control in Windows.  But neither of them will handle upper/lower limits as well, I don't think.  And in the end they are basically 3 spin buttons served by one master set of up/down buttons.

I am working with Merle to put together a standalone xbl example of building a date widget using his spinbuttons/numberboxes.  Once I get that done we can discuss whether this is a good looking widget and if the functionality is acceptable to all.  Deciding that will go a long way toward resolving what the event handlers should look like, where they should live, etc. because we will, by then, have a good idea of exactly what the different widgets should/shouldn't do.
Attached patch patch2 (Aaron) (obsolete) — Splinter Review
patch2 includes the non-xul spinbutton in widgets.xml instead of widgets-xhtml.xml. This is a stand-alone spinbutton that uses the xul:textbox[type="number"] and xul:spinbuttons and fires its own spinup/spindown events.
(In reply to comment #20)

> Ok. That's fine. I think you should to do the next to fix this bug:
> 1) move xul spinbuttons and numberbox to widgets.xml

There is unclear stuff with place where new widgets should be hosted. I asked to move them into widgets.xml. I believe the place is fine until we support some other xml language like SVG where these widgets will not be used. So, probably fine place for them is widgets-xul.xml since these widgets are actually xul widgets. Then styles for them can be declared in widgets-xul.css. But til new widgets are defined in widgets.xml then I think styles should be moved to widgets.css. What do you think?
I think it will be fine if you will include binding styles to xforms.css. Do we know proper css selectors at this point, right?
(In reply to comment #25)
> I believe that Alex was referring to somewhere on the web, in Java, in Linux,
> in Windows, etc. that dates and times are displayed using spinbuttons.  Not
> necessarily another XForms implementation that used it.  I have seen controls
> in OS/2 and Win 3.1 that did exactly what we are trying to do but I couldn't
> find any good pictures/examples on the web.  The closest I could find was
> Neil's XUL Planet work and the date time control in Windows.  But neither of
> them will handle upper/lower limits as well, I don't think.  And in the end
> they are basically 3 spin buttons served by one master set of up/down buttons.

You are right absolutely :)

> I am working with Merle to put together a standalone xbl example of building a
> date widget using his spinbuttons/numberboxes.  Once I get that done we can
> discuss whether this is a good looking widget and if the functionality is
> acceptable to all.  Deciding that will go a long way toward resolving what the
> event handlers should look like, where they should live, etc. because we will,
> by then, have a good idea of exactly what the different widgets
> should/shouldn't do.

That's fine. I think we should have separate bug for date widgets (not this and not bug 316353).
(In reply to comment #27)
> (In reply to comment #20)
> 
> > Ok. That's fine. I think you should to do the next to fix this bug:
> > 1) move xul spinbuttons and numberbox to widgets.xml
> 
> There is unclear stuff with place where new widgets should be hosted. I asked
> to move them into widgets.xml. I believe the place is fine until we support
> some other xml language like SVG where these widgets will not be used. So,
> probably fine place for them is widgets-xul.xml since these widgets are
> actually xul widgets. Then styles for them can be declared in widgets-xul.css.
> But til new widgets are defined in widgets.xml then I think styles should be
> moved to widgets.css. What do you think?
> 

I'm fine with the xul-based widgets being kept in widgets-xul.xml, even if they are also used for xhtml-hosted documents.  widgets.xml should probably be only used for base classes that don't have any anonymous content.  I think that the css styles should be kept in widgets-xul.css unless there is some reason that they should be styled differently in xhtml than xul.  I don't see a reason to create a file called widgets.css if we aren't going to allow any anonymous content in widgets.xml.
Comment on attachment 244249 [details] [diff] [review]
patch

Cancel request until following things are not fixed/answered.

(In reply to comment #30)

> I'm fine with the xul-based widgets being kept in widgets-xul.xml, even if they
> are also used for xhtml-hosted documents.  widgets.xml should probably be only
> used for base classes that don't have any anonymous content.  I think that the
> css styles should be kept in widgets-xul.css unless there is some reason that
> they should be styled differently in xhtml than xul.  I don't see a reason to
> create a file called widgets.css if we aren't going to allow any anonymous
> content in widgets.xml.
> 

Shortly, move spinbuttons/numberbox to widgets-xul.xml/widgets-xul.css and give comment before them that these widgets are used both for xul and xhtml. Sorry that I pointed widgets.xml file initially.

(In reply to comment #28)
> I think it will be fine if you will include binding styles to xforms.css. Do we
> know proper css selectors at this point, right?
> 

What about binding style rules for spinbuttons/numberbox?
Attachment #244249 - Flags: review?(surkov.alexander)
(In reply to comment #31)
OK, will move xul spinbuttons/numberbox into widgets-xul. Our xhtml spinbutton that contains the xul spinbutton/numberbox as anonymous content will be moved to widgets-xhtml but done in the patch for 316353 that will use it.

> > I think it will be fine if you will include binding styles to xforms.css. Do we know proper css selectors at this point, right?
Yes, I know the proper css selectors and they will likely go in xforms.css and also be part of the patch for 316353.
(In reply to comment #32)
> (In reply to comment #31)
> OK, will move xul spinbuttons/numberbox into widgets-xul. Our xhtml spinbutton
> that contains the xul spinbutton/numberbox as anonymous content will be moved
> to widgets-xhtml but done in the patch for 316353 that will use it.

Agree, it's better to save xhmlt spinbutton for bug 316353.

> > > I think it will be fine if you will include binding styles to xforms.css. Do we know proper css selectors at this point, right?
> Yes, I know the proper css selectors and they will likely go in xforms.css and
> also be part of the patch for 316353.
> 

I guess we shouldn't wait for bug 316353 to add binding style rules for xul spinbuttons/numberbox. It's right place to keep them here.
(In reply to comment #33)
> I guess we shouldn't wait for bug 316353 to add binding style rules for xul
> spinbuttons/numberbox. It's right place to keep them here.
You're right. I was thinking of the binding rules for the xhtml spinbutton.  They will go with bug 316353, but the rules for the spinbuttons/numberbox themselves will go in this bug.

Attached patch widgets-xul patch (obsolete) — Splinter Review
xul spinbuttons/numberbox in widgets-xul.xml; style in widgets-xul.css; binding rules in xforms.css.
Attachment #244249 - Attachment is obsolete: true
Attachment #244368 - Attachment is obsolete: true
Attachment #245120 - Flags: review?(surkov.alexander)
(In reply to comment #35)
> Created an attachment (id=245120) [edit]
> widgets-xul patch
> 
> xul spinbuttons/numberbox in widgets-xul.xml; style in widgets-xul.css; binding
> rules in xforms.css.
> 

I noticed in this patch that there is a missing html namespace attribute on the widgets-xul.xml root bindings element.

I also noticed that the spinbutton binding isn't in this patch.  I think that we need to have the spinbutton binding in this patch so that we can start making tough decisions like usability and eventing.  Putting in just the numberbox and spinbuttons binding doesn't gain us anything on their own...it is tough to really understand the context that they'll be used in without the spinbutton binding.

I've got a standalone testcase that uses the spinbutton binding to put together an example of what a dateTime widget might look like.  And we can use this example combined with this patch to start making decisions on how our widget should work.  Once we make those decisions (with the help of WierdAl and others, hopefully) and check in the spinbutton, numberbox and spinbuttions bindings, then we'll have a solid basis on which to finish up bug 316353.
Attached file example of a dateTime widget (obsolete) —
Attached patch widgets-xul patch (obsolete) — Splinter Review
Adding spinbutton to widgets-xul.
Attachment #245120 - Attachment is obsolete: true
Attachment #245233 - Flags: review?(surkov.alexander)
Attachment #245120 - Flags: review?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
Last patch was actually the testcase.  This is the real patch.
Attachment #245233 - Attachment is obsolete: true
Attachment #245241 - Flags: review?(surkov.alexander)
Attachment #245233 - Flags: review?(surkov.alexander)
Attached file example of a dateTime widget II (obsolete) —
I'd like to discuss how all of these spinbutton pieces play together by looking at Merle's patch (id=245241) in combination with this dateTime widget.

Issues I noticed right away that we'll have to address:
1) We still need keyup/keydown to generate spinUp/spinDown events (or whatever event the spinbutton binding ends up when it gets the up/down event from the spinbuttons binding.  I'd like to see that eventually in this patch so that all of the spinbutton binding work is done all in one patch.  A seperate patch for this is also fine, as long as it isn't part of a huge patch like 316353's.
2) We need to figure out how to handle edge values.  Like if min="2006-10-15", max="2006-10-16" and value="2005-11-05" and the user clicks the up button for the year spinbutton.  What should happen?  Should:
  a) year doesn't increase since that would set the widget out of range
  b) year does increase but as soon as it does, month and day decrease as necessary to keep things in range (so take on min's value).
  c) year does increase but as soon as it does month decreases as necessary to keep things in range (leaving day as it is)
  d) year increases but month and/or day doesn't change until 'year' loses focus.

The problem with d) is that the up/down spinbuttons don't take focus, so we can't track with 'blur' unless we make them take focus (or at least make the span/div that surrounds the spinbuttons binding take focus even if the buttons don't).

Whatever decision we make here we'll have to also spread to the month, day, hours, minutes, seconds fields, too.

3) Should we keep things like this widget has them where if we hit the min/max we just don't increment the values?  Or should we also gray out the up/down buttons as necessary?  I'd like to see the graying behavior, personally.

4) Should the labels for the different pieces stay to the left or should they sit above, below, to the right of the spinbutton control?
Attachment #245168 - Attachment is obsolete: true
(In reply to comment #40)
> Created an attachment (id=245289) [edit]
> example of a dateTime widget II
> I'd like to discuss how all of these spinbutton pieces play together by looking
> at Merle's patch (id=245241) in combination with this dateTime widget.
The stand-alone dateTime widget is essentially the same as the widget in the original patch for 316353.  Now that we are using new spinbutton, xul:spinbuttons, xul:numberbox widgets that generate events, the dateTime (or any other range widget) need to be modified to simply check if the inc/dec will be allowed and if not return false from the event handler.  If inc/dec is allowed, then the event handler does nothing and the underlying xul:numberbox/spinbuttons will perform the inc/dec of the value.


> Issues I noticed right away that we'll have to address:
> 1) We still need keyup/keydown to generate spinUp/spinDown events (or 
> whatever event the spinbutton binding ends up when it gets the up/down event > from the spinbuttons binding.  I'd like to see that eventually in this patch > so that all of the spinbutton binding work is done all in one patch.  A 
> seperate patch for this is also fine, as long as it isn't part of a huge
> patch like 316353's.
This is one of the things that could be done in this patch but may end up changing, depending on whether we have a separate widget for each range type (I think we need that to work with 'getControlElement') or if we do all of the work in the binding. So either do it in this patch and possibly change it anyway or wait to do it in 316353.


> 2) We need to figure out how to handle edge values.  Like if min="2006-10-15",
> max="2006-10-16" and value="2005-11-05" and the user clicks the up button for
> the year spinbutton. 
Shouldn't be allowed.  The spinbuttons should be initialized to the min value and as you click up and down the event handler makes sure that you cannot change the value to one that is out of range. If we allow direct entry of value into the numberbox then we would also handle events for that and not allow invalid values.

 > 3) Should we keep things like this widget has them where if we hit the min/max
> we just don't increment the values?  Or should we also gray out the up/down
> buttons as necessary?  I'd like to see the graying behavior, personally.
If (in patch 316353) we rework the widgets so that the up/down event handlers simply say yay or nay to inc/dec then we can get the disable functionality for free from the numberbox - BUT only if you change the min/max attributes on the numberbox on the fly. You cannot simply initialize the min and max attributes for the numberbox with the 'end' range values because number of days in the month depends on the month and also the year if it is a leap year. The up/down event handlers may have to update the min/max attributes on the numberbox so that the numberbox can handle disabling when the incrementing the value would put it out of range.

(In reply to comment #41)
> (In reply to comment #40)
> > Created an attachment (id=245289) [edit]
> > example of a dateTime widget II
> > I'd like to discuss how all of these spinbutton pieces play together by looking
> > at Merle's patch (id=245241) in combination with this dateTime widget.
> The stand-alone dateTime widget is essentially the same as the widget in the
> original patch for 316353.  Now that we are using new spinbutton,
> xul:spinbuttons, xul:numberbox widgets that generate events, the dateTime (or
> any other range widget) need to be modified to simply check if the inc/dec will
> be allowed and if not return false from the event handler.  If inc/dec is
> allowed, then the event handler does nothing and the underlying
> xul:numberbox/spinbuttons will perform the inc/dec of the value.
> 
> 
> > Issues I noticed right away that we'll have to address:
> > 1) We still need keyup/keydown to generate spinUp/spinDown events (or 
> > whatever event the spinbutton binding ends up when it gets the up/down event > from the spinbuttons binding.  I'd like to see that eventually in this patch > so that all of the spinbutton binding work is done all in one patch.  A 
> > seperate patch for this is also fine, as long as it isn't part of a huge
> > patch like 316353's.
> This is one of the things that could be done in this patch but may end up
> changing, depending on whether we have a separate widget for each range type (I
> think we need that to work with 'getControlElement') or if we do all of the
> work in the binding. So either do it in this patch and possibly change it
> anyway or wait to do it in 316353.
> 

can you give an example of why/where this could change?

> 
> > 2) We need to figure out how to handle edge values.  Like if min="2006-10-15",
> > max="2006-10-16" and value="2005-11-05" and the user clicks the up button for
> > the year spinbutton. 
> Shouldn't be allowed.  The spinbuttons should be initialized to the min value

I don't understand.  The initial value of the spin buttons will be set to the value of the widget, right?  In my example, that would be 2005-11-05.

> and as you click up and down the event handler makes sure that you cannot
> change the value to one that is out of range. If we allow direct entry of value
> into the numberbox then we would also handle events for that and not allow
> invalid values.
> 

So you are saying we wouldn't allow the user to click 'up' on the year spinbutton, even though the year here is 2005 and the max is 2006, right?  So if the user wanted to adjust the year up to 2006, he'd have to first change the month down to October or below.  I'm fine with that as long as the year's up button is grayed out.

>  > 3) Should we keep things like this widget has them where if we hit the
> min/max
> > we just don't increment the values?  Or should we also gray out the up/down
> > buttons as necessary?  I'd like to see the graying behavior, personally.
> If (in patch 316353) we rework the widgets so that the up/down event handlers
> simply say yay or nay to inc/dec then we can get the disable functionality for
> free from the numberbox - BUT only if you change the min/max attributes on the
> numberbox on the fly. You cannot simply initialize the min and max attributes
> for the numberbox with the 'end' range values because number of days in the
> month depends on the month and also the year if it is a leap year. The up/down
> event handlers may have to update the min/max attributes on the numberbox so
> that the numberbox can handle disabling when the incrementing the value would
> put it out of range.

Right, we'd need to set the min/max on the fly.  I'm ok with that.
> 

Comment on attachment 245241 [details] [diff] [review]
patch

New widget is not clear for me because I can't keep right now in my mind how exactly it will be used. But if you both want to keep it here, that's fine with me. Often I used this approach too.

>+  <!-- SPINBUTTON -->
>+  <binding id="spinbutton">

It's not spinbutton actually. It's rather spin control.

>+    <resources>
>+      <stylesheet src="chrome://xforms/skin/widgets-xul.css"/>
>+    </resources>

What styles are used?

>+    <content>
>+      <xul:hbox>

What role does hbox play here?

>+        <xul:textbox type="number" size="1" anonid="spin-text"
>+                     xbl:inherits="disabled,min,max,value"/>

Who wins? Our binding or toolkits' one?

>+      <constructor>
>+        // The up event is generated by the spinbuttons widget defined in
>+        // "widgets-xul.xml".
>+        var upHandler = {
>+          spinbutton: this,
>+          handleEvent: function(aEvent) {
>+            var evt = document.createEvent("Events");
>+            evt.initEvent("spinup", true, true);
>+            var cancel = !this.spinbutton.dispatchEvent(evt);
>+            if (cancel) {
>+              aEvent.preventDefault();

What's default action of "up" event. What do actually preventDefault() do?

>+        this.addEventListener("up", upHandler, false);
>+        this.addEventListener("down", downHandler, false);

I assume <xbl:handler/> doesn't work here, right?
(In reply to comment #42)
> > > 2) We need to figure out how to handle edge values.  Like if min="2006-10-15", max="2006-10-16" and value="2005-11-05" and the user clicks the up button for the year spinbutton. 
> > Shouldn't be allowed.  The spinbuttons should be initialized to the min > > value
> I don't understand.  The initial value of the spin buttons will be set to the
> value of the widget, right?  In my example, that would be 2005-11-05.
'Value' is not an attribute of a range, although I guess we could allow users to set value directly. If we do then we have more issues to deal with like making sure that value is a proper date (correct days for month and year) and falls between min and max.

> So you are saying we wouldn't allow the user to click 'up' on the year
> spinbutton, even though the year here is 2005 and the max is 2006, right?  So
> if the user wanted to adjust the year up to 2006, he'd have to first change the month down to October or below.  I'm fine with that as long as the year's up button is grayed out.
Either that or we could automatically adjust he month and day just as I currently do if the day is say 30 and you change the month to February - it automatically gets reset to 28 or 29 depending on whether it's a leap year or not.
(In reply to comment #43)
> (From update of attachment 245241 [details] [diff] [review] [edit])
> New widget is not clear for me because I can't keep right now in my mind how
> exactly it will be used. But if you both want to keep it here, that's fine with me. 
It will be used to display a date or one of the other new range types. What is so confusing about that?

> >+  <!-- SPINBUTTON -->
> >+  <binding id="spinbutton">
> It's not spinbutton actually. It's rather spin control.
We can call it whatever you want.  'Spinbutton' is a generic UI term that means a textfield with up and down buttons to increment or decrement the value of the textfield.  The XUL controls however split it into two components - the spinbuttons which are just the buttons and a separate textbox.

> >+    <resources>
> >+      <stylesheet src="chrome://xforms/skin/widgets-xul.css"/>
> >+    </resources>
> What styles are used?
None at the moment but if we need extra styling the rules would go in widgets-xul.css just like all the others. Since the spinbutton just contains the xul:spinbuttons and xul:numberbox it gets all the style it needs from the css rules for those widgets.

> >+    <content>
> >+      <xul:hbox>
> What role does hbox play here?
> >+        <xul:textbox type="number" size="1" anonid="spin-text"
> >+                     xbl:inherits="disabled,min,max,value"/>
The hbox was there originally because we also had a label preceding the numberbox and spinbuttons and wanted to keep them horizontally aligned.  It's not strictly necessary now that we have removed the label.

> Who wins? Our binding or toolkits' one?
Ours - the binding rule is in xforms.css. Obviously if you use the widget stand-alone you have to write the appropriate binding rule to use ours vs the toolkit.

> >+      <constructor>
> >+        // The up event is generated by the spinbuttons widget defined in
> >+        // "widgets-xul.xml".
> >+        var upHandler = {
> >+          spinbutton: this,
> >+          handleEvent: function(aEvent) {
> >+            var evt = document.createEvent("Events");
> >+            evt.initEvent("spinup", true, true);
> >+            var cancel = !this.spinbutton.dispatchEvent(evt);
> >+            if (cancel) {
> >+              aEvent.preventDefault();
> What's default action of "up" event. What do actually preventDefault() do?
> >+        this.addEventListener("up", upHandler, false);
> >+        this.addEventListener("down", downHandler, false);
> I assume <xbl:handler/> doesn't work here, right?

preventDefault is poorly named. It's sole purpose is to cause dispatchEvent to return false. As you know events travel down the tree to a target and then bubble up and there can be event listeners anywhere. If one handler handles an event from another widget/control and doesn't want the other control to continue with processing it calls preventDefault so the return value from dispatchEvent is false. We need to do that so that if our handler for 'up' does not want the underlying numberbox to change the value, it won't if it checks the return from dispatchEvent and finds it to be false.

For other events that the browser supports directly like onclick, the browser has a default event. If a handler handles onclick on a hyperlink but calls preventDefault, then the browser won't do the 'default' action of following that link.  In our case, the default event would be to modify the value of the numberbox and we call preventDefault when we don't want that to happen.

xbl:handler could have been used as well.
(In reply to comment #44)
> (In reply to comment #42)
> > > > 2) We need to figure out how to handle edge values.  Like if min="2006-10-15", max="2006-10-16" and value="2005-11-05" and the user clicks the up button for the year spinbutton. 
> > > Shouldn't be allowed.  The spinbuttons should be initialized to the min > > value
> > I don't understand.  The initial value of the spin buttons will be set to the
> > value of the widget, right?  In my example, that would be 2005-11-05.
> 'Value' is not an attribute of a range, although I guess we could allow users
> to set value directly. If we do then we have more issues to deal with like
> making sure that value is a proper date (correct days for month and year) and
> falls between min and max.
> 

But value is an attribute of the widget, right?  If we put all of the min/max/value, etc. handling in the widget, then we just have to set those attributes from the range and then none of that work needs to be handled in the range.

> > So you are saying we wouldn't allow the user to click 'up' on the year
> > spinbutton, even though the year here is 2005 and the max is 2006, right?  So
> > if the user wanted to adjust the year up to 2006, he'd have to first change the month down to October or below.  I'm fine with that as long as the year's up button is grayed out.
> Either that or we could automatically adjust he month and day just as I
> currently do if the day is say 30 and you change the month to February - it
> automatically gets reset to 28 or 29 depending on whether it's a leap year or
> not.
> 

Those are the two choices as I see them.  And I think I prefer graying the buttons to not allow the user to go out of range.  Having the control automatically alter values underneath the user is 'icky'.  In the case where the user is going from a month with 31 days to a month with 28 days, for example, I'm guessing that we shouldn't automatically alter the day to be between 1-28, either,like we currently have it.  Until the month field loses focus, I mean.  So that way if I am the user and currently have 01/31 selected and then go to spin up to 03/31, it shouldn't reset to 28 as I jump from 01 to 02 to 03 in the month field.  Unless the month field loses focus while it is set to 02.
(In reply to comment #45)
> (In reply to comment #43)
> > (From update of attachment 245241 [details] [diff] [review] [edit] [edit])

> > What's default action of "up" event. What do actually preventDefault() do?

> We need to do that so that if our handler for 'up' does
> not want the underlying numberbox to change the value, it won't if it checks
> the return from dispatchEvent and finds it to be false.
> 

Specifically in our case, if the dateTime widget changes the value of the spin textfield directly during the handling of the 'spinup' event, then we want to be able to call 'preventDefault()' so that the default handling of the 'up' event by the numberbox doesn't happen (since it would also adjust the value of the textfield).
(In reply to comment #45)
> (In reply to comment #43)
> > (From update of attachment 245241 [details] [diff] [review] [edit] [edit])
> > New widget is not clear for me because I can't keep right now in my mind how
> > exactly it will be used. But if you both want to keep it here, that's fine with me. 
> It will be used to display a date or one of the other new range types. What is
> so confusing about that?

In general I see where it can be used. But I can't talk precisely about its implementation.

> 
> > >+  <!-- SPINBUTTON -->
> > >+  <binding id="spinbutton">
> > It's not spinbutton actually. It's rather spin control.
> We can call it whatever you want.  'Spinbutton' is a generic UI term that means
> a textfield with up and down buttons to increment or decrement the value of the
> textfield.  The XUL controls however split it into two components - the
> spinbuttons which are just the buttons and a separate textbox.

I just didn't see this control is called as spinbuttons. For example, windows calls its like spin box. So, spin box will be fine with me.

> > >+    <resources>
> > >+      <stylesheet src="chrome://xforms/skin/widgets-xul.css"/>
> > >+    </resources>
> > What styles are used?
> None at the moment but if we need extra styling the rules would go in
> widgets-xul.css just like all the others. Since the spinbutton just contains
> the xul:spinbuttons and xul:numberbox it gets all the style it needs from the
> css rules for those widgets.

So, if it isn't used then I prefer to exclude this from the patch. If later we will need this then we always can add.

> > >+    <content>
> > >+      <xul:hbox>
> > What role does hbox play here?
> > >+        <xul:textbox type="number" size="1" anonid="spin-text"
> > >+                     xbl:inherits="disabled,min,max,value"/>
> The hbox was there originally because we also had a label preceding the
> numberbox and spinbuttons and wanted to keep them horizontally aligned.  It's
> not strictly necessary now that we have removed the label.

Please remove it too if it isn't needed.

> > Who wins? Our binding or toolkits' one?
> Ours - the binding rule is in xforms.css. Obviously if you use the widget
> stand-alone you have to write the appropriate binding rule to use ours vs the
> toolkit.

I'm about it's better to attach binding by other rule. What does Aaron think?

> xbl:handler could have been used as well.
> 

It xbl:handler can be used then please use it :)
>+      <handler event="up" preventdefault="true">
>+        this._modifyUp();
>+      </handler>
>+
>+      <handler event="down" preventdefault="true">
>+        this._modifyDown();
>+      </handler>

I talked with Neil, he said this code is not called per bug 350334.

>+        var evt = document.createEvent("Events");
>+        evt.initEvent(eventname, true, true);
>+        var cancel = this.dispatchEvent(evt);
>+        if (cancel) {
>+          if (this.hasAttribute("on" + eventname)) {
>+            var fn = new Function("event", this.getAttribute("on" + eventname));
>+            if (fn.call(this, event) == false)
>+              cancel = true;
>+          }
>+        }
>+        else {
>+          // The up/down event handler called preventDefault so stop further
>+          // propapgation of the up/down event.
>+          evt.stopPropagation();
>+        }

I still cannot pass this magic :). I think it's not beautiful to stop event listeners if event default action have been prevented. Probably we should summon someone else for this.
(In reply to comment #49)
> >+      <handler event="up" preventdefault="true">
> >+        this._modifyUp();
> >+      </handler>
> >+
> >+      <handler event="down" preventdefault="true">
> >+        this._modifyDown();
> >+      </handler>
> I talked with Neil, he said this code is not called per bug 350334.
That is in the numberbox and I am aware that it is not called. If it did get called it would require modification to check the return from dispatchEvent.

> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent(eventname, true, true);
> >+        var cancel = this.dispatchEvent(evt);
> >+        if (cancel) {
> >+          if (this.hasAttribute("on" + eventname)) {
> >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> >+            if (fn.call(this, event) == false)
> >+              cancel = true;
> >+          }
> >+        }
> >+        else {
> >+          // The up/down event handler called preventDefault so stop further
> >+          // propapgation of the up/down event.
> >+          evt.stopPropagation();
> >+        }
> I still cannot pass this magic :). I think it's not beautiful to stop event
> listeners if event default action have been prevented. Probably we should
> summon someone else for this.

Look back to your previous comments where you were adamant that stopPropagation be called; now you don't like stopPropagation being called?

stopPropogation has nothing to do with preventDefault. If preventDefault was called 'cancel' will be false; ie dispatchEvent returned false. We want to check the return from dispatchEvent so that we do NOT call the parent node's onup/ondown method because that will inc/dec the value. stopPropagation will prevent any further up/down handlers from being called (although there are none but someone could add one later that doesn't call preventDefault when necessary).


(In reply to comment #48)
> (In reply to comment #45)
> > (In reply to comment #43)
> > > (From update of attachment 245241 [details] [diff] [review] [edit] [edit] [edit])
> > > New widget is not clear for me because I can't keep right now in my mind how
> > > exactly it will be used. But if you both want to keep it here, that's fine with me. 
> > It will be used to display a date or one of the other new range types. What is
> > so confusing about that?
> 
> In general I see where it can be used. But I can't talk precisely about its
> implementation.

I'm not worried about the implementation for the widget (i.e. which code lives where, what the code that makes it work looks like).  I want everyone to apply this patch and try out the widget.  We need to reach agreement on how it behaves, since our range widget should behave just like it.  And many of the behaviors will carry over to other spinbox-based widgets so I'd like to reach agreements here before we invest much more work in the other widgets.  If someone has a problem with how this widget looks and behaves, then we need to note it here to make sure that the issue doesn't carry over to our future widgets.

> 
> > 
> > > >+  <!-- SPINBUTTON -->
> > > >+  <binding id="spinbutton">
> > > It's not spinbutton actually. It's rather spin control.
> > We can call it whatever you want.  'Spinbutton' is a generic UI term that means
> > a textfield with up and down buttons to increment or decrement the value of the
> > textfield.  The XUL controls however split it into two components - the
> > spinbuttons which are just the buttons and a separate textbox.
> 
> I just didn't see this control is called as spinbuttons. For example, windows
> calls its like spin box. So, spin box will be fine with me.
> 

spin box is fine with me.

> > >+    <content>
> > >+      <xul:hbox>
> > What role does hbox play here?
> > >+        <xul:textbox type="number" size="1" anonid="spin-text"
> > >+                     xbl:inherits="disabled,min,max,value"/>
>> The hbox was there originally because we also had a label preceding the
>> numberbox and spinbuttons and wanted to keep them horizontally aligned.  It's
>> not strictly necessary now that we have removed the label.

>Please remove it too if it isn't needed.

Are you sure we don't need it?  What if someone wants to surround the widget in a xul:vbox?  Wouldn't that cause the input to be placed on top of the spinbuttons?  I'd think we'd need the hbox to make sure that didn't happen.  But if that can't happen, then the hbox should be removed.

> > > Who wins? Our binding or toolkits' one?
> > Ours - the binding rule is in xforms.css. Obviously if you use the widget
> > stand-alone you have to write the appropriate binding rule to use ours vs the
> > toolkit.
> 
> I'm about it's better to attach binding by other rule. What does Aaron think?

We have to key off of a xul:textbox to get the nice blue surrounding border.  I don't care if we change the type to something else.
(In reply to comment #50)
> (In reply to comment #49)
> > >+      <handler event="up" preventdefault="true">
> > >+        this._modifyUp();
> > >+      </handler>
> > >+
> > >+      <handler event="down" preventdefault="true">
> > >+        this._modifyDown();
> > >+      </handler>
> > I talked with Neil, he said this code is not called per bug 350334.
> That is in the numberbox and I am aware that it is not called. If it did get
> called it would require modification to check the return from dispatchEvent.

I believe you don't need any additional check the return from dispatchEvent. This handlers of numberbox is bug I guess. It works until bug 350334 is fixed. So I think you just should remove this.

> > >+        var evt = document.createEvent("Events");
> > >+        evt.initEvent(eventname, true, true);
> > >+        var cancel = this.dispatchEvent(evt);
> > >+        if (cancel) {
> > >+          if (this.hasAttribute("on" + eventname)) {
> > >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> > >+            if (fn.call(this, event) == false)
> > >+              cancel = true;
> > >+          }
> > >+        }
> > >+        else {
> > >+          // The up/down event handler called preventDefault so stop further
> > >+          // propapgation of the up/down event.
> > >+          evt.stopPropagation();
> > >+        }
> > I still cannot pass this magic :). I think it's not beautiful to stop event
> > listeners if event default action have been prevented. Probably we should
> > summon someone else for this.
> 
> Look back to your previous comments where you were adamant that stopPropagation
> be called; now you don't like stopPropagation being called?

Also I said preventDefault() doesn't cancel any event listener. But you do it. I haven't in view that stopPropogation is fine here. Sorry, I should be more clear. Why do you want to stop event propogation? At least stoping of event propogation is strange default action of event.

Why do you need in onup/ondown attributes? Probably it's better to register real event handlers?
(In reply to comment #51)
> (In reply to comment #48)
> > (In reply to comment #45)

> >Please remove it too if it isn't needed.
> 
> Are you sure we don't need it?  What if someone wants to surround the widget in
> a xul:vbox?  Wouldn't that cause the input to be placed on top of the
> spinbuttons?  I'd think we'd need the hbox to make sure that didn't happen. 
> But if that can't happen, then the hbox should be removed.

Right now, xul:hbox has one element is xul:textbox and we don't need additional hierarchy.
(In reply to comment #53)
> (In reply to comment #51)
> > (In reply to comment #48)
> > > (In reply to comment #45)
> 
> > >Please remove it too if it isn't needed.
> > 
> > Are you sure we don't need it?  What if someone wants to surround the widget in
> > a xul:vbox?  Wouldn't that cause the input to be placed on top of the
> > spinbuttons?  I'd think we'd need the hbox to make sure that didn't happen. 
> > But if that can't happen, then the hbox should be removed.
> 
> Right now, xul:hbox has one element is xul:textbox and we don't need additional
> hierarchy.
> 

But remember xul:textbox type="number" has anonymous content...it contains an html:input and the spinbuttons.  So I assumed that having the hbox surrounding the textbox was to ensure that the html:input and the spinbuttons were on the same line.  If the textbox was not contained in the hbox and someone wrapped the control in a vbox, would the html:input and the spinbuttons stay on the same line?
(In reply to comment #52)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > >+      <handler event="up" preventdefault="true">
> > > >+        this._modifyUp();
> > > >+      </handler>
> > > >+
> > > >+      <handler event="down" preventdefault="true">
> > > >+        this._modifyDown();
> > > >+      </handler>
> > > I talked with Neil, he said this code is not called per bug 350334.
> > That is in the numberbox and I am aware that it is not called. If it did get
> > called it would require modification to check the return from dispatchEvent.
> 
> I believe you don't need any additional check the return from dispatchEvent.
> This handlers of numberbox is bug I guess. It works until bug 350334 is fixed.
> So I think you just should remove this.
> 
> > > >+        var evt = document.createEvent("Events");
> > > >+        evt.initEvent(eventname, true, true);
> > > >+        var cancel = this.dispatchEvent(evt);
> > > >+        if (cancel) {
> > > >+          if (this.hasAttribute("on" + eventname)) {
> > > >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> > > >+            if (fn.call(this, event) == false)
> > > >+              cancel = true;
> > > >+          }
> > > >+        }
> > > >+        else {
> > > >+          // The up/down event handler called preventDefault so stop further
> > > >+          // propapgation of the up/down event.
> > > >+          evt.stopPropagation();
> > > >+        }
> > > I still cannot pass this magic :). I think it's not beautiful to stop event
> > > listeners if event default action have been prevented. Probably we should
> > > summon someone else for this.
> > 
> > Look back to your previous comments where you were adamant that stopPropagation
> > be called; now you don't like stopPropagation being called?
> 
> Also I said preventDefault() doesn't cancel any event listener. But you do it.
> I haven't in view that stopPropogation is fine here. Sorry, I should be more
> clear. Why do you want to stop event propogation? At least stoping of event
> propogation is strange default action of event.

stopping of the event propogation is not the default action.  Stopping the propagation only happens here if someone called preventDefault.  I do think that 'cancel' is not a good variable name here.

I think the idea of stopping the propagation is to prevent any handlers up the chain from firing.  We are already saying 'preventDefault' because we don't want the logic in 'numberbox' to happen if we are modifying the spin box value.  So by saying 'stopPropagation' we are saying that we also don't want anyone else messing with the spin boxes.

> 
> Why do you need in onup/ondown attributes? Probably it's better to register
> real event handlers?
> 
I also agree that 'cancel' is confusing as a variable name because of the negative logic. If dispatchEvent returns true, then 'if (cancel)' is confusing because you do NOT want to cancel the call to the function onup/ondown. But hey, I didn't write the numberbox code and didn't want to change it.

Ideally it should be: var cancel = !dispatchEvent(...); if (!cancel) { call function onup/ondown} else { stopPropagation }

Alex, you keep saying 'preventDefault doesn't stop propagation, but you do it'. I've explained over and over the difference between preventDefault and stopPropagation and how the two are not related and why we need to call preventDefault. Aaron's test case is a good example of how it needs to work although we would change the evet handlers in the widget to simply make the decision as to whether inc/dec is allowed and let the numberbox do the inc/dec itself instead of having the widget directly modify the textbox.
> Why do you need in onup/ondown attributes? Probably it's better to register
> real event handlers?
> 

That is how the numberbox code was originally done.  We didn't change that.  I guess we can ask Neil why he did it this way.  The onup and ondown functions only get called if the default handler wasn't prevented and will get called, if I am right, after the default handler for "up" and "down" runs.  I don't know how you'd do that with a handler unless you fired some other event.

But again, we can ask Neil if you want to see why he did it this way.
updated example so that it uses 'preventDefault' only when it DOESN'T want the value to change (for example, when the value is on a min/max boundary), otherwise the value will change by the default handling in numberbox.  Replaces the previous behavior I had where the widget changed the value itself and then called preventDefault.
Attachment #245289 - Attachment is obsolete: true
(In reply to comment #55)
> (In reply to comment #52)
> > (In reply to comment #50)
> > 
> > > > >+        var evt = document.createEvent("Events");
> > > > >+        evt.initEvent(eventname, true, true);
> > > > >+        var cancel = this.dispatchEvent(evt);
> > > > >+        if (cancel) {
> > > > >+          if (this.hasAttribute("on" + eventname)) {
> > > > >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> > > > >+            if (fn.call(this, event) == false)
> > > > >+              cancel = true;
> > > > >+          }
> > > > >+        }
> > > > >+        else {
> > > > >+          // The up/down event handler called preventDefault so stop further
> > > > >+          // propapgation of the up/down event.
> > > > >+          evt.stopPropagation();

Please note, evt.stopPropogation() will never work here.

> stopping of the event propogation is not the default action. 

Right. Non stopping of the event propogation is the default action :). In any way it's not fine with me.
(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #51)
> > > (In reply to comment #48)
> > > > (In reply to comment #45)
> > 
> > > >Please remove it too if it isn't needed.
> > > 
> > > Are you sure we don't need it?  What if someone wants to surround the widget in
> > > a xul:vbox?  Wouldn't that cause the input to be placed on top of the
> > > spinbuttons?  I'd think we'd need the hbox to make sure that didn't happen. 
> > > But if that can't happen, then the hbox should be removed.
> > 
> > Right now, xul:hbox has one element is xul:textbox and we don't need additional
> > hierarchy.
> > 
> 
> But remember xul:textbox type="number" has anonymous content...it contains an
> html:input and the spinbuttons.  So I assumed that having the hbox surrounding
> the textbox was to ensure that the html:input and the spinbuttons were on the
> same line.

No, hbox doesn't play any role here. Textbox guarantee that html:input and spinbuttons will be on the same line.

>  If the textbox was not contained in the hbox and someone wrapped
> the control in a vbox, would the html:input and the spinbuttons stay on the
> same line?
> 

Exactly. They will be on the same line until you textbox has vertical orientation.
(In reply to comment #59)
> > > > > >+          evt.stopPropagation();
> 
> Please note, evt.stopPropogation() will never work here.
> 
> > stopping of the event propogation is not the default action. 
> 
> Right. Non stopping of the event propogation is the default action :). In any
> way it's not fine with me.
> 

Merle, please do new patch with those changes you are agree with. I'll say my comments and ask Neil for sr. I have not any problems with code til Neil is agree with you.
(In reply to comment #57)
> > Why do you need in onup/ondown attributes? Probably it's better to register
> > real event handlers?
> > 
> 
> That is how the numberbox code was originally done.  We didn't change that.  I
> guess we can ask Neil why he did it this way.  The onup and ondown functions
> only get called if the default handler wasn't prevented and will get called, if
> I am right, after the default handler for "up" and "down" runs.  I don't know
> how you'd do that with a handler unless you fired some other event.
> 
> But again, we can ask Neil if you want to see why he did it this way.
> 

Neil or Enn? :) Let ask, I'm not against this. Though I prefer to don't use onup/ondown attributes. It's nicer to use "normal" event listeners.
(In reply to comment #57)
> > Why do you need in onup/ondown attributes? Probably it's better to register
> > real event handlers?
> > 
> The onup and ondown functions
> only get called if the default handler wasn't prevented and will get called, if
> I am right, after the default handler for "up" and "down" runs.  I don't know
> how you'd do that with a handler unless you fired some other event.
> 

I can't still see a reason why if user click spinbutton then numberbox value can be not changed. My point is if numberbox value can't be increased/decreased then user shouldn't be able to click.
(In reply to comment #63)
> I can't still see a reason why if user click spinbutton then numberbox value
> can be not changed. My point is if numberbox value can't be
> increased/decreased then user shouldn't be able to click.

The only time the use cannot click on a spinbutton to change a value is when doing so would put the value out of range; eg. if the value of the year is 2005 and the end date is also 2005, then the year spinbutton up button will be disabled because the year cannot go beyond 2005.

Now that is a totally different issue than 'preventDefault'. The numberbox has onup/ondown attributes and the spinbutton creates functions to call the methods defined in those attributes.  Those methdods increment/decrement the value in the numberbox. I don't like that approach either, but the up/down event handlers in the numberbox don't get called as was pointed out in a prior comment. So we need to leave the onup/ondown attribute functionality for now...BUT there are instances where we don't want the numberbox to inc/dec the value and that is why our spinbox handlers will call preventDefault to prevent the numberbox from changing the value.

In the original patch for 316353 the range widgets change the values directly and that is also the way aaron's test case works. Now that we are using the xul textbox and spinbuttons we want to use their functionality and not modify the values directly. The up/down event handlers make the decision as to whether or not the textbox should change the value or not. This would be clear if you actually had tried to implement this feature or had studied the actual code instead of just looking at a diff out of context.

Attached patch patch (obsolete) — Splinter Review
Renaming spinbutton to spinbox; add firing of up/down events on keypress; removing hbox from spinbox content.
Attachment #245241 - Attachment is obsolete: true
Attachment #245665 - Flags: review?(surkov.alexander)
Attachment #245241 - Flags: review?(surkov.alexander)
(In reply to comment #64)
> (In reply to comment #63)
> > I can't still see a reason why if user click spinbutton then numberbox value
> > can be not changed. My point is if numberbox value can't be
> > increased/decreased then user shouldn't be able to click.
> 
> The only time the use cannot click on a spinbutton to change a value is when
> doing so would put the value out of range; eg. if the value of the year is 2005
> and the end date is also 2005, then the year spinbutton up button will be
> disabled because the year cannot go beyond 2005.
> 
> Now that is a totally different issue than 'preventDefault'. The numberbox has
> onup/ondown attributes and the spinbutton creates functions to call the methods
> defined in those attributes.  Those methdods increment/decrement the value in
> the numberbox. I don't like that approach either, but the up/down event
> handlers in the numberbox don't get called as was pointed out in a prior
> comment. So we need to leave the onup/ondown attribute functionality for
> now...BUT there are instances where we don't want the numberbox to inc/dec the
> value and that is why our spinbox handlers will call preventDefault to prevent
> the numberbox from changing the value.
> 

I think what Alex is saying is that we should never have to call preventDefault to prevent the numberbox from changing the value since the user should never be able click to change the value in the spinbox when we don't want them changing the value.  For example, if we grayed out the up/down button every time to prevent the user from going out of range, then we wouldn't have to call preventDefault.

I think we should gray the up/down buttons as necessary to not allow the user to get us in a situation where we need to call preventDefault.  However, I also think since this is a real widget that can be used independently of our widgets, then it should be able to honor preventDefault and not automatically change the numberbox value if someone does call preventDefault.

> In the original patch for 316353 the range widgets change the values directly
> and that is also the way aaron's test case works. Now that we are using the xul
> textbox and spinbuttons we want to use their functionality and not modify the
> values directly. The up/down event handlers make the decision as to whether or
> not the textbox should change the value or not.

I updated my example yesterday to take this approach.  It no longer updates the spin box value directly (except the 'day' field when the user changes the month such that if the new month doesn't contain 31 days, I'll still change the date field from 31 to a value that is valid for the new month).
In order to have the spinbuttons disabled we need to change the max attribute on the fly. For example if the the user is allowed to increment the month and it changes to February, the handler will have to look at the year value and if it is a leap year set the max attribute for day to 29 and possibly change the day value if it is currently greater than 29.

I think we still need to call preventDefault so that the onup/ondown function is not called in some cases. The original code always calls those functions and the only way to prevent that is to have the handler call peventDefault and then check the return from dispatchEvent. Let's just leave it alone for now.  When we actually get to bug 316353 and I implement the event handler stuff it may turn out that we really don't need preventDefault and we can remove it at that time.

Comment on attachment 245665 [details] [diff] [review]
patch

I don't like changes in spinbuttons and numberbox widgets because
1) it's impossible to migrate to toolkit's analogues when xforms extension will be targeted for f.x. 3.0
2) I don't like a lot the magic with preventDefault(). You will find details below.

>+        var evt = document.createEvent("Events");
>+        evt.initEvent(eventname, true, true);
>+        var cancel = this.dispatchEvent(evt);
>+        if (cancel) {
>+          if (this.hasAttribute("on" + eventname)) {
>+            var fn = new Function("event", this.getAttribute("on" + eventname));
>+            if (fn.call(this, event) == false)
>+              cancel = true;
>+          }
>+        }
>+
>+        return !cancel;

Toolkit's spinbuttons executes code of 'onup'/'ondown' attributes always. Since 'onup'/'ondown' attributes are analogues of event listeners for 'up'/'down' events then preventDefault() call shouldn't reject executing of these attributes.

>+      <handler event="keypress" keycode="VK_DOWN">
>+        <![CDATA[
>+        var evt = document.createEvent("Events");
>+        evt.initEvent("down", true, true);
>+        var cancel = !this.dispatchEvent(evt);
>+        if (cancel) {
>+          aEvent.preventDefault();
>+        }
>+        else {
>+          this._modifyDown();
>+        }
>+        ]]>
>+      </handler>

I'm not fine with we fire 'up'/'down' events from numberbox since these events are from spinbuttons. When value of numbebox is changed then 'change' event is fired so I'd like rather use 'change' event than to fire 'up'/'down' events here.
Comment on attachment 245665 [details] [diff] [review]
patch

r=me til Neil is fine with changes of toolkit's spinbuttons/numberbox widgets. My thoughts why I don't like a lot these changes are in comment 68.
Attachment #245665 - Flags: superreview?(neil)
Attachment #245665 - Flags: review?(surkov.alexander)
Attachment #245665 - Flags: review+
(In reply to comment #68)
> (From update of attachment 245665 [details] [diff] [review] [edit])
>> 2) I don't like a lot the magic with preventDefault(). You will find details
> below.
> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent(eventname, true, true);
> >+        var cancel = this.dispatchEvent(evt);
> >+        if (cancel) {
> >+          if (this.hasAttribute("on" + eventname)) {
> >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> >+            if (fn.call(this, event) == false)
> >+              cancel = true;
> >+          }
> >+        }
> >+
> >+        return !cancel;
> Toolkit's spinbuttons executes code of 'onup'/'ondown' attributes always. Since
> 'onup'/'ondown' attributes are analogues of event listeners for 'up'/'down'
> events then preventDefault() call shouldn't reject executing of these
> attributes.
But the issue is that they are always executed and in some instances we may need to prevent them frome excuting. Let's leave it for now and if we can be successful changing the max attribute for the numberbox on the fly then we won't need the preventDefault logic and can remove it.

> >+      <handler event="keypress" keycode="VK_DOWN">
> >+        <![CDATA[
> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent("down", true, true);
> >+        var cancel = !this.dispatchEvent(evt);
> >+        if (cancel) {
> >+          aEvent.preventDefault();
> >+        }
> >+        else {
> >+          this._modifyDown();
> >+        }
> >+        ]]>
> >+      </handler>
> I'm not fine with we fire 'up'/'down' events from numberbox since these events
> are from spinbuttons. When value of numbebox is changed then 'change' event is
> fired so I'd like rather use 'change' event than to fire 'up'/'down' events
> here.
But then how do you know if the key pressed was up or down? This could be another case where we go ahead and let the numberbox change it and then in the event handlers set it back to a correct value if the change shouldn't have been allowed.

A lot of these little details remain to be seen when we actually do the range for 316353.  I think we should just leave things as they are and then tweak them if necessary as part of 316353.  
XBL currently has a problem with its event system. If an XBL binding is defined in chrome then all of its <handler>s only listen to trusted events. Now the issue here is that you want to dispatch events from content (which are therefore untrusted). Toolkit's numberbox has this bug and in fact Enn accidentally implemented both a <handler> and the onup/ondown attributes (which don't use event listeners and therefore work in content), and nobody noticed.
(In reply to comment #71)
> XBL currently has a problem with its event system. If an XBL binding is defined
> in chrome then all of its <handler>s only listen to trusted events. Now the
> issue here is that you want to dispatch events from content (which are
> therefore untrusted). Toolkit's numberbox has this bug and in fact Enn
> accidentally implemented both a <handler> and the onup/ondown attributes (which
> don't use event listeners and therefore work in content), and nobody noticed.
> 

Yes, this is why I prefer to remove 'up'/'down' event handlers added in xbl:handler in numberbox, I think this helps to avoid future problem when that bug will be fixed. But my doubts are concerned with approach that took Merle to modify spinbuttons/numberbox. This is why I asked your approval for Merle's work.
Blocks: 316353
No longer depends on: 316353
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #70)
> (In reply to comment #68)
> > (From update of attachment 245665 [details] [diff] [review] [edit] [edit])

> > >+      <handler event="keypress" keycode="VK_DOWN">
> > >+        <![CDATA[
> > >+        var evt = document.createEvent("Events");
> > >+        evt.initEvent("down", true, true);
> > >+        var cancel = !this.dispatchEvent(evt);
> > >+        if (cancel) {
> > >+          aEvent.preventDefault();
> > >+        }
> > >+        else {
> > >+          this._modifyDown();
> > >+        }
> > >+        ]]>
> > >+      </handler>
> > I'm not fine with we fire 'up'/'down' events from numberbox since these events
> > are from spinbuttons. When value of numbebox is changed then 'change' event is
> > fired so I'd like rather use 'change' event than to fire 'up'/'down' events
> > here.
> But then how do you know if the key pressed was up or down? This could be
> another case where we go ahead and let the numberbox change it and then in the
> event handlers set it back to a correct value if the change shouldn't have been
> allowed.

This is a good point that maybe neil should consider for the toolkit's spinbuttons.  Why don't the key presses for the toolkit's spinbuttons generate an up/down event?  Change isn't a good event to listen for because then it is already too late to prevent anything.  And I think requiring a user to put a key handler on the spinbutton is really overkill.  A keyup and click on the up button should be able to be handled with one event by the user.
(In reply to comment #73)
>This is a good point that maybe neil should consider for the toolkit's
>spinbuttons.  Why don't the key presses for the toolkit's spinbuttons generate
>an up/down event?  Change isn't a good event to listen for because then it is
>already too late to prevent anything.  And I think requiring a user to put a
>key handler on the spinbutton is really overkill.  A keyup and click on the up
>button should be able to be handled with one event by the user.
CCing the appropriate Neil ;-)
(In reply to comment #68)
> (From update of attachment 245665 [details] [diff] [review] [edit])
> I don't like changes in spinbuttons and numberbox widgets because
> 1) it's impossible to migrate to toolkit's analogues when xforms extension will
> be targeted for f.x. 3.0
> 2) I don't like a lot the magic with preventDefault(). You will find details
> below.
> 
> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent(eventname, true, true);
> >+        var cancel = this.dispatchEvent(evt);
> >+        if (cancel) {
> >+          if (this.hasAttribute("on" + eventname)) {
> >+            var fn = new Function("event", this.getAttribute("on" + eventname));
> >+            if (fn.call(this, event) == false)
> >+              cancel = true;
> >+          }
> >+        }
> >+
> >+        return !cancel;
> 
> Toolkit's spinbuttons executes code of 'onup'/'ondown' attributes always. Since
> 'onup'/'ondown' attributes are analogues of event listeners for 'up'/'down'
> events then preventDefault() call shouldn't reject executing of these
> attributes.
> 

Actually, they are only similar to event listeners for up and down; they are not the same as event handlers.  They are executed like the event's default handler  since they are executed after dispatchEvent returns.  The problem is that they are executed unconditionally.  And I believe that this is in error and something that should be fixed in the toolkit's spinbuttons, also.  A user listening for events should be able to prevent a numberbox from increasing or decreasing by calling preventDefault.  Neil, what do you think?
(In reply to comment #75)

> Actually, they are only similar to event listeners for up and down; they are
> not the same as event handlers.  They are executed like the event's default
> handler  since they are executed after dispatchEvent returns.  The problem is
> that they are executed unconditionally.  And I believe that this is in error
> and something that should be fixed in the toolkit's spinbuttons, also.  A user
> listening for events should be able to prevent a numberbox from increasing or
> decreasing by calling preventDefault.  Neil, what do you think?
> 

Then 'onup'/'ondown' names are not right for this. Let call them 'upDefaultAction' or something otherwise it confuses.
Neil(s): do you think my comments about preventDefault not preventing numberboxes from incrementing and keyup keypresses not generating the same event as the up spinbutton click have merit?  If you think those are valid issues against toolkit's spinbuttons, then I'll open a bug.  And then we can fix the same issues in our spinbuttons once the toolkit bug is resolved.
(In reply to comment #77)
> Neil(s): do you think my comments about preventDefault not preventing
> numberboxes from incrementing and keyup keypresses not generating the same
> event as the up spinbutton click have merit?  If you think those are valid
> issues against toolkit's spinbuttons, then I'll open a bug.  And then we can
> fix the same issues in our spinbuttons once the toolkit bug is resolved.
> 

Probably it is difficult to say does your proposition make sense for toolkit right now. So, we can check in this patch as is (without additional r or sr), then implement range widgets that will show usecases of your proposition. Then we'll file new bug for toolkit widgets. Neil, Enn, Aaron, does it work for all of you?
spinbuttons aren't focusable so don't receive keyboard events.

I don't understand what you're asking for. A way to prevent the user from incrementing or decrementing the value? Why would you need that?
 
(In reply to comment #79)
> spinbuttons aren't focusable so don't receive keyboard events.
> 
> I don't understand what you're asking for. A way to prevent the user from
> incrementing or decrementing the value? Why would you need that?
> 
> 

Just to make it more consistent with other controls.  That an event handler attached by an author that calls preventDefault should have that work...that the default behavior of the control shouldn't happen.  For whatever reason.  I can't say that I have a really good usecase.
Default behaviour of which event? The up and down events don't have any default behaviour to prevent.
(In reply to comment #81)
> Default behaviour of which event? The up and down events don't have any default
> behaviour to prevent.
> 

I think we are looking at things differently.  I think Merle and I are looking at the 'numberbox' as a whole control, not necessarily the spinbuttons being separate from the behavior of the numberbox.  And up to now we've tried to make the up/down event fulfill our requirements.  So let's forget about the 'spinbuttons' widget and up/down events for a second and concentrate on the 'numberbox' type of control (textfield combined with spinbuttons widget) and our requirements.

For now, let's call the widget that is the combination of a textfield control and a spinbuttons control a 'fooWidget'.  I believe that there should be some type of up/down event that is targeted at the fooWidget and should have the default behavior of changing the value of the fooWidget to its next up or down value.  That could mean incrementing/decrementing a numerical value or going to the next/previous value in a list of values, etc.  Let's call these events 'fooUp' and 'fooDown'.

From following this thread, it seems to me that we are looking for:
1) fooUp and fooDown should probably be targeted at the fooWidget.  They need to be separate so that we can determine if a specific fooWidget has been requested by a user to go to its next up value or next down value.
2) if fooUp and fooDown are handled and preventDefault called, then the fooWidget's value should not change.
3) fooUp and fooDown should be generated by keyup and keydown key presses when the textfield has focus as well as by mouse clicks on the spinbuttons widget.  This way only one set of events need to be handled.

Now, it sounds like Neil does not think that altering up/down's behavior to handle the requirements that we have for fooUp and fooDown is a good idea.  So I guess my question now is: do any of the toolkit users have requirements like ours such that the toolkit's xul:textbox @type="number" widget might be altered to be an extension of fooWidget and thus we get fooWidget, fooUp and fooDown into the toolkit?  The upside to this is that we can keep our xforms efforts in sync with toolkit efforts and we'll be able to actually use the toolkit's fooWidget in the future.  Then we'd only need our own fooWidget, etc. for the branches.

Or should we import the code from numberbox and spinbuttons like we've already done in these patches and mangle them to meet our needs?  To create our own fooWidget.  In this way we'll always use our own fooUp, fooDown and fooWidget.

Alex, please correct me if my requirements list is incorrect.
Sounds like you just want to change the code here:

      <handler event="keypress" keycode="VK_UP">
        this._modifyUp();
      </handler>

      <handler event="keypress" keycode="VK_DOWN">
        this._modifyDown();
      </handler>

to fire up and down events instead. Is that all that is desired?
Of course that does depend on bug 350334 being fixed...
(In reply to comment #83)
> Sounds like you just want to change the code here:
> 
>       <handler event="keypress" keycode="VK_UP">
>         this._modifyUp();
>       </handler>
> 
>       <handler event="keypress" keycode="VK_DOWN">
>         this._modifyDown();
>       </handler>
> 
> to fire up and down events instead. Is that all that is desired?
> 

The only issue with doing it this way is that the up/down from the spinbuttons widget and the up/down that you'd generate here would have two different targets, right?  I think we can make that work for us.  But I think the better solution for the toolkit would be that the fooUp and fooDown event have the same target.  However, we'll take what we can get.
> The only issue with doing it this way is that the up/down from the spinbuttons
> widget and the up/down that you'd generate here would have two different
> targets, right?

Not to a consumer of a number box, no.

Or, are you creating a binding that extends numberbox where that is an issue? What extra functionality do you need?
(In reply to comment #86)
> > The only issue with doing it this way is that the up/down from the spinbuttons
> > widget and the up/down that you'd generate here would have two different
> > targets, right?
> 
> Not to a consumer of a number box, no.
> 

The target of up and down now are the "spinbuttons" widget, right?  So if you modify the "numberbox" binding to generate up and down events when VK_UP and VK_DOWN keys are pressed, I guess I assumed you'd target these new up and down events at the numberbox.  So if someone put an "up" listener on the numberbox they could potentially see one targeted at the numberbox and one targeted at the spinbuttons.  That is the point I was trying to make.

Or did you mean that the "up" and "down" events generated by VK_UP and VK_DOWN key presses on a numberbox would also target the spinbuttons?

> Or, are you creating a binding that extends numberbox where that is an issue?
> What extra functionality do you need?
> 

We are currently extending numberbox only because we need to know if the user is requesting to move to the next up value or down value of the numberbox.  We don't care if it is due to a click or a key press, we just need to know which direction.  So we create a custom fooUp event to signal a VK_UP or the click on the up spinbutton happened and we create a custom fooDown event to signal a VK_DOWN or the click on the down spinbutton.  We also need to know which numberbox is about to change (so we make the numberbox the target of our custom events).  If the toolkit is willing to generate such events targeted at the numberbox, then we won't have any reason to extend numberbox, I don't think.
(In reply to comment #87)
> (In reply to comment #86)
> > > The only issue with doing it this way is that the up/down from the spinbuttons
> > > widget and the up/down that you'd generate here would have two different
> > > targets, right?
> > 
> > Not to a consumer of a number box, no.
> > 
> 
> The target of up and down now are the "spinbuttons" widget, right?  So if you
> modify the "numberbox" binding to generate up and down events when VK_UP and
> VK_DOWN keys are pressed, I guess I assumed you'd target these new up and down
> events at the numberbox.  So if someone put an "up" listener on the numberbox
> they could potentially see one targeted at the numberbox and one targeted at
> the spinbuttons.  That is the point I was trying to make.
> 

event.target always will be the <textbox type="number"/> The event.originalTarget will differ, but that's an xbl implementation detail that users of the numberbox don't need to deal with.


> Or did you mean that the "up" and "down" events generated by VK_UP and VK_DOWN
> key presses on a numberbox would also target the spinbuttons?

> 
> We are currently extending numberbox only because we need to know if the user
> is requesting to move to the next up value or down value of the numberbox.

But why? Why does it matter how the user changes the value?
(In reply to comment #88)
> (In reply to comment #87)
> > (In reply to comment #86)
> > > > The only issue with doing it this way is that the up/down from the spinbuttons
> > > > widget and the up/down that you'd generate here would have two different
> > > > targets, right?
> > > 
> > > Not to a consumer of a number box, no.
> > > 
> > 
> > The target of up and down now are the "spinbuttons" widget, right?  So if you
> > modify the "numberbox" binding to generate up and down events when VK_UP and
> > VK_DOWN keys are pressed, I guess I assumed you'd target these new up and down
> > events at the numberbox.  So if someone put an "up" listener on the numberbox
> > they could potentially see one targeted at the numberbox and one targeted at
> > the spinbuttons.  That is the point I was trying to make.
> > 
> 
> event.target always will be the <textbox type="number"/> The
> event.originalTarget will differ, but that's an xbl implementation detail that
> users of the numberbox don't need to deal with.
> 

In our particular case, we are trying to combine numberboxes to build a new widget to represet data of type xsd:dateTime.  So this will require 6 spinbuttons inside our widget to represent year, month, day, hours, minutes and seconds.  We'd prefer putting an event listener on our dateTime widget listening for up and down events and then comparing the event's original target against the numberbox.  Otherwise we have to have event listeners on each numberbox which is, of course, much less efficient.

> 
> > Or did you mean that the "up" and "down" events generated by VK_UP and VK_DOWN
> > key presses on a numberbox would also target the spinbuttons?
> 
> > 
> > We are currently extending numberbox only because we need to know if the user
> > is requesting to move to the next up value or down value of the numberbox.
> 
> But why? Why does it matter how the user changes the value?
> 

We envision using spin controls for more than just numbers, like to navigate a list of items.  Then we'd have to know if the user is selecting the next value or the previous value in the list.  I know that this isn't necessarily needed for numberbox, but if toolkit establishes how these events should behave on a numberbox (i.e. fooUp and fooDown targeted at the fooWidget) then we just continue the same behavior in our own list spin widgets.  But no, we probably don't need to know if up or down happened on a numberbox anymore.  We think we can get around this if we listen for change events instead of up or down and set min/max attributes on the numberbox on the fly rather than managing these ourselves.
> In our particular case, we are trying to combine numberboxes to build a new
> widget to represet data of type xsd:dateTime.  So this will require 6
> spinbuttons inside our widget to represent year, month, day, hours, minutes and
> seconds.

In that case, it would be easier to just use the date and time pickers in bug 92174

> We envision using spin controls for more than just numbers, like to navigate a
> list of items.  Then we'd have to know if the user is selecting the next value
> or the previous value in the list.

<menulist> is the right UI widget to use for choosing from a fixed list of items.
(In reply to comment #90)
> > In our particular case, we are trying to combine numberboxes to build a new
> > widget to represet data of type xsd:dateTime.  So this will require 6
> > spinbuttons inside our widget to represent year, month, day, hours, minutes and
> > seconds.
> 
> In that case, it would be easier to just use the date and time pickers in bug
> 92174

I should have mentioned that it isn't just a simple dateTime widget.  It is a dateTime widget that has a min and max values...so that the user can only pick dates and times from within a range.  We figured it would be much quicker to implement (and simpler for a user to navigate quickly) using spinbuttons.

> 
> > We envision using spin controls for more than just numbers, like to navigate a
> > list of items.  Then we'd have to know if the user is selecting the next value
> > or the previous value in the list.
> 
> <menulist> is the right UI widget to use for choosing from a fixed list of
> items.
> 

Depends on the list of items, right?  I'd say a large finite list of anything with a predictable pattern is easier to navigate with a spin control than a menulist.  For example, a large list of ordered numbers like phone numbers or television channels or a large, alphabetical list of names or words.
I tried out the widgets in bug 92174.  They look really nice.  Have you considered putting min/max attribute support on date picker and time picker?  If not, we could copy the code into our extension and add that support for our own use.  We'd probably start out with only using the 'entry' type.  Maybe go to popup once we figure out how to 'range-ify' it.
(In reply to comment #92)
> We'd probably start out with only using the 'entry' type.  Maybe go
> to popup once we figure out how to 'range-ify' it.

You can already express this in XForms with XML Schema using the facets minInclusive, maxInclusive, minExclusive, and maxExclusive: http://www.w3.org/TR/2001/REC-xmlschema-2-20010502/#dateTime-facets

It would be nice if the XForms reccommendation made xf:bind support all of XSD's constraining facets (http://www.w3.org/TR/2001/REC-xmlschema-2-20010502/#rf-facets) directly, just as it supports type.  Then you could just do 
  <xf:bind nodeset="birthday" minInclusive="1990-01-01" maxExclusive="2006-12-05" />

(In reply to comment #93)
> (In reply to comment #92)
> > We'd probably start out with only using the 'entry' type.  Maybe go
> > to popup once we figure out how to 'range-ify' it.
> 
> You can already express this in XForms with XML Schema using the facets
> minInclusive, maxInclusive, minExclusive, and maxExclusive:
> http://www.w3.org/TR/2001/REC-xmlschema-2-20010502/#dateTime-facets
> 
> It would be nice if the XForms reccommendation made xf:bind support all of
> XSD's constraining facets
> (http://www.w3.org/TR/2001/REC-xmlschema-2-20010502/#rf-facets) directly, just
> as it supports type.  Then you could just do 
>   <xf:bind nodeset="birthday" minInclusive="1990-01-01"
> maxExclusive="2006-12-05" />
> 

Leigh, we are looking at using these XUL widgets to render the date and time types for xf:range, so we'll already have the information that we need as far as how far the value can change.  The problem is how easy it will be to modify the calendar popup code to take an arbitrary limit.
(In reply to comment #88)
> (In reply to comment #87)
> > (In reply to comment #86)
> > > > The only issue with doing it this way is that the up/down from the spinbuttons
> > > > widget and the up/down that you'd generate here would have two different
> > > > targets, right?
> > > 
> > > Not to a consumer of a number box, no.
> > > 
> > 
> > The target of up and down now are the "spinbuttons" widget, right?  So if you
> > modify the "numberbox" binding to generate up and down events when VK_UP and
> > VK_DOWN keys are pressed, I guess I assumed you'd target these new up and down
> > events at the numberbox.  So if someone put an "up" listener on the numberbox
> > they could potentially see one targeted at the numberbox and one targeted at
> > the spinbuttons.  That is the point I was trying to make.
> > 
> 
> event.target always will be the <textbox type="number"/> The
> event.originalTarget will differ, but that's an xbl implementation detail that
> users of the numberbox don't need to deal with.

Like Aaron said, I can't use event.target if I try to build new XBL widget based on numberbox (new widget contains numberbox as child node).

I'd like if numberbox will fire additional events like 'spinbuttonup'/'spinbuttondown'. But I still don't think default action of these (and up/down) events should be cancelable because spinbuttons should be disabled if numberbox can't be incremeted/decremented.

What's state of this bug?
Depends on: 365907
Blocks: 365907
No longer depends on: 365907
(In reply to comment #95)
> What's state of this bug?
Waiting for superreview from Neil?

As I see it, these are the remaining issues and proposed solutions:

1. Remove 'if (cancel)...' logic from spinbuttons command handler because we don't want the mouse press events to be cancelable.
2. Remove similar logic in numberbox keypress handler that calls preventDefault because we don't want keypress events to be cancelable.
3. Spinbox WILL still fire spinup and spindown events as in the current patch.
4. Add logic to numberbox keypress handlers to fire spinup and spindown events. This will be required for the range bindings that will use the spinbox.  Shall we do it as part of this bug or defer it until the range binding bug?






(In reply to comment #96)
> (In reply to comment #95)
> > What's state of this bug?
> Waiting for superreview from Neil?

I guess Neil delegated to Enn :)

> As I see it, these are the remaining issues and proposed solutions:
> 
> 1. Remove 'if (cancel)...' logic from spinbuttons command handler because we
> don't want the mouse press events to be cancelable.
> 2. Remove similar logic in numberbox keypress handler that calls preventDefault
> because we don't want keypress events to be cancelable.
> 3. Spinbox WILL still fire spinup and spindown events as in the current patch.
> 4. Add logic to numberbox keypress handlers to fire spinup and spindown events.
> This will be required for the range bindings that will use the spinbox.  Shall
> we do it as part of this bug or defer it until the range binding bug?
> 

Initially, I guess we should change toolkit bindings to reflect our needs. Though we need to get approval for #4.
(In reply to comment #97)
> (In reply to comment #96)
> > (In reply to comment #95)
> > > What's state of this bug?
> > Waiting for superreview from Neil?
> 
> I guess Neil delegated to Enn :)
> 
> > As I see it, these are the remaining issues and proposed solutions:
> > 
> > 1. Remove 'if (cancel)...' logic from spinbuttons command handler because we
> > don't want the mouse press events to be cancelable.
> > 2. Remove similar logic in numberbox keypress handler that calls preventDefault
> > because we don't want keypress events to be cancelable.
> > 3. Spinbox WILL still fire spinup and spindown events as in the current patch.
> > 4. Add logic to numberbox keypress handlers to fire spinup and spindown events.
> > This will be required for the range bindings that will use the spinbox.  Shall
> > we do it as part of this bug or defer it until the range binding bug?
> > 
> 
> Initially, I guess we should change toolkit bindings to reflect our needs.
> Though we need to get approval for #4.
> 


I believe that Merle meant OUR numberbox, not the toolkit's.  From the feedback on this bug so far, it doesn't sound like our requirements will be pulled into the toolkit.

I vote that we do all of those changes in the patch for this bug since they all go toward basic spinbutton behaviors.
(In reply to comment #98)

> I vote that we do all of those changes in the patch for this bug since they all
> go toward basic spinbutton behaviors.
> 

Do you want to make changes in xforms imported numberbox/spinbuttons and implement xforms range controls, see how all works and then get back to pulling the changes into the toolkit?
(In reply to comment #99)
> (In reply to comment #98)
> 
> > I vote that we do all of those changes in the patch for this bug since they all
> > go toward basic spinbutton behaviors.
> > 
> 
> Do you want to make changes in xforms imported numberbox/spinbuttons and
> implement xforms range controls, see how all works and then get back to pulling
> the changes into the toolkit?
> 

We have to do the changes on our end first anyhow since we need them for FF2.  Hopefully by then they'll see how useful our changes are.
Comment on attachment 245665 [details] [diff] [review]
patch

So, then I guess we don't need sr right now (since these changes aren't going to be reflected on toolkit's widgets). Aaron, what do we want in this bug per Merle's comment #96?
Attachment #245665 - Flags: superreview?(neil)
Attached patch patch (obsolete) — Splinter Review
Remove code to cancel events; fire up/down events in keypress handler of numberbox.
Attachment #251105 - Flags: review?(surkov.alexander)
Attachment #245665 - Attachment is obsolete: true
(In reply to comment #101)
> (From update of attachment 245665 [details] [diff] [review])
> So, then I guess we don't need sr right now (since these changes aren't going
> to be reflected on toolkit's widgets). Aaron, what do we want in this bug per
> Merle's comment #96?
> 

What Merle mentioned in that comment sounds like what we've agreed upon in this bug.  The only other thing in this bug that we mentioned that I can think of is having the dateTime control automatically adjust the min/max on the various numberboxes it contains so that the user can't spin an invalid date or time.  But we'll do that in the bug that implements the dateTime widget for XForms.
Comment on attachment 251105 [details] [diff] [review]
patch


>+      <handler event="keypress" keycode="VK_UP">
>+        <![CDATA[
>+        var evt = document.createEvent("Events");
>+        evt.initEvent("up", true, true);
>+        this.dispatchEvent(evt);
>+        ]]>
>+      </handler>

>+      <handler event="up" preventdefault="true">
>+        this._modifyUp();
>+      </handler>
>+

Do you want to say new fired 'up' event will be handled here? Does it work?

>+  <!-- SPINBOX
>+   The spinbox widget contains a textbox with spinbuttons for incrementing
>+   and decrementing the value in the textbox.
>+  -->

I'd say you don't need spinbox widget. I guess it's fine to fire 'spinup'/'spindown' events inside _modifyUp/_modfiyDown methods of numberbox. What do you think?
(In reply to comment #104)
> (From update of attachment 251105 [details] [diff] [review])
> >+      <handler event="keypress" keycode="VK_UP">
> >+        <![CDATA[
> >+        var evt = document.createEvent("Events");
> >+        evt.initEvent("up", true, true);
> >+        this.dispatchEvent(evt);
> >+        ]]>
> >+      </handler>
> >+      <handler event="up" preventdefault="true">
> >+        this._modifyUp();
> >+      </handler>
> >+
> Do you want to say new fired 'up' event will be handled here? Does it work?
The up event won't be handled directly by the numberbox. Recall the discussion above that there seems to be an XBL bug where for some reason the numberbox never sees the up event. That is the behavior I observed doing the other bug.  Anyway when the event bubbles up, the spinbox gets it and fires spinup.

This is a case where I think we need to just go with it as is so we can get to the other range bug and then if even firing changes we can address any new issues it creates in a new bug.

> >+  <!-- SPINBOX
> >+   The spinbox widget contains a textbox with spinbuttons for incrementing
> >+   and decrementing the value in the textbox.
> >+  -->
> I'd say you don't need spinbox widget. I guess it's fine to fire
> 'spinup'/'spindown' events inside _modifyUp/_modfiyDown methods of numberbox.
> What do you think?

We do need the spinbox.  It is the combination of spinbuttons and numberbox and it needs to handle up/down events from the spinbuttons (mouse clicks) and up/down events from the numberbox (keypresses).  The way it will work is the spinbox handles up/down events and fires spinup/spindown.  The range binding will handle spinup/down and change the min/max attrs as necessary so the range is constrained to the min/max set on the range control.
(In reply to comment #105)

> > Do you want to say new fired 'up' event will be handled here? Does it work?
> The up event won't be handled directly by the numberbox. Recall the discussion
> above that there seems to be an XBL bug where for some reason the numberbox
> never sees the up event. That is the behavior I observed doing the other bug. 

Right, I know this and that means value of numberbox will not be changed on up/down pressing.

> > I'd say you don't need spinbox widget. I guess it's fine to fire
> > 'spinup'/'spindown' events inside _modifyUp/_modfiyDown methods of numberbox.
> > What do you think?
> 
> We do need the spinbox.  It is the combination of spinbuttons and numberbox and
> it needs to handle up/down events from the spinbuttons (mouse clicks) and
> up/down events from the numberbox (keypresses).  The way it will work is the
> spinbox handles up/down events and fires spinup/spindown.  The range binding
> will handle spinup/down and change the min/max attrs as necessary so the range
> is constrained to the min/max set on the range control.
> 

Yes, that is how is in your patch. But that involves redurand 'spinbox' widget imo. Do you think is is bad if numberbox will fire spinup/spindown events itself? If it will then we don't need 'spinbox' widget.

Aaron, what do you think?
It's not necessarily 'bad' to have numberbox fire spinup/spindown events but I like consistency and it seems more logical to fire up/down. That way the spinbox widget handles up/down from both the spinbuttons and the numberbox and refires spinup/spindown.

Without the spinbox widget, the range bindings will have 4 event handlers: up/down from spinbuttons and spinup/spindown from the numberbox. Also we want the ability to place labels next to the spinbuttons because a range control will consist of more than one in some cases (eg.date, dateTime). If we were to modify numberbox to include a label then it would get even further away from the toolkit implementation and likely be an issue were we to try to port our changes back to the toolkit in the future.

For those reasons, I don't consider the spinbox widget redundant.
So, if we don't want any label abibility like said Aaron then please remove spinbox widget and send spinup/down events from numberbox. Note, send these events in _modifyUp/Down methods and you will not need to listen 4 events.
ah, also please remove up/down xbl:handlers element since they don't work and when they start to work then they broke numberbox.
Attachment #251105 - Flags: review?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
Remove spinbox widgets; fire spinup/spindown events from numberbox _modifyUp/_modifyDown methods.
Attachment #251105 - Attachment is obsolete: true
Attachment #251315 - Flags: review?(surkov.alexander)
Comment on attachment 251315 [details] [diff] [review]
patch


>+      <method name="_modifyUp">
>+        <body>
>+          <![CDATA[
>+            if (this.disabled || this.readOnly)
>+              return;
>+
>+            var evt = document.createEvent("Events");
>+            evt.initEvent("spinup", true, true);
>+            this.dispatchEvent(evt);

I think event shoudn't be cancelable and event should be fired after value change, otherwise what do we need it?
WIth those r=me
Attachment #251315 - Flags: review?(surkov.alexander) → review+
Comment on attachment 251315 [details] [diff] [review]
patch

There is a lot happening in validateValue() and it gets called from a lot of places, so please comment the function with either a summary or a step by step comment about what is going on.

Also please comment the keyPress handler so we don't have to go looking up char codes each time we debug through.

Please get with me tomorrow to discuss alex's comments to make sure that we are on the same page and then I'll r+ your new patch with the comments I've requested.  Thanks.
Attached patch another patchSplinter Review
Moving firing of spinup/spindown events to after the value has been changed; commented keypress handler and validateValue method.
Attachment #251315 - Attachment is obsolete: true
Attachment #252523 - Flags: review?(aaronr)
Attachment #252523 - Flags: review?(surkov.alexander)
Attachment #252523 - Flags: review?(aaronr) → review+
Comment on attachment 252523 [details] [diff] [review]
another patch


>+            var oldval = this.value;
>+            var newval = this.increase();
>+            this.inputField.select();
>+            if (oldval != newval)
>+              this._fireChange();
>+
>+            var evt = document.createEvent("Events");
>+            evt.initEvent("spinup", true, false);
>+            this.dispatchEvent(evt);

I'm not sure but why do you need 'spinup' event if values hasn't been changed? If actually you don't then please fix it.
Attachment #252523 - Flags: review?(surkov.alexander) → review+
(In reply to comment #114)
> (From update of attachment 252523 [details] [diff] [review])
> 
> >+            var oldval = this.value;
> >+            var newval = this.increase();
> >+            this.inputField.select();
> >+            if (oldval != newval)
> >+              this._fireChange();
> >+
> >+            var evt = document.createEvent("Events");
> >+            evt.initEvent("spinup", true, false);
> >+            this.dispatchEvent(evt);
> 
> I'm not sure but why do you need 'spinup' event if values hasn't been changed?
> If actually you don't then please fix it.
> 

By agreement with surkov, msterlin will change this if he finds out that xf:range doesn't need the event fired even when a value change hasn't occurred.  Leaving as is for now.
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: