Last Comment Bug 370551 - Support dateTime types for Range
: Support dateTime types for Range
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
:
Mentors:
Depends on:
Blocks: 316353 372736
  Show dependency treegraph
 
Reported: 2007-02-15 14:01 PST by Merle Sterling
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
dateTime testcase: one year range (1.18 KB, text/xml)
2007-02-15 14:10 PST, Merle Sterling
no flags Details
dateTime testcase: multi-year range (1.30 KB, text/xml)
2007-02-15 14:11 PST, Merle Sterling
no flags Details
XUL dateTime range testcase (1.06 KB, application/vnd.mozilla.xul+xml)
2007-02-15 14:12 PST, Merle Sterling
no flags Details
patch (42.11 KB, patch)
2007-02-15 14:15 PST, Merle Sterling
no flags Details | Diff | Splinter Review
patch (39.33 KB, patch)
2007-02-16 16:55 PST, Merle Sterling
surkov.alexander: review-
Details | Diff | Splinter Review
patch (41.60 KB, patch)
2007-02-22 12:05 PST, Merle Sterling
no flags Details | Diff | Splinter Review
patch (43.95 KB, patch)
2007-02-23 15:09 PST, Merle Sterling
no flags Details | Diff | Splinter Review
tabindex testcase (1.64 KB, application/xhtml+xml)
2007-02-26 16:52 PST, Merle Sterling
no flags Details
patch (44.15 KB, patch)
2007-02-26 16:53 PST, Merle Sterling
surkov.alexander: review+
aaronr: review+
Details | Diff | Splinter Review
final patch (44.80 KB, patch)
2007-03-05 11:39 PST, Merle Sterling
no flags Details | Diff | Splinter Review

Description Merle Sterling 2007-02-15 14:01:17 PST
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.9a3pre) Gecko/20070213 Minefield/3.0a3pre

This bug is to be used for adding dateTime support for Range.

Reproducible: Always
Comment 1 Merle Sterling 2007-02-15 14:10:26 PST
Created attachment 255253 [details]
dateTime testcase: one year range
Comment 2 Merle Sterling 2007-02-15 14:11:43 PST
Created attachment 255254 [details]
dateTime testcase: multi-year range
Comment 3 Merle Sterling 2007-02-15 14:12:31 PST
Created attachment 255255 [details]
XUL dateTime range testcase
Comment 4 Merle Sterling 2007-02-15 14:15:15 PST
Created attachment 255256 [details] [diff] [review]
patch
Comment 5 alexander :surkov 2007-02-16 06:40:25 PST
Please place labels and values of textboxes on the same line especially for XUL.
Comment 6 alexander :surkov 2007-02-16 06:44:20 PST
Also I believe XUL's versions shouldn't use html:table. XUL has box oriented layout. I think you should use it to allow user easy orient his control.
Comment 7 alexander :surkov 2007-02-16 06:47:50 PST
Merle, I'd like to see previous comments be addressed here because fixing of it may change your bindings structure.
Comment 8 Merle Sterling 2007-02-16 10:23:23 PST
The range-dateTime widget is already a hybrid of html and xul because we use xul:spinbuttons and xul:numberbox. It works for both xhtml and xul but alignment of labels and sizing of the numberbox is problematic. Believe me, I tried to get the xhtml and xul to look the same but couldn't quite do it.

If I use css and text-align property (tried 'center') then the start and end labels are centered in xhtml but xul seems to ignore them and places them at the top anyway. xul properties like 'cols', 'size', and 'maxlength' result in numberboxes much larger than we want or in some cases 'unrecognized property'. The Year, Month, Day, etc labels ALWAYS get aligned to the top in both xhtml and xul and I couldn't figure out a way around it. I think it looks fine the way it is anyway and we should concentrate on making sure the code is correct (it is :)) and worry about styling later.

If you want to eliminate html tags for a xul binding (maybe change html:table to xul:vbox and xul:hbox), then we have to duplicate the range-dateTime widget just to change the content. Do you really want to have a duplicate xul widget just to eliminate html tags?

I say we should go with it the way it is and address minor styling issues later.
Comment 9 alexander :surkov 2007-02-16 11:31:17 PST
I don't like any dublication. I guess we can use base interface bindings and XUL and XHTML versions may be extended from it and have content only. 


I didn't test but probably your XUL version may be:

<hbox>
<xul:label value="&xforms.date.year.label;"/>
<xul:textbox type="number" size="3" anonid="yearSpin"/>
</hbox>
 and so on.

At lest it helps with orienting. If it won't help with label aligning then it's up to you to save for following up bug.

Also please specify @control attribute on label that should point on related @anonid attribute, it helps to make textbox more accessible.

Please implement nsIAccessibleProvider for your range widget, accessibleType should be equal with XFormsContainer IIRC.
Comment 10 Merle Sterling 2007-02-16 11:47:38 PST
(In reply to comment #9)
> I don't like any dublication. I guess we can use base interface bindings and
> XUL and XHTML versions may be extended from it and have content only. 
> I didn't test but probably your XUL version may be:
> <hbox>
> <xul:label value="&xforms.date.year.label;"/>
> <xul:textbox type="number" size="3" anonid="yearSpin"/>
> </hbox>
>  and so on.
That's exactly how it is now. I can add the control attribute and use align on the hbox and that seems to work for label aligning.

I don't like duplicating code much either. How about I remove all html tags and use xul only since we already have to use xul:textbox (and spinbuttons)? Then we can move it from widgets-xhtml to widgets-xul and it should work for both xhtml and xul.
Comment 11 alexander :surkov 2007-02-16 12:08:06 PST
Looks good(In reply to comment #10)

> I don't like duplicating code much either. How about I remove all html tags and
> use xul only since we already have to use xul:textbox (and spinbuttons)? Then
> we can move it from widgets-xhtml to widgets-xul and it should work for both
> xhtml and xul.
> 

Looks good.
Comment 12 Merle Sterling 2007-02-16 16:55:43 PST
Created attachment 255398 [details] [diff] [review]
patch

range-datetime widget now using only xul elements. Added CSS to align labels properly for both xul and xhtml.
Comment 13 alexander :surkov 2007-02-18 23:35:02 PST
Comment on attachment 255398 [details] [diff] [review]
patch

>Index: resources/content/range-xhtml.xml

>+  <!-- RANGE: <DATETIME>
>+  -->
>+  <binding id="xformswidget-range-datetime"
>+           extends="chrome://xforms/content/range.xml#xformswidget-range-base">
>+
>+    <content>
>+      <children includes="label"/>
>+      <html:span mozType:range-datetime="true" anonid="control"
>+                 xbl:inherits="start=start,end=end,step=step"/>
>+      <children/>
>+    </content>

Is there reason to bind range widget to html:span? I am confused a bit because we use XUL elements for representation excepting that span.

>+    <implementation implements="nsIAccessibleProvider">
>+
>+      <property name="accessibleType" readonly="true">
>+        <getter>
>+          return Components.interfaces.nsIAccessibleProvider.XFormsDateTimeRange;

The available constants are listed here http://lxr.mozilla.org/mozilla/source/accessible/public/nsIAccessibleProvider.idl#107.
Note there is accessible object for every constant http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1493.
You can't add your constants without accessible module changes. I think XFormsContainer constant will be more suitable here but please file new bug to create accessible object for new range widgets.

>+    <handlers>
>+      <handler event="blur" phase="capturing">
>+        this.updateInstanceData(false);
>+      </handler>

Don't forget to fire DOMFocusIn/DOMFocusOut events here.

More comments later.
Comment 14 alexander :surkov 2007-02-18 23:37:21 PST
Comment on attachment 255398 [details] [diff] [review]
patch


>Index: resources/content/range.xml

>+   -  Merle Sterling <msterlin@us.ibm.com>

>+

Btw, this file has only these two changes. Are they necessary :)?
Comment 15 alexander :surkov 2007-02-19 00:06:54 PST
Comment on attachment 255398 [details] [diff] [review]
patch


>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/widgets-xul.xml,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 widgets-xul.xml
>--- resources/content/widgets-xul.xml	14 Feb 2007 06:06:04 -0000	1.6
>+++ resources/content/widgets-xul.xml	17 Feb 2007 00:53:52 -0000

>-            var evt = document.createEvent("Events");
>-            evt.initEvent("spinup", true, false);
>-            this.dispatchEvent(evt);
>+              var evt = document.createEvent("Events");
>+              evt.initEvent("spinup", true, false);
>+              this.dispatchEvent(evt);

nit: probably it's better  to have method fireEvent(), the code will be more compact.

>+    <content>
>+      <children includes="label"/>
>+      <xul:hbox class="range-box">
>+        <xul:label value="" anonid="minLabel"/>
>+        <xul:label control="yearSpin" value="&xforms.date.year.label;"/>
>+        <xul:textbox type="number" size="3" anonid="yearSpin"/>
>+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
>+        <xul:textbox type="number" size="1" anonid="monthSpin"/>
>+        <xul:label control="daySpin" value="&xforms.date.day.label;"/>
>+        <xul:textbox type="number" size="1" anonid="daySpin"/>
>+        <xul:label control="hoursSpin" value="&xforms.datetime.hours.label;"/>
>+        <xul:textbox type="number" size="1" anonid="hoursSpin"/>
>+        <xul:label control="minutesSpin" value="&xforms.datetime.minutes.label;"/>
>+        <xul:textbox type="number" size="1" anonid="minutesSpin"/>
>+        <xul:label control="secondsSpin" value="&xforms.datetime.seconds.label;"/>
>+        <xul:textbox type="number" size="1" anonid="secondsSpin"/>
>+        <xul:label value="" anonid="maxLabel"/>
>+      </xul:hbox>
>+      <children/>
>+    </content>

Actually, I'd like to see every label+textbox inside hbox, and container should inherit @orient attribute. I think xforsm author should be able to change orientation of range content. Btw, who will have @class="xf-value" attribute? Also, please add support of @tabindex attribute.
Comment 16 alexander :surkov 2007-02-19 00:19:04 PST
Comment on attachment 255398 [details] [diff] [review]
patch

>Index: resources/content/range-xhtml.xml

>+      <method name="getControlElement">
>+        <body>
>+          return {
>+            __proto__: this.ownerDocument.
>+              getAnonymousElementByAttribute(this, "anonid", "control"),

span hans't "right implementation" of focus method but this method is required by range-base binding.

I'll try to put more comments later if they will be :) but I guess you can start to fix mentioned minuses.
Comment 17 Merle Sterling 2007-02-19 13:06:03 PST
(In reply to comment #13)
> (From update of attachment 255398 [details] [diff] [review])
> >Index: resources/content/range-xhtml.xml
> >+  <!-- RANGE: <DATETIME>
> >+  -->
> >+  <binding id="xformswidget-range-datetime"
> >+           extends="chrome://xforms/content/range.xml#xformswidget-range-base">
> >+
> >+    <content>
> >+      <children includes="label"/>
> >+      <html:span mozType:range-datetime="true" anonid="control"
> >+                 xbl:inherits="start=start,end=end,step=step"/>
> >+      <children/>
> >+    </content>
> Is there reason to bind range widget to html:span? I am confused a bit because we use XUL elements for representation excepting that span.
Only because the range binding is in widgets-xhtml and that is how I had it originally. I suppose we can use xul:hbox here instead.

> >+    <implementation implements="nsIAccessibleProvider">
> >+
> >+      <property name="accessibleType" readonly="true">
> >+        <getter>
> >+          return Components.interfaces.nsIAccessibleProvider.XFormsDateTimeRange;
> The available constants are listed here
> http://lxr.mozilla.org/mozilla/source/accessible/public/nsIAccessibleProvider.idl#107.
> Note there is accessible object for every constant
> http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1493.
> You can't add your constants without accessible module changes. I think
> XFormsContainer constant will be more suitable here but please file new bug to create accessible object for new range widgets.
I am aware that we will need another bug for accessiblility. I just made up a new constant similar to the one used in the slider range with the intention of having that constant added in the accessibility bug.

> >+    <handlers>
> >+      <handler event="blur" phase="capturing">
> >+        this.updateInstanceData(false);
> >+      </handler>
> Don't forget to fire DOMFocusIn/DOMFocusOut events here.
> More comments later.

Why? The numeric range binding doesn't fire those events.
Comment 18 Merle Sterling 2007-02-19 13:10:27 PST
(In reply to comment #15)
> (From update of attachment 255398 [details] [diff] [review])
> >RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/widgets-xul.xml,v
> >retrieving revision 1.6
> >diff -u -8 -p -r1.6 widgets-xul.xml
> >--- resources/content/widgets-xul.xml	14 Feb 2007 06:06:04 -0000	1.6
> >+++ resources/content/widgets-xul.xml	17 Feb 2007 00:53:52 -0000
> >-            var evt = document.createEvent("Events");
> >-            evt.initEvent("spinup", true, false);
> >-            this.dispatchEvent(evt);
> >+              var evt = document.createEvent("Events");
> >+              evt.initEvent("spinup", true, false);
> >+              this.dispatchEvent(evt);
> nit: probably it's better  to have method fireEvent(), the code will be more
> compact.
That is fine but really that should have been done in bug353880. Why the comment now?

> >+    <content>
> >+      <children includes="label"/>
> >+      <xul:hbox class="range-box">
> >+        <xul:label value="" anonid="minLabel"/>
> >+        <xul:label control="yearSpin" value="&xforms.date.year.label;"/>
> >+        <xul:textbox type="number" size="3" anonid="yearSpin"/>
> >+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="monthSpin"/>
> >+        <xul:label control="daySpin" value="&xforms.date.day.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="daySpin"/>
> >+        <xul:label control="hoursSpin" value="&xforms.datetime.hours.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="hoursSpin"/>
> >+        <xul:label control="minutesSpin" value="&xforms.datetime.minutes.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="minutesSpin"/>
> >+        <xul:label control="secondsSpin" value="&xforms.datetime.seconds.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="secondsSpin"/>
> >+        <xul:label value="" anonid="maxLabel"/>
> >+      </xul:hbox>
> >+      <children/>
> >+    </content>
> Actually, I'd like to see every label+textbox inside hbox, and container should
> inherit @orient attribute. I think xforsm author should be able to change
> orientation of range content. Btw, who will have @class="xf-value" attribute?
> Also, please add support of @tabindex attribute.

I used to have an hbox around every label+textbox but removed them because I don't think they are necessary. I can put them back but then you know I'll have to add @class=range-box to every single one of them to maintain the alignment. We can do away with the main hbox around all of them and put it in the range binding to address the prior comment about using html:span in the range binding.

Comment 19 Merle Sterling 2007-02-19 13:11:59 PST
(In reply to comment #14)
> (From update of attachment 255398 [details] [diff] [review])
> >Index: resources/content/range.xml
> >+   -  Merle Sterling <msterlin@us.ibm.com>
> >+
> Btw, this file has only these two changes. Are they necessary :)?

Nope, not really necessary. I didn't end up making any changes to the base range binding but I did in other bugs.
Comment 20 alexander :surkov 2007-02-19 19:47:55 PST
(In reply to comment #17)

> > Is there reason to bind range widget to html:span? I am confused a bit because we use XUL elements for representation excepting that span.
> Only because the range binding is in widgets-xhtml and that is how I had it
> originally. I suppose we can use xul:hbox here instead.

So then I'd like to keep the widget in XUL entererly.

> > >+    <implementation implements="nsIAccessibleProvider">
> > >+
> > >+      <property name="accessibleType" readonly="true">
> > >+        <getter>
> > >+          return Components.interfaces.nsIAccessibleProvider.XFormsDateTimeRange;
> > The available constants are listed here
> > http://lxr.mozilla.org/mozilla/source/accessible/public/nsIAccessibleProvider.idl#107.
> > Note there is accessible object for every constant
> > http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1493.
> > You can't add your constants without accessible module changes. I think
> > XFormsContainer constant will be more suitable here but please file new bug to create accessible object for new range widgets.
> I am aware that we will need another bug for accessiblility. I just made up a
> new constant similar to the one used in the slider range with the intention of
> having that constant added in the accessibility bug.

I'd say it's rather to have XFormsContainer constant instead of undefined constant because widget will be accessible partially at least with XFormsContainer constaint. Please file bug for ally support and add XXX comment there.

> > >+    <handlers>
> > >+      <handler event="blur" phase="capturing">
> > >+        this.updateInstanceData(false);
> > >+      </handler>
> > Don't forget to fire DOMFocusIn/DOMFocusOut events here.
> > More comments later.
> 
> Why? The numeric range binding doesn't fire those events.

Why should not they fire these events? I think they should but I'm not sure when. I guess it's Aaron call. If it's not clear what to do and you don't like to deal with these issue in this bug then please file new one add XXX comment.
Comment 21 alexander :surkov 2007-02-19 19:54:59 PST
(In reply to comment #18)

> That is fine but really that should have been done in bug353880. Why the
> comment now?

Because I didn't notice this there :). If it's fine then please fix it.

> 
> > >+    <content>
> > >+      <children includes="label"/>

I wonder why this?

> I used to have an hbox around every label+textbox but removed them because I
> don't think they are necessary. I can put them back but then you know I'll have
> to add @class=range-box to every single one of them to maintain the alignment.
> We can do away with the main hbox around all of them and put it in the range
> binding to address the prior comment about using html:span in the range
> binding.
> 

box around every label+textbox will allow to keep them oriented horizontally. And parent box of those box can inherit orientation from widget. Or you can remove I guess parent box at all. I like to have the following layouts for XUL:

label textbox | label textbox and so on.

label textbox
label textbox
and so on.
Comment 22 alexander :surkov 2007-02-19 20:01:03 PST
(In reply to comment #19)
> (In reply to comment #14)
> > (From update of attachment 255398 [details] [diff] [review] [details])
> > >Index: resources/content/range.xml
> > >+   -  Merle Sterling <msterlin@us.ibm.com>
> > >+
> > Btw, this file has only these two changes. Are they necessary :)?
> 
> Nope, not really necessary. I didn't end up making any changes to the base
> range binding but I did in other bugs.
> 

I do not have problems with your changes if the license block :). If you like to have them then keep them. But why is additional line neeed, does it makes code more clearer?
Comment 23 alexander :surkov 2007-02-19 23:57:51 PST
Comment on attachment 255398 [details] [diff] [review]
patch


>+      <!-- Increment value -->
>+      <method name="increment">
>+        <body>
>+          this.updateSpinbuttons();
>+        </body>
>+      </method>
>+
>+      <!-- Decrement value -->
>+      <method name="decrement">
>+        <body>
>+          this.updateSpinbuttons();
>+        </body>
>+      </method>
>+

>+      <constructor>
>+        // Spinup events are generated by the numberbox widget
>+        // defined in "widgets-xul.xml".
>+        var upHandler = {
>+          range: this,
>+          handleEvent: function(aEvent) {
>+            this.range.increment();
>+          }
>+        };
>+        // Spindown events are generated by the numberbox widgets
>+        // defined in "widgets-xul.xml".
>+        var downHandler = {
>+          range: this,
>+          handleEvent: function(aEvent) {
>+            this.range.decrement();
>+          }
>+        };

It looks you don't need increment()/decrement() methods and you need only one event handler instead upHandler/downHandler handlers.
Comment 24 alexander :surkov 2007-02-20 00:03:02 PST
Comment on attachment 255398 [details] [diff] [review]
patch

Also please fix code styling issues:

>+    <implementation>
>+      <method name="updateLabels">
>+        <body>
>+          <![CDATA[
>+          this.minLabel.value = this.start+" >> ";

add space, should be this.start + " >> ";

>+          this.maxLabel.value = " << "+this.end;

here too.

>+          var value = year+"-"+month+"-"+day+"T"+hours+":"+minutes+":"+seconds;

too

>+          if (!value) {
>+            value = this.start;
>+          }

you don't need to use braces here.

>+          this.setAttribute('value', value);

rather use "" instead ''

>+            }
>+          } else {
>+            // Start and End year span 1 or more years.
>+            if (currentYear == endYear) {
>+              if (currentMonth == endMonth) {
>+                // Current month is the end month of the end year.
>+                // Max day is the day of the end month;
>+                maxDay = this.getDay(this.end);
>+              }
>+              else {
>+                // Current month falls between the first month of the
>+                // current (end) year and the month of the end date.
>+                // May day is the number of days in the month.
>+                maxDay = daysCount;
>+              }
>+            }
>+            else {

please line up 'else' statement.
Comment 25 alexander :surkov 2007-02-20 00:06:14 PST
updateValue() gathers info from fields, then calls value property that updates fields, please fix it.
Comment 26 alexander :surkov 2007-02-20 00:12:13 PST
Probably it's better to call setSpinbuttonMinMax as updateSpinbuttonsState?
Comment 27 alexander :surkov 2007-02-20 00:16:38 PST
Right now I don't have different questions/comments but I'd like to see new version before subsequent review.
Comment 28 Merle Sterling 2007-02-21 10:23:38 PST
(In reply to comment #20)
> So then I'd like to keep the widget in XUL entererly.
Eliminated html:span and just used xul:range-datetime directly because it is a xul widget.

> I'd say it's rather to have XFormsContainer constant instead of undefined
> constant because widget will be accessible partially at least with
> XFormsContainer constaint. Please file bug for ally support and add XXX comment there.
Done. Changed it to XFormsContainer for now.

> Why should not they fire these events? I think they should but I'm not sure
> when. I guess it's Aaron call. If it's not clear what to do and you don't like to deal with these issue in this bug then please file new one add XXX comment.
Done. Not sure they are necessary but it works fine with firing DomFocusIn/Out.
Comment 29 Merle Sterling 2007-02-21 10:28:30 PST
(In reply to comment #21)
> (In reply to comment #18)
> > That is fine but really that should have been done in bug353880. Why the
> > comment now?
> Because I didn't notice this there :). If it's fine then please fix it.
I did fix it. This is another one of those situations where I think I should have a new method but didn't want to change the original code because someone will say on review that I shouldn't have changed it!

> > 
> > > >+    <content>
> > > >+      <children includes="label"/>
> I wonder why this?
Great question. If you read the xbl docs that doesn't seem to apply to ANY of our widgets and yet every one has it.

> box around every label+textbox will allow to keep them oriented horizontally.
> And parent box of those box can inherit orientation from widget. Or you can
> remove I guess parent box at all. I like to have the following layouts for XUL:
> label textbox | label textbox and so on.
> label textbox
> label textbox
> and so on.
That's fine. I do need the parent hbox around all of them as well though. Have to be able to use moz-box-align:end to align the start and end labels in the same way the spinbutton labels are aligned.

Comment 30 Merle Sterling 2007-02-21 10:35:52 PST
(In reply to comment #25)
> updateValue() gathers info from fields, then calls value property that updates fields, please fix it.
??? If you were to set the value directly you must update the fields, adjust the spinbutton min/max and fire the value change event.  If the user spins the spinbuttons, we need to read the new values and do all the same things. updateFields will be called twice but I don't see any way around that unless you don't want to allow the value to be set initially from the bound data.
Comment 31 Merle Sterling 2007-02-21 10:37:37 PST
(In reply to comment #26)
> Probably it's better to call setSpinbuttonMinMax as updateSpinbuttonsState?
I have to make up some name for them method. I think setSpinbuttonMinMax is more descriptive - the name describes exactly what it does. Maybe updateSpinbuttonMinMax instead but I don't particulary like update...STATE.

Comment 32 Merle Sterling 2007-02-21 10:42:00 PST
(In reply to comment #23)
> It looks you don't need increment()/decrement() methods and you need only one
> event handler instead upHandler/downHandler handlers.
rangeDateTime is just one of the many ranges that will use spinbuttons. Being that they all inherit from a single range interface widget we might want to keep them separate. rangeDateTime can do all the work necessary using only the base methods but others may need to do things differently for increment vs decrement and will override those methods. Besides you previously asked specifically for inc/dec methods in the interface.
Comment 33 Merle Sterling 2007-02-21 10:44:50 PST
(In reply to comment #24)
> (From update of attachment 255398 [details] [diff] [review])
> Also please fix code styling issues:
You are killing me. :)

Other than the mis-aligned 'else' I keep the spacing and use of "" vs '' consistent with every other method or property setter/getter.
Comment 34 Merle Sterling 2007-02-21 11:03:18 PST
(In reply to comment #24)
> (From update of attachment 255398 [details] [diff] [review])
> Also please fix code styling issues:
> >+          this.setAttribute('value', value);
> rather use "" instead ''
All other setAttribute calls use '' and so does this one.

> >+            }
> >+          } else {
> >+            // Start and End year span 1 or more years.
> >+            if (currentYear == endYear) {
> >+              if (currentMonth == endMonth) {
> >+                // Current month is the end month of the end year.
> >+                // Max day is the day of the end month;
> >+                maxDay = this.getDay(this.end);
> >+              }
> >+              else {
> >+                // Current month falls between the first month of the
> >+                // current (end) year and the month of the end date.
> >+                // May day is the number of days in the month.
> >+                maxDay = daysCount;
> >+              }
> >+            }
> >+            else {
> please line up 'else' statement.
Did you mean you'd rather see 'else if (currentYear == endYear) {...} rather than the entire block in a single else as I have it? I didn't do that for one reason - it makes it look like the currentYear=endYear is another case equal to startYear==endYear when it is not. If it had been (startYear == endYear) else if (startYear == anotherYear) then it would be logical to continue with 'else if', but that is not the case.

Trust me these methods are extremely complicated. I have the logic worked out to handle every case correctly and I'm not about to mess with it and screw it up.

Comment 35 aaronr 2007-02-21 11:27:56 PST
(In reply to comment #34)
> (In reply to comment #24)
> > (From update of attachment 255398 [details] [diff] [review] [details])
> > Also please fix code styling issues:
> > >+          this.setAttribute('value', value);
> > rather use "" instead ''
> All other setAttribute calls use '' and so does this one.
> 
> > >+            }
> > >+          } else {
> > >+            // Start and End year span 1 or more years.
> > >+            if (currentYear == endYear) {
> > >+              if (currentMonth == endMonth) {
> > >+                // Current month is the end month of the end year.
> > >+                // Max day is the day of the end month;
> > >+                maxDay = this.getDay(this.end);
> > >+              }
> > >+              else {
> > >+                // Current month falls between the first month of the
> > >+                // current (end) year and the month of the end date.
> > >+                // May day is the number of days in the month.
> > >+                maxDay = daysCount;
> > >+              }
> > >+            }
> > >+            else {
> > please line up 'else' statement.
> Did you mean you'd rather see 'else if (currentYear == endYear) {...} rather
> than the entire block in a single else as I have it? I didn't do that for one
> reason - it makes it look like the currentYear=endYear is another case equal to
> startYear==endYear when it is not. If it had been (startYear == endYear) else
> if (startYear == anotherYear) then it would be logical to continue with 'else
> if', but that is not the case.
> 
> Trust me these methods are extremely complicated. I have the logic worked out
> to handle every case correctly and I'm not about to mess with it and screw it
> up.
> 

I think what Alex meant is that you are starting lines with 'else'.  We use the style of:

} else {

not:

}
else {
Comment 36 Merle Sterling 2007-02-21 11:33:23 PST
Ok, got it.
Comment 37 aaronr 2007-02-21 11:36:01 PST
(In reply to comment #29)
> (In reply to comment #21)
> > (In reply to comment #18)
> > > 
> > > > >+    <content>
> > > > >+      <children includes="label"/>
> > I wonder why this?
> Great question. If you read the xbl docs that doesn't seem to apply to ANY of
> our widgets and yet every one has it.

I think what Alex means is that for <binding id="range-datetime"> you are splitting out <children includes="label"/>, but it is impossible for a label to be there in the first place since you don't use a label element in your anonymous content for <binding id="xformswidget-range-datetime">
Comment 38 Merle Sterling 2007-02-21 13:40:58 PST
(In reply to comment #30)
> (In reply to comment #25)
> > updateValue() gathers info from fields, then calls value property that updates fields, please fix it.
> ??? If you were to set the value directly you must update the fields, adjust
> the spinbutton min/max and fire the value change event.  If the user spins the
> spinbuttons, we need to read the new values and do all the same things.
> updateFields will be called twice but I don't see any way around that unless
> you don't want to allow the value to be set initially from the bound data.

Never mind. I'll just call setAttribute("value", value) directly instead of using the value setter.
Comment 39 Merle Sterling 2007-02-22 12:05:14 PST
Created attachment 256063 [details] [diff] [review]
patch

New patch with changes per review comments. Surkov, please check the tabindex stuff - it works properly to tab between the numberboxes but I've found there are a number of different ways to do it and get the same behavior.

I'm leaving the separate increment and decrement handlers for now but will consolidate them into one later if we find that a single handler will work for all of the other range types as well.
Comment 40 alexander :surkov 2007-02-22 21:05:36 PST
Comment on attachment 256063 [details] [diff] [review]
patch

If you're control is completely in XUL then it doesn't make sense to keep it in range-xhtml file.

>+  <!-- RANGE: <DATETIME>
>+  -->
>+  <binding id="xformswidget-range-datetime"
>+           extends="chrome://xforms/content/range.xml#xformswidget-range-base">
>+
>+    <content>
>+      <children includes="label"/>
>+      <xul:range-datetime tabindex="0" class="xf-value" anonid="control"

I'd avoid to use @tabindex on this element, it may put it into tab order navigation. Why do you need it?

Also, I'd avoid to use new tag name (XUL haven't strong spec, once we can get that this tagname is occupied :), I'd avoid to do it without strong reason) especially you didn't add display CSS property for it. Therefore we may have some display problems. Why you don't like xul:box for example?

>+        xbl:inherits="start=start,end=end,step=step, mozType:tabindex=tabindex"/>
>+      <children/>
>+    </content>

It seems you don't use mozType:tabindex at all. I think this attribute should be used in native widget for range control (on xul:textbox).

>+    <handlers>
>+      <handler event="focus" phase="capturing">
>+        if (event.originalTarget == this.control) {

nit: please remove braces around single line 'if' :)

This won't work because I think original target will be html:input but not your control. Though if you have @tabindex on it then this events should be fired. But I'm not sure whether native widget control should be tab nagigable.

>+    <content>
>+      <xul:hbox class="range-box">

I'd suppose to remove this xul:box. It involve -moz-box-orient on .xf-value will work. You can put your class="range-box" on xbl:content but probably it's better to put it on native widget directly.
Comment 41 alexander :surkov 2007-02-22 21:08:49 PST
(In reply to comment #37)
> (In reply to comment #29)
> > (In reply to comment #21)
> > > (In reply to comment #18)
> > > > 
> > > > > >+    <content>
> > > > > >+      <children includes="label"/>
> > > I wonder why this?
> > Great question. If you read the xbl docs that doesn't seem to apply to ANY of
> > our widgets and yet every one has it.
> 
> I think what Alex means is that 

You are absolutely right. Aaron, thank you for translation from me ;) Sometimes you're capable to find much better words than me.
Comment 42 Merle Sterling 2007-02-23 10:37:28 PST
(In reply to comment #40)
> (From update of attachment 256063 [details] [diff] [review])
> If you're control is completely in XUL then it doesn't make sense to keep it in range-xhtml file.
Would you like to create a new range-xul.xml file and put all of the new ranges there?

> >+  <!-- RANGE: <DATETIME>
> >+  -->
> >+  <binding id="xformswidget-range-datetime"
> >+           extends="chrome://xforms/content/range.xml#xformswidget-range-base">
> >+
> >+    <content>
> >+      <children includes="label"/>
> >+      <xul:range-datetime tabindex="0" class="xf-value" anonid="control"
> I'd avoid to use @tabindex on this element, it may put it into tab order
> navigation. Why do you need it?
> >+        xbl:inherits="start=start,end=end,step=step, mozType:tabindex=tabindex"/>
> >+      <children/>
> >+    </content>
> It seems you don't use mozType:tabindex at all. I think this attribute should
> be used in native widget for range control (on xul:textbox).
Just tried to follow the numeric range. I believe mozType:tabindex=tabindex would mean make a mozType:tabindex attribute and assign it the value of tabindex. If the widget itself doesn't have tabindex=0 then what will it inherit? I guess I am rather confused by the tabindex attribute. You don't need it at all because by default tab will move between the numberboxes.

> Also, I'd avoid to use new tag name (XUL haven't strong spec, once we can get
> that this tagname is occupied :), I'd avoid to do it without strong reason)
> especially you didn't add display CSS property for it. Therefore we may have
> some display problems. Why you don't like xul:box for example?
So you can't create a new widget and then use it in the range binding? We need an hbox to align the start and end labels and the labels for each numberbox using moz-box-align so either xul:range-datetime is replaced with a box that has class=range-box and the outer hbox is removed from the range-datetime widget or you have it the way I currently have it. The problem I had was getting the CSS in xforms.css to work when the range binding used an hbox instead of range-datetime.

> >+    <handlers>
> >+      <handler event="focus" phase="capturing">
> >+        if (event.originalTarget == this.control) {
 Though if you have @tabindex on it then this events should be fired.
> But I'm not sure whether native widget control should be tab nagigable.
So, don't use tabindex (per comments above) but you have to use tabindex for the domfocus events to be fired???

> >+    <content>
> >+      <xul:hbox class="range-box">
> I'd suppose to remove this xul:box. It involve -moz-box-orient on .xf-value
> will work. You can put your class="range-box" on xbl:content but probably it's better to put it on native widget directly.
The range control includes start and end labels and they too must be aligned with the same moz-boz-orient property and that is why I have an hbox around all of the other hboxes. We need an hbox somewhere, either here as it is or in the range binding. Note that the hbox around each label+numberbox must also have the class=range-box; otherwise you get the start and end labels aligned one way and the labels for the numberboxes aligned differently.

Comment 43 Merle Sterling 2007-02-23 15:09:58 PST
Created attachment 256222 [details] [diff] [review]
patch

New file range-xul.xml for all of the xul ranges; using xul:box instead of xul:range-datetime for the content of xformswidget-range-datetime; removed hbox around elements in range-datetime widget and updated css rules to use xf-value for styling.
Comment 44 alexander :surkov 2007-02-25 08:08:10 PST
(In reply to comment #42)

> > It seems you don't use mozType:tabindex at all. I think this attribute should
> > be used in native widget for range control (on xul:textbox).
> Just tried to follow the numeric range. I believe mozType:tabindex=tabindex
> would mean make a mozType:tabindex attribute and assign it the value of
> tabindex. If the widget itself doesn't have tabindex=0 then what will it
> inherit? I guess I am rather confused by the tabindex attribute. You don't need
> it at all because by default tab will move between the numberboxes.

You was right when started to use mozType:tabindex=tabindex but you didn't end it properly. You should inherit additionaly tabindex=mozType:tabindex on textbox.

> > Also, I'd avoid to use new tag name (XUL haven't strong spec, once we can get
> > that this tagname is occupied :), I'd avoid to do it without strong reason)
> > especially you didn't add display CSS property for it. Therefore we may have
> > some display problems. Why you don't like xul:box for example?
> So you can't create a new widget and then use it in the range binding?

You can introduce new tagnames for XUL but be care. Here I guess we can just use xul:box.
 
> > >+    <handlers>
> > >+      <handler event="focus" phase="capturing">
> > >+        if (event.originalTarget == this.control) {
>  Though if you have @tabindex on it then this events should be fired.
> > But I'm not sure whether native widget control should be tab nagigable.
> So, don't use tabindex (per comments above) but you have to use tabindex for
> the domfocus events to be fired???

focus/blur events will be fired if your control is focusable, tabindex makes it focusable. In new patch your condition won't be successfull since your control is not focusable.

I think range control shouldn't be focusable itself, only textboxes. But I'm not sure should we fire DOMFocusIn/DOMFocusOut on every focus/blur event from internal textboxes. We need to get Aaron's note.

> The range control includes start and end labels and they too must be aligned
> with the same moz-boz-orient property and that is why I have an hbox around all
> of the other hboxes. We need an hbox somewhere, either here as it is or in the
> range binding. Note that the hbox around each label+numberbox must also have
> the class=range-box; otherwise you get the start and end labels aligned one way
> and the labels for the numberboxes aligned differently.
> 

So if you can use class=range-box on box around label+numberbox then it's fine.

Please let me know if my answers are incomplete or if I missed something.
Comment 45 aaronr 2007-02-26 13:57:48 PST
(In reply to comment #44)
> (In reply to comment #42)
> 
> > > >+    <handlers>
> > > >+      <handler event="focus" phase="capturing">
> > > >+        if (event.originalTarget == this.control) {
> >  Though if you have @tabindex on it then this events should be fired.
> > > But I'm not sure whether native widget control should be tab nagigable.
> > So, don't use tabindex (per comments above) but you have to use tabindex for
> > the domfocus events to be fired???
> 
> focus/blur events will be fired if your control is focusable, tabindex makes it
> focusable. In new patch your condition won't be successfull since your control
> is not focusable.
> 
> I think range control shouldn't be focusable itself, only textboxes. But I'm
> not sure should we fire DOMFocusIn/DOMFocusOut on every focus/blur event from
> internal textboxes. We need to get Aaron's note.
> 

I'd say open a new bug for handling DOMFocusIn and DOMFocusOut for all of the different range widgets once they are done.  Then we can make a decision in that bug for all the different range types.  Then we can write up some testcases and test the other implementations to see what they do.  But I would think that we'd want to handle these events.  Perhaps fire a DOMFocusIn on the range when an underlying widget first gets focus and fire a DOMFocusOut when focus moves somewhere that isn't inside the range.  Otherwise the form author won't be able to write focus action handlers against these controls.
Comment 46 Merle Sterling 2007-02-26 14:26:52 PST
I would agree with a new bug for tabindex and focus events. As it stands now you can tab to each individual numberbox regardless of which combination of tabindex and focus events I use (they all behave the same). 

So can we move on and get this patch checked in so we can continue with the other range types?
Comment 47 Merle Sterling 2007-02-26 16:52:25 PST
Created attachment 256539 [details]
tabindex testcase
Comment 48 Merle Sterling 2007-02-26 16:53:34 PST
Created attachment 256540 [details] [diff] [review]
patch

Adding tabindex support.
Comment 49 alexander :surkov 2007-02-26 17:14:05 PST
Comment on attachment 256540 [details] [diff] [review]
patch

I have only nits.

>+   - The Initial Developer of the Original Code is
>+   - Merle Sterling
>+   - Portions created by the Initial Developer are Copyright (C) 2006

2007

>+    <content>
>+      <children includes="label"/>
>+      <xul:box class="xf-value" anonid="control"
>+        xbl:inherits="start=start,end=end,step=step,mozType:tabindex=tabindex"/>

wrong indentation. I saw wrong attributes indentation below, please fix it too.

Also, Merle, please file bug about DOMFocusIn/Out events.
Comment 50 Merle Sterling 2007-02-27 09:07:34 PST
So which indentation is 'correct'? Line up xbl:inherits with class=xf-value" or line it up with <xul:hbox?

Which 'wrong attributes indentation below'? There are no other attributes in that file, are you talking about a different file?

Comment 51 alexander :surkov 2007-02-27 09:14:04 PST
(In reply to comment #50)
> So which indentation is 'correct'? Line up xbl:inherits with class=xf-value" or
> line it up with <xul:hbox?

with @class

> Which 'wrong attributes indentation below'? There are no other attributes in
> that file, are you talking about a different file?
> 

I'm talking about your patch :) widgets.xml,  <property name="end". Please look precisely probably I missed something like that.
Comment 52 aaronr 2007-03-02 19:13:52 PST
Comment on attachment 256540 [details] [diff] [review]
patch

>Index: resources/content/range-xul.xml
>===================================================================

>+
>+      <method name="isInRange">
>+        <parameter name="aValue"/>
>+        <body>
>+        <![CDATA[
>+          var startDate = this.control.getDateTime(this.start);
>+          var endDate = this.control.getDateTime(this.end);
>+          var currentDate = this.control.getDateTime(aValue);
>+          return startDate <= currentDate && currentDate <= endDate;

nit: store off this.control in a variable so that it isn't re-evaluated 3 times.

>Index: resources/content/range.xml
>===================================================================

>@@ -72,16 +73,17 @@
>           if (this.start > this.end) {
>             this.delegate.reportError("rangeBeginEndError");
>             return;
>           }
> 
>           this.control.readonly = this.accessors.isReadonly();
> 
>           var value = this.accessors.getValue();
>+

nit: extraneous newline

>Index: resources/content/widgets-xul.xml
>===================================================================

>+  <!-- DATETIME RANGE-->
>+  <binding id="range-datetime"
>+           extends="chrome://xforms/content/widgets.xml#datetime">
>+
>+    <content>
>+      <xul:label value="" anonid="minLabel"/>
>+      <xul:box class="xf-value">
>+        <xul:label control="yearSpin" value="&xforms.date.year.label;"/>
>+        <xul:textbox type="number" size="3" anonid="yearSpin"
>+                     xbl:inherits="tabindex=mozType:tabindex"/>
>+      </xul:box>
>+      <xul:box class="xf-value">
>+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
>+        <xul:textbox type="number" size="1" anonid="monthSpin"
>+                     xbl:inherits="tabindex=mozType:tabindex"/>

why xf-value on all of these boxes?  Shouldn't just the top box (contained
inside the range) have this class?  I'd assume that if the top box changed orientation, for example, you'd want these inner boxes to keep their current orientation.  Maybe give the top box and these inner boxes the same class and have the top box have the additional class of xf-value.

>Index: resources/content/widgets.xml
>===================================================================

>@@ -498,9 +499,617 @@
>         this.month++;
>       </handler>
>       <handler event="keypress" keycode="VK_PAGE_DOWN">
>         this.month--;
>       </handler>
>     </handlers>
>   </binding>
> 
>+  <!-- RANGE
>+       Interface for range widgets that use one or more spinbuttons/numberbox
>+       combinations to represent a type. The widget assumes that successor
>+       widgets have the following nterface:

nit: missing 'i' in interface

>+
>+      <!-- Set the min and max attributes for the spinbuttons.
>+          If any one of the spinbuttons/numberboxes changes value,
>+          the valid min and max for one or more of the others may
>+          have to change too.

nit: align comment rows.  Also, please give an example of when the value of one
spinbox's value may affect the min/max of another spinbox.  When reading through this in 6 months, month/day might not immediately pop into my head like it does now.

>+
>+           The range widget updates its value on each 'change'event from the
>+           spinbuttons and then fires ValueChange after they have all been
>+           updated so that the range binding can update the instance data.

nit: missing space between 'change' and 'event'

With these nits and Alex's addressed, r=me
Comment 53 alexander :surkov 2007-03-02 19:22:50 PST
(In reply to comment #52)

> >+      <xul:box class="xf-value">
> >+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
> >+        <xul:textbox type="number" size="1" anonid="monthSpin"
> >+                     xbl:inherits="tabindex=mozType:tabindex"/>
> 
> why xf-value on all of these boxes?  Shouldn't just the top box (contained
> inside the range) have this class?  I'd assume that if the top box changed
> orientation, for example, you'd want these inner boxes to keep their current
> orientation.  Maybe give the top box and these inner boxes the same class and
> have the top box have the additional class of xf-value.

Ah, right, I missed this. It should be defined on outer widget. Thanks for the catch.
Comment 54 Merle Sterling 2007-03-05 10:08:50 PST
(In reply to comment #53)
> (In reply to comment #52)
> > >+      <xul:box class="xf-value">
> > >+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
> > >+        <xul:textbox type="number" size="1" anonid="monthSpin"
> > >+                     xbl:inherits="tabindex=mozType:tabindex"/>
> > 
> > why xf-value on all of these boxes?  Shouldn't just the top box (contained
> > inside the range) have this class?  I'd assume that if the top box changed
> > orientation, for example, you'd want these inner boxes to keep their current
> > orientation.  Maybe give the top box and these inner boxes the same class and
> > have the top box have the additional class of xf-value.
> Ah, right, I missed this. It should be defined on outer widget. Thanks for the catch.

I used xf-value on all of the hboxes so that we only have to change one css rule to change from horizontal to vertical. I would assume that if you want vertical orientation then you would want all of the labels+numberboxes to be vertical as well. If I add a new class in widgets-xul.css (as in a prior patch)and use that class on the individual hboxes then we could have vertically oriented range with each label+numberbox remaining horizontal but would we really want that and is that ok considering you then have to change two css rules?


Comment 55 aaronr 2007-03-05 11:01:48 PST
(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > >+      <xul:box class="xf-value">
> > > >+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
> > > >+        <xul:textbox type="number" size="1" anonid="monthSpin"
> > > >+                     xbl:inherits="tabindex=mozType:tabindex"/>
> > > 
> > > why xf-value on all of these boxes?  Shouldn't just the top box (contained
> > > inside the range) have this class?  I'd assume that if the top box changed
> > > orientation, for example, you'd want these inner boxes to keep their current
> > > orientation.  Maybe give the top box and these inner boxes the same class and
> > > have the top box have the additional class of xf-value.
> > Ah, right, I missed this. It should be defined on outer widget. Thanks for the catch.
> 
> I used xf-value on all of the hboxes so that we only have to change one css
> rule to change from horizontal to vertical. I would assume that if you want
> vertical orientation then you would want all of the labels+numberboxes to be
> vertical as well. If I add a new class in widgets-xul.css (as in a prior
> patch)and use that class on the individual hboxes then we could have vertically
> oriented range with each label+numberbox remaining horizontal but would we
> really want that and is that ok considering you then have to change two css
> rules?
> 

That is what I personally would expect the user to want -> having vertically-aligned spinbuttons with labels to the left.  I don't think the user having to change two CSS rules would be too bad.
Comment 56 Merle Sterling 2007-03-05 11:39:21 PST
Created attachment 257400 [details] [diff] [review]
final patch

Fixing nits on comments; adding class range-box to individual hboxes in dateTime widget.
Comment 57 aaronr 2007-03-05 12:40:05 PST
checked into trunk for msterlin
Comment 58 aaronr 2007-04-23 16:37:55 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

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