316353 - Support range for other datatypes than numbers

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

The spec. states that range should support: xsd:duration, xsd:date, xsd:time, xsd:dateTime, xsd:gYearMonth, xsd:gYear, xsd:gMonthDay, xsd:gDay, xsd:gMonth, xsd:float, xsd:decimal, xsd:double

Comment 1

[aaronr]

I don't think that this is an enhancement since it clearly says in the spec that these datatypes must be supported.

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> I don't think that this is an enhancement since it clearly says in the spec
> that these datatypes must be supported.

Spec. schmeck :) The spec. only says that we should not bind to anything else than these types ;)

Yes, you are right.

Comment 3

[Merle Sterling]

With the exception of gDay, gMonth, and gYear (which work with the current code for numeric types) none of these types will work well with a regular slider control. 

Date and dateTime could perhaps use a calendar control but we would need to implement an entirely new control to restrict the range to fall between 'start' and 'end' and the notion of 'step' is not reallly applicable. If a step attribute is provided, we would have to interpret something like "0001-01-05" to mean 'step year by 1, step month by 1, step day by 5' (Steve Speicher's suggestion) and that is not really possible with a calendar control.

The remaining types would be best represented as a series of spin buttons, with one spin button per component of the type (eg. 'time' would have one spin button for hours(hh), one for minutes(mm), one for seconds(ss), and one for milliseconds(.sss)). date and dateTime will be easier to represent as spin buttons as well and we would have greater control over stepping thru the range. With spin buttons, we could interpret any provided step value for date as in the example above.

So the best approach for implementing support for all of these types is probably to implement a base spin button control and create a new binding for each of the supported range types using one or more spin buttons to represent the type.

Comment 4

[aaronr]

I would also vote to ignore @step for any type not represented by a slider control for the moment.  Until we get a good use case.  It isn't clear to me how step should be specified for the different types, or how it would be used.  In the case of a dateTime, if I specify step="0001-01-02" would that mean that I'm not allowed to modify the hours, minutes and seconds?  Would that mean that I can only specify days in increments of 2?  If we don't implement @step for these types, though, we should probably open a placeholder bug for the functionality and some statement in the bug for the types of behavior that we need defined.

Comment 5

[aaronr]

Attachment: xul number spinbutton sample

I found this numberbox spinbutton-type widget in mozilla.  Seems to work well.  Might be useful to study the code behind it and reuse it where possible.

Comment 6

[aaronr]

(In reply to comment #5)
> Created an attachment (id=236298) [edit]
> xul number spinbutton sample
> 
> I found this numberbox spinbutton-type widget in mozilla.  Seems to work well. 
> Might be useful to study the code behind it and reuse it where possible.
> 

Only works on trunk, so if we want this to work on branches, it won't be as simple as just pointing to the binding.

Comment 7

[Merle Sterling]

Attachment: testcase - xsd:date

Comment 8

[Merle Sterling]

Attachment: testcase - xsd:dateTime

Comment 9

[Merle Sterling]

Attachment: testcase - xsd:time

Comment 10

[Merle Sterling]

Attachment: testcase - xsd:gDay

Comment 11

[Merle Sterling]

Attachment: testcase - xsd:gMonth

Comment 12

[Merle Sterling]

Attachment: testcase - xsd:gMonthDay

Comment 13

[Merle Sterling]

Attachment: testcase - xsd:gYear

Comment 14

[Merle Sterling]

Attachment: testcase - xsd:gYearMonth

Comment 15

[Merle Sterling]

Attachment: testcase - xforms:dayTimeDuration

Comment 16

[Merle Sterling]

Attachment: testcase - xforms:yearMonthDuration

Comment 17

[Merle Sterling]

Attachment: Bindings for new range types

Created bindings for the new range types covered by the attached testcases. Each type is implemented using one or more spinbuttons for each component of the type; eg xsd:date is implemented using 3 spinbuttons - one each for years, months, and days.

Further enhancements to the spinbuttons and the types using them will be handled in another bug as this patch only addresses the required functionality to implement a range control for each of the new types.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 239714 [details] [diff] [review]
Bindings for new range types

I will do some comments below about code styling and general issues. Also I'd vote to do the patch after bug 352462 since changes are intersected. Also I will be happy if you can split the patch on several if it's possible.

>? resources/content/range-date.xml
>? resources/content/range-datetime.xml
>? resources/content/range-duration.xml
>? resources/content/range-gregorian.xml
>? resources/content/range-time.xml

You should use cvsdo to avoid this. On Windows OS I will do:

cd d:\cygwin\bin
perl cvsdo add /cygdrive/d/mozilla/mozilla/extensions/xforms/resources/content/range-date.xml

>+  content/xforms/range-date.xml                (resources/content/range-date.xml)
>+  content/xforms/range-time.xml                (resources/content/range-time.xml)
>+  content/xforms/range-datetime.xml            (resources/content/range-datetime.xml)
>+  content/xforms/range-gregorian.xml           (resources/content/range-gregorian.xml)
>+  content/xforms/range-duration.xml            (resources/content/range-duration.xml)

I'd prefer to put new bindings into range-xhtml.xml file if the bindings are not very big. But in any way if your widgets are for xhtml context then your file should have '-xhtml' postfix. It will be consistent with other xforms code. Also I don't see in your bindings where do you use "start", "end", "step" attributes.

>+  /* range type="xsd:int" */
>+html|*:root range[mozType|type="http://www.w3.org/2001/XMLSchema#int"] {
>+  -moz-binding: url('chrome://xforms/content/range.xml#xformswidget-range');
>+}

You should use @typelist attribute. I'm not sure but since xsd:int are derived from xsd:decimal then xsd:int binding isn't used. That's valid only if @typelist attirbute contains base type for shema spec definded types.

But in any way I prefer to see:

>+html|*:root range[mozType|type="http://www.w3.org/2001/XMLSchema#int"],
>+html|*:root range[mozType|type="http://www.w3.org/2001/XMLSchema#float"] {
>+  -moz-binding: url('chrome://xforms/content/range.xml#xformswidget-range');
>+}

And so one. The same is true for spinbuttons bindings. Also please use mozType:type attribute to add binding for spinbutton instead @class.

>+   - The Initial Developer of the Original Code is
>+   - Novell, Inc.

That's news for me :) Your spy essence was discovered!

>+   - Contributor(s):
>+   - Merle Sterling <[e-mail]>

I guess you can add '(original author)' after email.

>+      <property name="XHTML_NS" readonly="true"
>+                onget="return 'http://www.w3.org/1999/xhtml';"/>

Probably that's not very nice but I'd like to move it into 'xformswidget-base'.

>+      <method name="refresh">
>+        <body>
>+          <![CDATA[

What about readonly support?

>+      <method name="focus">
>+        <body>
>+          return true;
>+        </body>
>+      </method>

What about this?


>+      
>+  <binding id="xformswidget-range-gYearMonth"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">

Every binding should have comment before its declaration. Look at other our bindings.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 239714 [details] [diff] [review]
Bindings for new range types

Firstly I'm not sure that range widget should have look of spin control. Specs says: In graphical environments, this form control may be rendered as a "slider" or "rotary control". I don't know what is "rotary control" though. Also specs say: Implementations should inform the user of the upper and lower bounds, as well as the step size, if any. I don't see how your controls informs about it.
Second, if you using spin controls for range then 1) it's better to steal spinbuttons widget from toolkit since they are on trunk only I guess and 2) spin buttons should be placed together (increment on top, decrement on bottom) since it's more comfortable.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> You should use @typelist attribute. I'm not sure but since xsd:int are derived
> from xsd:decimal then xsd:int binding isn't used. That's valid only if
> @typelist attirbute contains base type for shema spec definded types.

Right, if type is xsd:integer then @typelist attribute is "xsd:integer xsd:decimal". Therefore you need css rule for xsd:decimal only when you will use @typelist attribute.

> Also please use
> mozType:type attribute to add binding for spinbutton instead @class.

Sorry, not mozType:type, please use mozType:spinbutton or something like.

But firstly (before you will update your patch) we should decide how the control should looks. I'd don't like a lot to have usual textfield with spinbuttons for range controls. But again, I don't know how they should look.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> Also I don't see in your bindings where do you use "start", "end", "step"
> attributes.

Never mind, I can see.

> Probably that's not very nice but I'd like to move it into 'xformswidget-base'.

Probably should be in 'xformswidget-general'.

> >+      <method name="focus">
> >+        <body>
> >+          return true;
> >+        </body>
> >+      </method>
> 
> What about this?

At least why do you return true if method is actually not implemented?

(In reply to comment #19)

> Second, if you using spin controls for range then 1) it's better to steal
> spinbuttons widget from toolkit since they are on trunk only I guess

Even if you posted bug 353880 and spinbuttons on trunk are only for xul then I guess it's good to try to steal them or at least to try to place them in more friendly way.

Comment 22

[aaronr]

(In reply to comment #19)
> (From update of attachment 239714 [details] [diff] [review] [edit])
> Firstly I'm not sure that range widget should have look of spin control. Specs
> says: In graphical environments, this form control may be rendered as a
> "slider" or "rotary control". I don't know what is "rotary control" though.

Rotary control is like the volume knob on a radio.  Circular and when spun clockwise, values increase.  When counter-clockwise, values decrease.

> Also specs say: Implementations should inform the user of the upper and lower
> bounds, as well as the step size, if any. I don't see how your controls informs
> about it.

Yes, eventually we'll have to have a lower limit on one side and upper limit on the other side of the set of spin buttons.  We chose spin buttons since it is easier for the user to get exactly the value that they want, especially when 'step' has very little meanging outside of numerical types.  For example, if you have a beginning date and an end date that are 100 years apart, it would take a long time to move a slider to the correct day somewhere in the middle when there are 36500 days in between the beginning and end.

> Second, if you using spin controls for range then 1) it's better to steal
> spinbuttons widget from toolkit since they are on trunk only I guess and 2)
> spin buttons should be placed together (increment on top, decrement on bottom)
> since it's more comfortable.
> 

This will be a multi-step process.  Multiple patches.  First is to get the functionality and usability agreed upon and checked in.  What things look like will need to go in on a seperate bug.  We'd be very happy to get our spin buttons to look as good as they do in XUL.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> > Also specs say: Implementations should inform the user of the upper and lower
> > bounds, as well as the step size, if any. I don't see how your controls informs
> > about it.
> 
> Yes, eventually we'll have to have a lower limit on one side and upper limit on
> the other side of the set of spin buttons.

How about @size?

>  We chose spin buttons since it is
> easier for the user to get exactly the value that they want, especially when
> 'step' has very little meanging outside of numerical types.  For example, if
> you have a beginning date and an end date that are 100 years apart, it would
> take a long time to move a slider to the correct day somewhere in the middle
> when there are 36500 days in between the beginning and end.

I understand slider or rotary controls are not always useful. The another thing confuses me. Now input that is bound to number data type looks like spin control in xul context. That means range and input are almost one and the same for some cases. I guess it's very free interpretation of specification. And therefore I'd prefer to have spin control rather for input than for range. Also that allows us to be care to specs.
 
> > Second, if you using spin controls for range then 1) it's better to steal
> > spinbuttons widget from toolkit since they are on trunk only I guess and 2)
> > spin buttons should be placed together (increment on top, decrement on bottom)
> > since it's more comfortable.
> > 
> 
> This will be a multi-step process.  Multiple patches.  First is to get the
> functionality and usability agreed upon and checked in.  What things look like
> will need to go in on a seperate bug.  We'd be very happy to get our spin
> buttons to look as good as they do in XUL.
> 

I can assume it's multi-step process. But firstly I'd prefer to have one good control instead many non-ended. Second, I guess it's better to modify existing things instead of writing new, more over if new things are not very comfortable.

Comment 24

[alexander :surkov (:asurkov)]

I'd vote to use spin controls for input that is bound to types Merle uses. It will be consistent with input[xsd:integer] for xul. @start/@end attributes we can handle from schema, though I don't know how to specify the @step.

But if we cannot handle @step via schema and you'd like to save spin controls for range in any way. 1) Then I guess we should remove spin controls for xul's input[xsd:integer]. 2) we should name these ranges as range[appearance='compact'] or something else to have both range presentations (spin-like and slider-like). Note, we should have spin-like range for number types too. 3) Also we should have slider-like range for all these types like we have it for numbers type now.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> I would also vote to ignore @step for any type not represented by a slider
> control for the moment.  Until we get a good use case.  It isn't clear to me
> how step should be specified for the different types, or how it would be used. 
> In the case of a dateTime, if I specify step="0001-01-02" would that mean that
> I'm not allowed to modify the hours, minutes and seconds?  Would that mean that
> I can only specify days in increments of 2?  If we don't implement @step for
> these types, though, we should probably open a placeholder bug for the
> functionality and some statement in the bug for the types of behavior that we
> need defined.
> 

Sorry, I missed your previous discussion :).
And if @step is supposed to be missed then I can't see no one reason why don't preserve spin controls for xforms input excepting the reason that @start/@end is more friendly than schema magic. But again we are in the spec wholly: 1) we don't miss @step attribute (since input hasn't it), 2) range looks like slider control.

Comment 26

[Merle Sterling]

The patch for this bug shares some code with bug 331987, so making this bug depend on 331987.

Comment 27

[aaronr]

(In reply to comment #25)
> (In reply to comment #4)
> > I would also vote to ignore @step for any type not represented by a slider
> > control for the moment.  Until we get a good use case.  It isn't clear to me
> > how step should be specified for the different types, or how it would be used. 
> > In the case of a dateTime, if I specify step="0001-01-02" would that mean that
> > I'm not allowed to modify the hours, minutes and seconds?  Would that mean that
> > I can only specify days in increments of 2?  If we don't implement @step for
> > these types, though, we should probably open a placeholder bug for the
> > functionality and some statement in the bug for the types of behavior that we
> > need defined.
> > 
> 
> Sorry, I missed your previous discussion :).
> And if @step is supposed to be missed then I can't see no one reason why don't
> preserve spin controls for xforms input excepting the reason that @start/@end
> is more friendly than schema magic. But again we are in the spec wholly: 1) we
> don't miss @step attribute (since input hasn't it), 2) range looks like slider
> control.
> 

You want to use a spin button for a xf:input bound to a node with a numerical type?  My only issue with that would behave differently than the other XForms processors on a control that would be OFTEN used (as opposed to date bound ranges which I doubt will be very common).  If you want to create a binding that a mozilla user can use via CSS instead of our default binding, I have no problem with that.  Or maybe hook it into with a xf:input with @appearance="full".

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)

> You want to use a spin button for a xf:input bound to a node with a numerical
> type?  My only issue with that would behave differently than the other XForms
> processors on a control that would be OFTEN used (as opposed to date bound
> ranges which I doubt will be very common).  If you want to create a binding
> that a mozilla user can use via CSS instead of our default binding, I have no
> problem with that.  Or maybe hook it into with a xf:input with
> @appearance="full".
> 

I'm not sure on 100% I like to see spin widget as xforms input presentation. Just input[xsd:integer] for xul have a look of spin control since it contains xul:textbox@type="number". If we decide that spin controls is for range than we should not use spin for input. That's all. How other xforms processors shows range controls?

Comment 29

[aaronr]

(In reply to comment #28)
> (In reply to comment #27)
> 
> > You want to use a spin button for a xf:input bound to a node with a numerical
> > type?  My only issue with that would behave differently than the other XForms
> > processors on a control that would be OFTEN used (as opposed to date bound
> > ranges which I doubt will be very common).  If you want to create a binding
> > that a mozilla user can use via CSS instead of our default binding, I have no
> > problem with that.  Or maybe hook it into with a xf:input with
> > @appearance="full".
> > 
> 
> I'm not sure on 100% I like to see spin widget as xforms input presentation.
> Just input[xsd:integer] for xul have a look of spin control since it contains
> xul:textbox@type="number". If we decide that spin controls is for range than we
> should not use spin for input. That's all. How other xforms processors shows
> range controls?
> 

usually sliders for the types that they support, which are mostly the numerical types.  formsPlayer doesn't support all of the types that we eventually will, but users sliders for the types that they do support.  XSmiles uses slider for numerical types and combines slider with calendar for some date types, I believe.  Might be nice to support that at some point (maybe for @appearance="full").  But that isn't trivial so we are doing spinbuttons first.  To do that we'd probably have to refactor datepicker to some degree.

Comment 30

[Merle Sterling]

Attachment: patch

New patch: 
- new bindings now extend from xformswidget-range-base.
- Fixed the issues mentioned by Surkov.

Comment 31

[alexander :surkov (:asurkov)]

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


>+  content/xforms/range-date-xhtml.xml          (resources/content/range-date-xhtml.xml)
>+  content/xforms/range-datetime-xhtml.xml      (resources/content/range-datetime-xhtml.xml)
>+  content/xforms/range-duration-xhtml.xml      (resources/content/range-duration-xhtml.xml)
>+  content/xforms/range-gregorian-xhtml.xml     (resources/content/range-gregorian-xhtml.xml)
>+  content/xforms/range-time-xhtml.xml          (resources/content/range-time-xhtml.xml)

Merle, do you think about xul controls support? I belive your bindings can be splitted on two, one is base bingind, other is xhtml binding. Probably in this case you don't need in huge amount of new files.

>+  <!-- The file contains the implementation of xforms range element for type
>+       xsd:date.

You should mention here you use spin representation.

>+          this.monthSpin = document.getAnonymousElementByAttribute(this, "anonid", "monthSpin");
>+          this.daySpin = document.getAnonymousElementByAttribute(this, "anonid", "daySpin");
>+          this.yearSpin = document.getAnonymousElementByAttribute(this, "anonid", "yearSpin");

That's not usual practice. Use the next template:

<property name="monthSpin" readonly="true">
  <getter>
    if (!this._monthSpin) {
      this._monthSpin = this.onwerDocument.getAnonymousElementByAttribute(this, "anonid", "monthSpin");
    }
    return this._monthSpin;
  </getter>
</property>
<field name="_monthSpin">null</field>

Comment 32

[alexander :surkov (:asurkov)]

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


>+ /* range type="xsd:date" */
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"] html|span[mozType|spinbutton] {
>+  -moz-binding: url('chrome://xforms/content/widgets-xhtml.xml#spinbutton');
>+}
>+
>+ /* range type="xsd:dateTime" */
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"] html|span[mozType|spinbutton] {
>+  -moz-binding: url('chrome://xforms/content/widgets-xhtml.xml#spinbutton');
>+}
>+
>+ /* range type="xsd:time" */
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"] html|span[mozType|spinbutton] { 
>+  -moz-binding: url('chrome://xforms/content/widgets-xhtml.xml#spinbutton');
>+}

I dont like a much many spinbutton bindings, please do:

html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"] html|span[mozType|spinbutton],
html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"] html|span[mozType|spinbutton],
html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"] html|span[mozType|spinbutton] {
  -moz-binding: url('chrome://xforms/content/widgets-xhtml.xml#spinbutton');
}

and etc.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> usually sliders for the types that they support, which are mostly the numerical
> types.  formsPlayer doesn't support all of the types that we eventually will,
> but users sliders for the types that they do support.  XSmiles uses slider for
> numerical types and combines slider with calendar for some date types, I
> believe.  Might be nice to support that at some point (maybe for
> @appearance="full").  But that isn't trivial so we are doing spinbuttons first.
>  To do that we'd probably have to refactor datepicker to some degree.
> 

Ok, in general spin widget looks good for range since spin supports start/end/step. But I'd propose to use for example appearance="minimal" for spin presentation and don't do spin presentation available by default. By default I think it's better to use slider control, it will be go with specs. Then we should stop spin control using for xforms input controls. What do you think?

Comment 34

[aaronr]

(In reply to comment #33)
> (In reply to comment #29)
> > usually sliders for the types that they support, which are mostly the numerical
> > types.  formsPlayer doesn't support all of the types that we eventually will,
> > but users sliders for the types that they do support.  XSmiles uses slider for
> > numerical types and combines slider with calendar for some date types, I
> > believe.  Might be nice to support that at some point (maybe for
> > @appearance="full").  But that isn't trivial so we are doing spinbuttons first.
> >  To do that we'd probably have to refactor datepicker to some degree.
> > 
> 
> Ok, in general spin widget looks good for range since spin supports
> start/end/step. But I'd propose to use for example appearance="minimal" for
> spin presentation and don't do spin presentation available by default. By
> default I think it's better to use slider control, it will be go with specs.
> Then we should stop spin control using for xforms input controls. What do you
> think?
> 

I think that spin button needs to be the default until slider becomes capable of representing data types other than numerical data types.  When that is the case, then I don't mind if spin buttons aren't the default.

Comment 35

[Merle Sterling]

(In reply to comment #31)
> Merle, do you think about xul controls support? I belive your bindings can be
> splitted on two, one is base bingind, other is xhtml binding. Probably in this case you don't need in huge amount of new files.
You would have even more new files. I didn't want to put all of the new bindings in one file because the file would be huge, so I split them into what I consider logical groupings; eg all of the gregorian calendar types are in one file - range-gregorian.

There is some opportunity for new base classes but you would need one new base for each of the 10 new types as there isn't common functionality that applies to all of them. I think that is best done in a follow-on bug as it won't affect the functionality.  I'd like to get this patch and the associated patch from 331987 checked-in quickly and then we can improve it from there.

> >+          this.monthSpin = document.getAnonymousElementByAttribute(this, "anonid", "monthSpin");
> >+          this.daySpin = document.getAnonymousElementByAttribute(this, "anonid", "daySpin");
> >+          this.yearSpin = document.getAnonymousElementByAttribute(this, "anonid", "yearSpin");
> That's not usual practice. Use the next template:
> <property name="monthSpin" readonly="true">
>   <getter>
>     if (!this._monthSpin) {
>       this._monthSpin = this.onwerDocument.getAnonymousElementByAttribute(this, "anonid", "monthSpin");
>     }
>     return this._monthSpin;
>   </getter>
> </property>
> <field name="_monthSpin">null</field>

Done.  I had used that pattern for the spinbutton but didn't carry it over to the new bindings.

Comment 36

[aaronr]

(In reply to comment #35)
> (In reply to comment #31)
> > Merle, do you think about xul controls support? I belive your bindings can be
> > splitted on two, one is base bingind, other is xhtml binding. Probably in this case you don't need in huge amount of new files.
> You would have even more new files. I didn't want to put all of the new
> bindings in one file because the file would be huge, so I split them into what
> I consider logical groupings; eg all of the gregorian calendar types are in one
> file - range-gregorian.
> 
> There is some opportunity for new base classes but you would need one new base
> for each of the 10 new types as there isn't common functionality that applies
> to all of them. I think that is best done in a follow-on bug as it won't affect
> the functionality.  I'd like to get this patch and the associated patch from
> 331987 checked-in quickly and then we can improve it from there.
> 

I appreciate the logic of moving them into seperate files.  I really do.  But at the current pace we are going to have around 10-11 files just for range and that seems crazy, too. I don't mind a reorg being done after we get this functionality in so that we can test this work, fix any bugs and make sure that we have everything working right before we reorg.  But we've at least got to look into possibly reorging this range work even if we eventually decide that things are best left like they are to keep the files a manageable size.

Sounds like perhaps we need a base file (that holds the base binding for all of the different types as well as the range base binding), a -xhtml file for the xhtml specific stuff, and a -xul file for the xul stuff.

Comment 37

[Merle Sterling]

Attachment: patch

Comment 38

[alexander :surkov (:asurkov)]

Merle, this will not go for me until following things are fixed:

1) use(steal) toolkit's spinbuttons (http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/spinbuttons.xml). How they are used you can look at xul's numberbox implementation(http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/numberbox.xml). The advantages are:
  - when we will stop 1.8.x support then we'll just remove our bindings and will use toolkit's analogue.
  - spinbuttons will be grouped together, because if 'down' and 'up' buttons are on different sides of control then it's not accessible (we should look like standard spin widget)
  - spinbuttons will know nothing about parent widget, now your child calls parent methods (child widget should always avoid to call methods of parent of parent if it's possible)

2) Like Aaron said you should have three binding: base, xhtml, and xul. Look, for example, at 'range-date-xhtml'. The base methods should be 'refresh', 'parseStartDate' and so on, the content specific part should be moved into xhtml binding (for example, thease are 'monthSpin' and so on).

I vote for these two items since if you will say we will fix this later then it will involve very big code changes I guess. We'll get the case when we should be forced to refactor your widgets like I did for almost all xforms bindings.

The other issue I can see is you don't use start/end/step properties but you inherited from 'xfromswidget-range-base'. Also I guess you should implement 'getControlElement()' method for your widget. That will be compatible with other rules of xforms widgets writing.

Comment 39

[alexander :surkov (:asurkov)]

Also, please use two spaces before your name in license block. And add bindings for numeric datatypes (we should have spin controls for numeric ranges too). We can rename this bug like 'spin representation of range controls' to better reflect what you do here.

Merle, I filed bug 355198 to stop spins using for input controls. If this bug is targeted to version 0.7 then bug 355198 should be fixed there too. Merle, do you like to take that bug?

Comment 40

[aaronr]

(In reply to comment #39)
> Also, please use two spaces before your name in license block. And add bindings
> for numeric datatypes (we should have spin controls for numeric ranges too). We
> can rename this bug like 'spin representation of range controls' to better
> reflect what you do here.
> 
> Merle, I filed bug 355198 to stop spins using for input controls. If this bug
> is targeted to version 0.7 then bug 355198 should be fixed there too. Merle, do
> you like to take that bug?
> 


I'd say that we NOT put this bug into 0.7.  It is going to take too long to get it in AND flush any bugs out of it.  Plus I'm sure we'll still have follow on bug work to get the UI looking/behaving right.  I'd rather it go into 0.8.

Comment 41

[Merle Sterling]

Attachment: patch

- Refactored new bindings based on range-base binding.
- Base binding for each new type in range.xml.
- Successor bindings for each new type in range-xhtml.

Comment 42

[alexander :surkov (:asurkov)]

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


>+          this.control.monthSpin.label = "Month";
>+          this.control.daySpin.label = "Day";
>+          this.control.yearSpin.label = "Year";

Not localizable things will not go.

Comment 43

[aaronr]

(In reply to comment #42)
> (From update of attachment 242257 [details] [diff] [review] [edit])
> 
> >+          this.control.monthSpin.label = "Month";
> >+          this.control.daySpin.label = "Day";
> >+          this.control.yearSpin.label = "Year";
> 
> Not localizable things will not go.
> 

one way to do this would be to use the string bundle service, just like we use in our cpp code.  An example of a place in mozilla that does this in .js would be: http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#525

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 242257 [details] [diff] [review] [edit] [edit])
> > 
> > >+          this.control.monthSpin.label = "Month";
> > >+          this.control.daySpin.label = "Day";
> > >+          this.control.yearSpin.label = "Year";
> > 
> > Not localizable things will not go.
> > 
> 
> one way to do this would be to use the string bundle service, just like we use
> in our cpp code.  An example of a place in mozilla that does this in .js would
> be:
> http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#525
> 

No. I believe we shouldn't use string bundles in xbl where it's possible. That's property of very big bindings, application level bindings. Firstly it's better to think how to reorganize bindings to use dtd. I think it's possible since year is always year and we can use static element for showing that it's year. The only one reason is dtd looks much readable than string bundles.

Comment 45

[aaronr]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > (From update of attachment 242257 [details] [diff] [review] [edit] [edit] [edit])
> > > 
> > > >+          this.control.monthSpin.label = "Month";
> > > >+          this.control.daySpin.label = "Day";
> > > >+          this.control.yearSpin.label = "Year";
> > > 
> > > Not localizable things will not go.
> > > 
> > 
> > one way to do this would be to use the string bundle service, just like we use
> > in our cpp code.  An example of a place in mozilla that does this in .js would
> > be:
> > http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#525
> > 
> 
> No. I believe we shouldn't use string bundles in xbl where it's possible.
> That's property of very big bindings, application level bindings. Firstly it's
> better to think how to reorganize bindings to use dtd. I think it's possible
> since year is always year and we can use static element for showing that it's
> year. The only one reason is dtd looks much readable than string bundles.
> 


Then we can put them in xforms.dtd and access them like datepicker does: http://lxr.mozilla.org/mozilla/source/extensions/xforms/resources/content/input-xhtml.xml#196

Comment 46

[alexander :surkov (:asurkov)]

1) I think refreshInternal() methods should be defined in range.xml.
2) Merle, probably will you try to fix bug 353880 before this one. If you will add spinbuttons widget firstly, it reduces this patch and should make things clearly. Note, even if you will not use xul's spinbuttons but you should use it's styling. Especially, -moz-appearance style.
3) It looks like widgets-xhtml are excess, probably the things will more easy if you will implement functionality of widget-xhtml bindings in range-xhtml bidings.

Comment 47

[Merle Sterling]

(In reply to comment #46)
> 1) I think refreshInternal() methods should be defined in range.xml.
No. If it were possible to put refreshInternal in range.xml then its contents could be moved into refresh and refreshInternal wouldn't be necessary! refreshInternal is necessary because the bindings for the new types need to do things that the base refresh cannot do. refreshInternal is template method just like adjustRangeValues.

> 2) Merle, probably will you try to fix bug 353880 before this one. If you will add spinbuttons widget firstly, it reduces this patch and should make things clearly. Note, even if you will not use xul's spinbuttons but you should use it's styling. Especially, -moz-appearance style.
No, 353880 is the follow-on bug to improve things and should go in after this patch.  None of the methods or inheritance hieararchy will need to change to improve the spinbuttons. We need help from someone with good CSS skills to improve their appearance or we could rewrite them completely without affecting any of the other widgets for bindings.

> 3) It looks like widgets-xhtml are excess, probably the things will more easy
> if you will implement functionality of widget-xhtml bindings in range-xhtml
> bidings.
No again.  I am forced to use the getControlElement method and all of the other methds and properties in range.xml and therefore I need a widget for each type - that widget is the 'control' element. Previously they were all implemented in range-xtml bindings and were much simpler.  Now instead of 3 getAttribute calls, I have hundreds of lines of code to conform to the pattern established by the refactoring of range.xml - but that's what you wanted.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> (In reply to comment #46)
> > 1) I think refreshInternal() methods should be defined in range.xml.
> No. If it were possible to put refreshInternal in range.xml then its contents
> could be moved into refresh and refreshInternal wouldn't be necessary!
> refreshInternal is necessary because the bindings for the new types need to do
> things that the base refresh cannot do. refreshInternal is template method just
> like adjustRangeValues.

You add many new bindings into range.xml. I mean you should move refreshInternal() there. The reason is you will be able to reuse it for xul implementation.

> > 2) Merle, probably will you try to fix bug 353880 before this one. If you will add spinbuttons widget firstly, it reduces this patch and should make things clearly. Note, even if you will not use xul's spinbuttons but you should use it's styling. Especially, -moz-appearance style.
> No, 353880 is the follow-on bug to improve things and should go in after this
> patch.

What's the difference? You are always can replay it. My point is if this patch will easier then it will be fine.

> None of the methods or inheritance hieararchy will need to change to
> improve the spinbuttons. We need help from someone with good CSS skills to
> improve their appearance or we could rewrite them completely without affecting
> any of the other widgets for bindings.

There are styles in toolkit. Why do we need something special? Do they not work for us?

> > 3) It looks like widgets-xhtml are excess, probably the things will more easy
> > if you will implement functionality of widget-xhtml bindings in range-xhtml
> > bidings.
> No again.  I am forced to use the getControlElement method and all of the other
> methds and properties in range.xml and therefore I need a widget for each type
> - that widget is the 'control' element. Previously they were all implemented in
> range-xtml bindings and were much simpler.  Now instead of 3 getAttribute
> calls, I have hundreds of lines of code to conform to the pattern established
> by the refactoring of range.xml - but that's what you wanted.
> 

Merle, my english is not so clear and I think I'm not always able to say you what I actually mean. I don't like as you these hundreds of line of code. But I didn't remember I said that you should split your bindings on three. I think we should have bindings spinbuttons and bindings for xhtml ranges. I'm not sure you need in huge amount of bindings in widgets-xhtml.xml.

Comment 49

[aaronr]

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

removing from review queue.  New version will be coming with reorg of the bindings.

Comment 50

[alexander :surkov (:asurkov)]

Merle, you can steal idea or even code from bug 92174.

Comment 51

[alexander :surkov (:asurkov)]

Merle, I'd like if your native widgets for range will adress requirements of bug 365907 too.

IIRC bug 92174 suggests the widget that contains text fields for day, month, year and it looks fine. For range I'd suggested to have widget with those three fields with one spinbuttons controls. Spinbuttons control increment/decrement the focused (or last previously focused) field. But for calendar control I need to have widget  that contains month and year fields with spinbuttons controls for every field. So, I think to achive this you should have interface widget (widgets.xml) that will provide increment/decrement methods and at least two content widgets (widgets-xul.xml) (one is for calendar, other(s) is for range. What do you think?

Comment 52

[Merle Sterling]

Alex, I believe the interface I am currently working on will work for you. I sent you an email describing what I am doing to make sure we are on the same page.  Did you happen to change your email address?

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> Alex, I believe the interface I am currently working on will work for you. I
> sent you an email describing what I am doing to make sure we are on the same
> page.  Did you happen to change your email address?
> 

I sent an email to you. But it sounds you did't receive it. I guess IBM banned my mail box :).

Please look at the bug 365907. I use monthyear widget there. Please see is it possible to share code between your widgets and that one.

Comment 54

[Merle Sterling]

Attachment: dateTime range

This patch is for a range of type dateTime.
- Interface for range widgets that use spinbuttons to represent the components of a type (widgets.xml).
- Interface for date and dateTime widgets (widgets.xml)
- Content widget for dateTime (widgets-xhtml.xml)
- range binding for dateTime ranges (range-xhtml.xml)

The 'setSpinbuttonMinMax' method in the range-dateTime widget is rather long and complicated to deal with all of the possible valid date ranges and will be identical for a date range without a time. I need to break this method down into individual setYearMin/Max, setMonthMin/Max, setDayMin/Max etc methods and put them in the date/dateTime interface widget so that xhtml date ranges and non-xhtml controls could use them too without having to duplicate all that code.

Comment 55

[alexander :surkov (:asurkov)]

Comment on attachment 253785 [details] [diff] [review]
dateTime range

It looks that your patch has all to add simply range control for XUL. Please finish XUL support to have complete idea of your approach.


>+  <!-- DATETIME RANGE-->
>+  <binding id="range-datetime"
>+           extends="chrome://xforms/content/widgets.xml#datetime">
>+  
>+    <content>
>+      <children includes="label"/>
>+      <html:table>
>+        <html:body>
>+          <html:tr>
>+            <html:td>
>+              <xul:hbox>
>+                <xul:label value="&xforms.date.year.label;"/>
>+                <xul:textbox type="number" anonid="yearSpin"/>
>+              </xul:hbox>

Also, I'd like if range controls will be compatible with analogies toolkit's widgets.

Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers
Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/

Do we like this?

Comment 56

[alexander :surkov (:asurkov)]

Comment on attachment 253785 [details] [diff] [review]
dateTime range

I cancel review because I hope to have the patch with XUL documents support :) Merle, does it work for you?

Comment 57

[Steve Speicher]

I'd recommend moving forward with XHTML and address XUL in separate bug.  This bug keeps growing.

Comment 58

[Merle Sterling]

(In reply to comment #55)
> (From update of attachment 253785 [details] [diff] [review])
> It looks that your patch has all to add simply range control for XUL. Please
> finish XUL support to have complete idea of your approach.
I also think that xul controls should be done later, if ever, in a separate bug. I believe the interface is sufficient to do non-xhtml controls at a future point but that is not the focus of this bug. 

> >+  <!-- DATETIME RANGE-->
> >+  <binding id="range-datetime"
> >+           extends="chrome://xforms/content/widgets.xml#datetime">
> >+  
> >+    <content>
> >+      <children includes="label"/>
> >+      <html:table>
> >+        <html:body>
> >+          <html:tr>
> >+            <html:td>
> >+              <xul:hbox>
> >+                <xul:label value="&xforms.date.year.label;"/>
> >+                <xul:textbox type="number" anonid="yearSpin"/>
> >+              </xul:hbox>
> Also, I'd like if range controls will be compatible with analogies toolkit's
> widgets.
> Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers
> Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/
> Do we like this?

If the idea is to change from individual spinbuttons for the components of a date to multiple textboxes with one spinbutton, then no we do not like that idea. We've discussed that idea before. We need to stay on track and move forward with the current approach and get it done.

Comment 59

[alexander :surkov (:asurkov)]

(In reply to comment #58)
> (In reply to comment #55)
> > (From update of attachment 253785 [details] [diff] [review] [details])
> > It looks that your patch has all to add simply range control for XUL. Please
> > finish XUL support to have complete idea of your approach.

> I also think that xul controls should be done later, if ever, in a separate
> bug. I believe the interface is sufficient to do non-xhtml controls at a future
> point but that is not the focus of this bug. 

Really XUL support won't grow your patch but it may change your binding a little. Therefore I'd very like to see XUL, it will helps a lot in understanding of your approach. Merle, I believe it wouldn't take a lot of your time because it looks your patch has anything to keep XUL excepting some CSS styles.

Comment 60

[alexander :surkov (:asurkov)]

(In reply to comment #58)

> > Also, I'd like if range controls will be compatible with analogies toolkit's
> > widgets.
> > Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers
> > Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/
> > Do we like this?
> 
> If the idea is to change from individual spinbuttons for the components of a
> date to multiple textboxes with one spinbutton, then no we do not like that
> idea. We've discussed that idea before. We need to stay on track and move
> forward with the current approach and get it done.
> 

I'm fine with this if it works for Aaron and Olli.

Comment 61

[aaronr]

(In reply to comment #59)
> (In reply to comment #58)
> > (In reply to comment #55)
> > > (From update of attachment 253785 [details] [diff] [review] [details] [details])
> > > It looks that your patch has all to add simply range control for XUL. Please
> > > finish XUL support to have complete idea of your approach.
> 
> > I also think that xul controls should be done later, if ever, in a separate
> > bug. I believe the interface is sufficient to do non-xhtml controls at a future
> > point but that is not the focus of this bug. 
> 
> Really XUL support won't grow your patch but it may change your binding a
> little. Therefore I'd very like to see XUL, it will helps a lot in
> understanding of your approach. Merle, I believe it wouldn't take a lot of your
> time because it looks your patch has anything to keep XUL excepting some CSS
> styles.
> 

I agree with Alex on this one.  Supporting XUL version of the range isn't an option.  We've got to do it at some point.  Now is as good a time as any.

Comment 62

[aaronr]

(In reply to comment #60)
> (In reply to comment #58)
> 
> > > Also, I'd like if range controls will be compatible with analogies toolkit's
> > > widgets.
> > > Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers
> > > Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/
> > > Do we like this?
> > 
> > If the idea is to change from individual spinbuttons for the components of a
> > date to multiple textboxes with one spinbutton, then no we do not like that
> > idea. We've discussed that idea before. We need to stay on track and move
> > forward with the current approach and get it done.
> > 
> 
> I'm fine with this if it works for Aaron and Olli.
> 

I agree with Merle on this one.  We've been working on this feature since October.  We've got to get it out there for people to use and to comment on.

Comment 63

[Merle Sterling]

Comment on attachment 253785 [details] [diff] [review]
dateTime range

Created bug 370551 for dateTime range. Current patch and xhtml/xul testcaes are attached there.

Comment 64

[Merle Sterling]

Investigate whether separate increment/decrement handlers can be consolidated into one handler. If a single handler will work for all range types (as it does for dateTime) we could simplify the code a bit.

Comment 65

[Merle Sterling]

Bugs 372736, 372737, and 372739 have been fixed, so this meta bug is fixed.