Closed Bug 370551 Opened 17 years ago Closed 17 years ago

Support dateTime types for Range

Categories

(Core Graveyard :: XForms, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: msterlin, Assigned: msterlin)

References

Details

(Keywords: fixed1.8.0.12, fixed1.8.1.4)

Attachments

(5 files, 5 obsolete files)

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

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

Reproducible: Always
Assignee: xforms → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #255256 - Flags: review?(surkov.alexander)
Attachment #255256 - Flags: review?(aaronr)
Blocks: 316353
Attachment #255255 - Attachment mime type: application/octet-stream → application/vnd.mozilla.xul+xml
Please place labels and values of textboxes on the same line especially for XUL.
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.
Merle, I'd like to see previous comments be addressed here because fixing of it may change your bindings structure.
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.
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.
(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.
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.
Attachment #255256 - Flags: review?(surkov.alexander)
Attachment #255256 - Flags: review?(aaronr)
Attached patch patch (obsolete) — Splinter Review
range-datetime widget now using only xul elements. Added CSS to align labels properly for both xul and xhtml.
Attachment #255256 - Attachment is obsolete: true
Attachment #255398 - Flags: review?(surkov.alexander)
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 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 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 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.
Attachment #255398 - Flags: review?(surkov.alexander) → review-
(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.
(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.

(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.
(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.
(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.
(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 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 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.
updateValue() gathers info from fields, then calls value property that updates fields, please fix it.
Probably it's better to call setSpinbuttonMinMax as updateSpinbuttonsState?
Right now I don't have different questions/comments but I'd like to see new version before subsequent review.
(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.
(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.

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

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

(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 {
Ok, got it.
(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">
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #255398 - Attachment is obsolete: true
Attachment #256063 - Flags: review?(surkov.alexander)
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.
(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.
(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.

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #256063 - Attachment is obsolete: true
Attachment #256222 - Flags: review?(surkov.alexander)
Attachment #256063 - Flags: review?(surkov.alexander)
(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.
(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.
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?
Attached file tabindex testcase
Attached patch patch (obsolete) — Splinter Review
Adding tabindex support.
Attachment #256222 - Attachment is obsolete: true
Attachment #256540 - Flags: review?(surkov.alexander)
Attachment #256222 - Flags: review?(surkov.alexander)
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.
Attachment #256540 - Flags: review?(surkov.alexander) → review+
Attachment #256540 - Flags: review?(aaronr)
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?

(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 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
Attachment #256540 - Flags: review?(aaronr) → review+
(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.
(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?


(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.
Attached patch final patchSplinter Review
Fixing nits on comments; adding class range-box to individual hboxes in dateTime widget.
Attachment #256540 - Attachment is obsolete: true
Blocks: 372736
Attachment #256539 - Attachment mime type: application/octet-stream → application/xhtml+xml
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: