Last Comment Bug 331987 - support range for undefined @start and/or @end
: support range for undefined @start and/or @end
Status: RESOLVED FIXED
: fixed1.8.0.8, fixed1.8.1.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- minor (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
:
Mentors:
http://www.w3.org/MarkUp/Forms/Test/X...
Depends on: 352462
Blocks: 316353 322255 334603 343525
  Show dependency treegraph
 
Reported: 2006-03-28 07:58 PST by Steve Speicher
Modified: 2016-07-15 14:46 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
test case (1.78 KB, application/xhtml+xml)
2006-03-28 07:59 PST, Steve Speicher
no flags Details
Use default values for missing attributres (10.91 KB, patch)
2006-07-23 17:40 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
patch (12.08 KB, patch)
2006-07-25 12:12 PDT, Merle Sterling
no flags Details | Diff | Splinter Review
test case with couple more tests (WARNING: will take time to load) (2.39 KB, application/xhtml+xml)
2006-07-27 11:37 PDT, Steve Speicher
no flags Details
Calculate values for missing attributes (13.08 KB, patch)
2006-08-09 13:59 PDT, Merle Sterling
no flags Details | Diff | Splinter Review
testcase: start greater than end (error) (1.15 KB, application/xml)
2006-08-15 13:54 PDT, Merle Sterling
no flags Details
testcase: type Decimal (1.79 KB, application/xml)
2006-08-15 13:55 PDT, Merle Sterling
no flags Details
testcase: type Double (1.97 KB, application/xml)
2006-08-15 13:56 PDT, Merle Sterling
no flags Details
testcase: type Float (1.79 KB, application/xml)
2006-08-15 13:57 PDT, Merle Sterling
no flags Details
new patch (13.74 KB, patch)
2006-09-25 18:02 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
patch (5.99 KB, patch)
2006-09-29 14:25 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
patch (8.79 KB, patch)
2006-10-02 14:39 PDT, Merle Sterling
surkov.alexander: review-
Details | Diff | Splinter Review
patch (10.87 KB, patch)
2006-10-05 09:36 PDT, Merle Sterling
no flags Details | Diff | Splinter Review
patch (10.49 KB, patch)
2006-10-05 11:21 PDT, Merle Sterling
surkov.alexander: review+
Details | Diff | Splinter Review
patch (10.88 KB, patch)
2006-10-05 13:00 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
final patch! (11.22 KB, patch)
2006-10-05 13:46 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review

Description Steve Speicher 2006-03-28 07:58:52 PST
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.55  [en] (IBM EVV/3.8/EAK01AGF/LE)
Build Identifier: 

If any of start/end/step are missing, the range control doesn't show.  This is not consistent with other processors.  Perhaps a reasonable guess can be made to show the slider.

This fails test cases: 8.1.7.a, 8.1.7.b, 8.1.7.c

Reproducible: Always
Comment 1 Steve Speicher 2006-03-28 07:59:36 PST
Created attachment 216541 [details]
test case
Comment 2 Allan Beaufour 2006-03-28 10:58:21 PST
(In reply to comment #0)
> Perhaps a reasonable guess can be made to show the slider.

I agree that it would be nice, but how would you propose that start and end be allocated?

(Step should be easier)
Comment 3 Steve Speicher 2006-03-28 11:10:44 PST
(In reply to comment #2)
> I agree that it would be nice, but how would you propose that start and end be
> allocated?
> 
Obviously, if the instance node has a restricted schema type (minInclusive, maxInclusive, minExclusive, maxExclusive) should be used for start and end.

For type="xsd:integer" and step="100", you could guess that start/end should be a multiple of 100 (exact mathematical alogrithm omited).  Though if start="100" and nothing else is specified, could there be a 'dynamic' value for end?  So as the user moves the slider to the default end, the slider could grow another chunk of numbers.  Though that approach can be a bit unnerving as the user is chasing the end.

Just some random thoughts, I can attempt to come up with better defaults if desired.
Comment 4 Allan Beaufour 2006-03-28 11:24:58 PST
(In reply to comment #3)
> (In reply to comment #2)
> > I agree that it would be nice, but how would you propose that start and end be
> > allocated?
> > 
> Obviously, if the instance node has a restricted schema type (minInclusive,
> maxInclusive, minExclusive, maxExclusive) should be used for start and end.

Yes, but problem is that the built-in types have rather large mins and max. So it would make for a very weird range.

And for derived types we have another problem: Bug 316691 :(

> For type="xsd:integer" and step="100", you could guess that start/end should be
> a multiple of 100 (exact mathematical alogrithm omited).  Though if start="100"
> and nothing else is specified, could there be a 'dynamic' value for end?  So as
> the user moves the slider to the default end, the slider could grow another
> chunk of numbers.  Though that approach can be a bit unnerving as the user is
> chasing the end.

Yes, and possibly needing another kind of range control.

I agree that it would be really nice with "guessed" starts and ends, but for built-in types I'm not sure we'll guess right / make sense. If there is no start or end, maybe we should display the range as something else than a "slider". A knob f.x.
Comment 5 aaronr 2006-03-28 12:00:31 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > I agree that it would be nice, but how would you propose that start and end be
> > > allocated?
> > > 
> > Obviously, if the instance node has a restricted schema type (minInclusive,
> > maxInclusive, minExclusive, maxExclusive) should be used for start and end.
> 
> Yes, but problem is that the built-in types have rather large mins and max. So
> it would make for a very weird range.
> 
> And for derived types we have another problem: Bug 316691 :(
> 
> > For type="xsd:integer" and step="100", you could guess that start/end should be
> > a multiple of 100 (exact mathematical alogrithm omited).  Though if start="100"
> > and nothing else is specified, could there be a 'dynamic' value for end?  So as
> > the user moves the slider to the default end, the slider could grow another
> > chunk of numbers.  Though that approach can be a bit unnerving as the user is
> > chasing the end.
> 
> Yes, and possibly needing another kind of range control.
> 
> I agree that it would be really nice with "guessed" starts and ends, but for
> built-in types I'm not sure we'll guess right / make sense. If there is no
> start or end, maybe we should display the range as something else than a
> "slider". A knob f.x.
> 

A knob would rock and would be an ideal solution!  Simpler/quicker to code UI could also use left and right arrows or + and - buttons combined with an input that shows the current value.
Comment 6 Allan Beaufour 2006-03-28 23:52:34 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > I agree that it would be nice, but how would you propose that start and end be
> > > > allocated?
> > > > 
> > > Obviously, if the instance node has a restricted schema type (minInclusive,
> > > maxInclusive, minExclusive, maxExclusive) should be used for start and end.
> > 
> > Yes, but problem is that the built-in types have rather large mins and max. So
> > it would make for a very weird range.
> > 
> > And for derived types we have another problem: Bug 316691 :(
> > 
> > > For type="xsd:integer" and step="100", you could guess that start/end should be
> > > a multiple of 100 (exact mathematical alogrithm omited).  Though if start="100"
> > > and nothing else is specified, could there be a 'dynamic' value for end?  So as
> > > the user moves the slider to the default end, the slider could grow another
> > > chunk of numbers.  Though that approach can be a bit unnerving as the user is
> > > chasing the end.
> > 
> > Yes, and possibly needing another kind of range control.
> > 
> > I agree that it would be really nice with "guessed" starts and ends, but for
> > built-in types I'm not sure we'll guess right / make sense. If there is no
> > start or end, maybe we should display the range as something else than a
> > "slider". A knob f.x.
> > 
> 
> A knob would rock and would be an ideal solution!

I actualy wrote about that, way back when, in the original bug (bug 271044 comment 8), but I can see forgot to create a bug for it... Steps can definately be handled better, f.x. xsd:integer should automatically have "1" as a step if none is given.

> Simpler/quicker to code UI could also use left and right arrows or + and -
> buttons combined with an input that shows the current value.

Heh. Good point. Maybe we should just hack that together for now.
Comment 7 Allan Beaufour 2006-03-30 04:33:29 PST
So: Either we should do a knob, or the input that Aaron proposed.
Comment 8 Steve Speicher 2006-07-18 12:26:37 PDT
Think this needs split into:
 1) A simple mechanism, like XSmiles and formsPlayer, to set a default if attributes are omitted and
 2) An enhancement on the UI to handle unbounded ranges.

So Merle is working on #1 now and will propose patch shortly.  He is also working on validating the start/end/step values if appropriate, i.e. there is a type from bound node to validate against.
Comment 9 Merle Sterling 2006-07-23 17:40:30 PDT
Created attachment 230383 [details] [diff] [review]
Use default values for missing attributres
Comment 10 aaronr 2006-07-24 14:34:24 PDT
Comment on attachment 230383 [details] [diff] [review]
Use default values for missing attributres

>Index: nsXFormsRangeAccessors.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.cpp,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsRangeAccessors.cpp
>--- nsXFormsRangeAccessors.cpp	24 May 2006 03:45:45 -0000	1.3
>+++ nsXFormsRangeAccessors.cpp	24 Jul 2006 00:35:41 -0000
>@@ -36,23 +36,49 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXFormsRangeAccessors.h"
> #include "nsDOMString.h"
> #include "nsIDOMElement.h"
> #include "nsString.h"
> #include "nsXFormsUtils.h"
>+#include "nsIModelElementPrivate.h"
>+#include "nsIXFormsControl.h"
>+#include "nsISchema.h"
> 
> NS_IMPL_ISUPPORTS_INHERITED2(nsXFormsRangeAccessors,
>                              nsXFormsRangeConditionAccessors,
>                              nsIXFormsRangeAccessors, 
>                              nsIClassInfo)
> 
> // nsXFormsRangeAccessors
>+
>+nsXFormsRangeAccessors::nsXFormsRangeAccessors(nsIDelegateInternal* aDelegate,
>+                                               nsIDOMElement*       aElement)
>+                       : nsXFormsRangeConditionAccessors(aDelegate, aElement)
>+{
>+  // Determine the type bound to the range control so we can provide suitable
>+  // default values for the start/step/end attributes if they are missing.
>+  nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(mDelegate));
>+  nsCOMPtr<nsIModelElementPrivate> modelPriv =
>+    nsXFormsUtils::GetModel(mElement);
>+  nsISchemaType *schemaType;
>+  modelPriv->GetTypeForControl(control, &schemaType);
>+
>+  nsCOMPtr<nsISchemaSimpleType> type = do_QueryInterface(schemaType);
>+  PRUint16 simpleTypeValue;
>+  type->GetSimpleType(&simpleTypeValue);
>+
>+  if (nsISchemaSimpleType::SIMPLE_TYPE_BUILTIN == simpleTypeValue) {
>+    nsCOMPtr<nsISchemaBuiltinType> builtinType = do_QueryInterface(type);
>+    builtinType->GetBuiltinType(&mBuiltinType);
>+  }
>+}
>+

Your patch assumes some things that aren't necessarily guarenteed...

1) The bound type will be a simple type.  

It could be complex.  So you should at least check type before you use it.  You also might want to put a XXX where the type is queried that mentions bug 313313 so that once the derived type code is done, it can be used here.  Or open a new bug to make sure that work gets done and make it dependent on bug 313313.  I think I prefer both.  A new bug that says that changes need to be made and a XXX to show where the changes need to go.

2) The bound node's type can't change.

The node's bound type CAN change in a variety of ways.  Through JS the author could change the type on the xf:bind.  Also the author could change the xsi:type that is directly on the bound node by updating the instance data and then calling refresh() so that the changes are reflected in the form.  You'll notice that the control sets the type attribute on the node for every refresh of the control in nsXFormsDelegateStub::Refresh (via SetMozTypeAttribute()).

3) You query the type in the nsXFormsRangeAccessors constructor.

This will probably work in all cases.  However, it is theoretically possible (I believe, though I could be wrong) that the range control's XBL widget could attach and try to run before the control is bound to its instance node.  Probably not a big deal, though, since #2 may make you change how you set up mBuiltinType anyhow.


>@@ -62,40 +88,161 @@ nsXFormsRangeAccessors::AttributeGetter(
>   }
> 
>   return NS_OK;
> }
> 
> 
> // nsIXFormsRangeElement
> 
>-// XXX this should do a max(type.minumum, @start)
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeStart(nsAString &aMin)
> {
>-  return AttributeGetter(NS_LITERAL_STRING("start"), aMin);
>+  nsresult rv;
>+  
>+  AttributeGetter(NS_LITERAL_STRING("start"), aMin);
>+  if (aMin.IsEmpty()) {
>+    rv = GetDefaultRangeStart(aMin);
>+    if (NS_SUCCEEDED(rv)) {
>+      // Warn that we are using a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeStart"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+    else {
>+      // Warn that we cannot provide a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeStartInvalid"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+  }
>+  return rv;
> }

Here and in other places in your patch, couldn't you set a nsAutoString to be the NS_LITERAL_STRING and then  in each function just have the one call to ReportError?

> 
>-// XXX this should do min(type.maximu, @end)
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeEnd(nsAString &aMax)
> {
>-  return AttributeGetter(NS_LITERAL_STRING("end"), aMax);
>+  nsresult rv;
>+
>+  AttributeGetter(NS_LITERAL_STRING("end"), aMax);
>+  if (aMax.IsEmpty()) {
>+    rv = GetDefaultRangeEnd(aMax);
>+    if (NS_SUCCEEDED(rv)) {
>+      // Warn that we are using a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeEnd"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+    else {
>+      // Warn that we cannot provide a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeEndInvalid"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+  }
>+  return rv;
> }
> 
>-// XXX if step is not set, it should be set to something "smart" and also
>-// needs to be something that is valid for the given type. This could be
>-// pushed to the widget though.
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeStep(nsAString &aStep)
> {
>-  return AttributeGetter(NS_LITERAL_STRING("step"), aStep);
>+  nsresult rv;
>+
>+  AttributeGetter(NS_LITERAL_STRING("step"), aStep);
>+  if (aStep.IsEmpty()) {
>+    rv = GetDefaultRangeStep(aStep);
>+    if (NS_SUCCEEDED(rv)) {
>+      // Warn that we are using a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeStep"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+    else {
>+      // Warn that we cannot provide a default value.
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("warnRangeStepInvalid"),
>+                                 mElement, nsIScriptError::warningFlag);
>+    }
>+  }
>+  return rv;
> }
> 
> 
>+NS_IMETHODIMP
>+nsXFormsRangeAccessors::GetDefaultRangeStart(nsAString &aMin)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (mBuiltinType) {
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INTEGER:
>+      aMin = NS_LITERAL_STRING("1");
>+      break;
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DOUBLE:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DECIMAL:
>+      aMin = NS_LITERAL_STRING("1.0");
>+      break;
>+
>+    default:
>+      // A type for which we cannot provide a suitable default.
>+      rv = NS_ERROR_FAILURE;
>+      break;
>+  }
>+
>+  return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsRangeAccessors::GetDefaultRangeEnd(nsAString &aMax)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (mBuiltinType) {
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INTEGER:
>+      aMax = NS_LITERAL_STRING("10");
>+      break;
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DOUBLE:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DECIMAL:
>+      aMax = NS_LITERAL_STRING("10.0");
>+      break;
>+
>+    default:
>+      // A type for which we cannot provide a suitable default.
>+      rv = NS_ERROR_FAILURE;
>+      break;
>+  }
>+
>+  return rv;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsRangeAccessors::GetDefaultRangeStep(nsAString &aStep)
>+{
>+  nsresult rv = NS_OK;
>+
>+  switch (mBuiltinType) {
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INTEGER:
>+      aStep = NS_LITERAL_STRING("1");
>+      break;
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DOUBLE:
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DECIMAL:
>+      aStep = NS_LITERAL_STRING("1.0");
>+      break;
>+
>+    default:
>+      // A type for which we cannot provide a suitable default.
>+      rv = NS_ERROR_FAILURE;
>+      break;
>+  }
>+
>+  return rv;
>+}
>+
> // nsIClassInfo implementation
> 
> static const nsIID sScriptingIIDs[] = {
>   NS_IXFORMSACCESSORS_IID,
>   NS_IXFORMSRANGECONDITIONACCESSORS_IID,
>   NS_IXFORMSRANGEACCESSORS_IID
> };
> 
>Index: nsXFormsRangeAccessors.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.h,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsRangeAccessors.h
>--- nsXFormsRangeAccessors.h	24 May 2006 03:45:45 -0000	1.3
>+++ nsXFormsRangeAccessors.h	24 Jul 2006 00:35:41 -0000
>@@ -50,25 +50,45 @@ class nsXFormsRangeAccessors : public ns
> public:
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSIXFORMSRANGEACCESSORS
>   NS_FORWARD_NSIXFORMSRANGECONDITIONACCESSORS(nsXFormsRangeConditionAccessors::)
>   NS_FORWARD_NSIXFORMSACCESSORS(nsXFormsAccessors::)
> 
>   // Constructor
>   nsXFormsRangeAccessors(nsIDelegateInternal* aDelegate,
>-                         nsIDOMElement* aElement)
>-    : nsXFormsRangeConditionAccessors(aDelegate, aElement)
>-  {
>-  }
>+                         nsIDOMElement* aElement);
> 
>   // nsIClassInfo overrides
>   NS_IMETHOD GetInterfaces(PRUint32 *aCount, nsIID * **aArray);
> 
> protected:
>   /**
>    * Gets the value of an attribute on the element (mElement).
>    *
>    * @param aAttr        The attribute
>    * @param aVal         The returned value ("DOMNull"s it if it's not there or empty)
>    */
>   nsresult AttributeGetter(const nsAString &aAttr, nsAString &aVal);
>+
>+  /**
>+   * Get the default range start value based on the xsi:type attribute.
>+   *
>+   * @param aMin         The returned value
>+   */
>+  NS_IMETHODIMP GetDefaultRangeStart(nsAString &aMin);
>+

Here (and in other comments below) you mention it is based on the xsi:type attribute.  The bound node's type can be determined in 3 different ways...@type on xf:bind, xsi:type, and through inline or external schema applied to the model that the instance data lives.


>+  /**
>+   * Get the default range end value based on the xsi:type attribute.
>+   *
>+   * @param aMax         The returned value
>+   */
>+  NS_IMETHODIMP GetDefaultRangeEnd(nsAString &aMax);
>+
>+  /**
>+   * Get the default range step value based on the xsi:type attribute.
>+   *
>+   * @param aSteo         The returned value
>+   */
>+  NS_IMETHODIMP GetDefaultRangeStep(nsAString &aStep);
>+
>+  PRUint16 mBuiltinType;
> };
>Index: resources/locale/en-US/xforms.properties
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v
>retrieving revision 1.31
>diff -u -8 -p -r1.31 xforms.properties
>--- resources/locale/en-US/xforms.properties	24 May 2006 05:29:30 -0000	1.31
>+++ resources/locale/en-US/xforms.properties	24 Jul 2006 00:35:41 -0000
>@@ -80,16 +80,22 @@ duplicateSchema      = XForms Error (40)
> # Warning Messages:
> warnSOAP                  = XForms Warning (1): You are using the SOAP post feature, which is an experimental feature! Beware that the functionality might change, and forms may stop working at any time.
> warnMailtoBodyParam       = XForms Warning (2): The submission action uri already contains a body parameter.
> instDocumentInvalid       = XForms Warning (3): Instance document did not validate.
> warnSubmitAlreadyRunning  = XForms Warning (4): Submission is already running.
> warnSubmitInvalidNode     = XForms Warning (5): Submission validation failed for node
> warnSubmitSerializeFailed = XForms Warning (6): Submission failed to serialize data
> warnSubmitNetworkFailure  = XForms Warning (7): Submission could not send data to the network
>+warnRangeStart            = XForms Warning (8): Range start attribute not specified; using default value
>+warnRangeStep             = XForms Warning (9): Range step attribute not specified; using default value
>+warnRangeEnd              = XForms Warning (10): Range end attribute not specified; using default value
>+warnRangeStartInvalid     = XForms Warning (11): Range start attribute not specified and no suitable default is available
>+warnRangeStepInvalid      = XForms Warning (12): Range step attribute not specified and no suitable default is available
>+warnRangeEndInvalid       = XForms Warning (13): Range end attribute not specified and no suitable default is available

good messages!  These will definitely be helpful to form authors.
Comment 11 Merle Sterling 2006-07-25 12:12:05 PDT
Created attachment 230615 [details] [diff] [review]
patch

Remove the code to determine the bound type from the constructor and Add an interface 'refreshType' to be called from the refresh method in range.xml.
Comment 12 aaronr 2006-07-25 17:32:31 PDT
Comment on attachment 230615 [details] [diff] [review]
patch


>Index: nsXFormsRangeAccessors.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.cpp,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsRangeAccessors.cpp
>--- nsXFormsRangeAccessors.cpp	24 May 2006 03:45:45 -0000	1.3
>+++ nsXFormsRangeAccessors.cpp	25 Jul 2006 19:08:30 -0000

>+NS_IMETHODIMP
>+nsXFormsRangeAccessors::RefreshType()
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(mDelegate));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIModelElementPrivate> modelPriv =
>+    nsXFormsUtils::GetModel(mElement);
>+  nsISchemaType *schemaType;
>+  modelPriv->GetTypeForControl(control, &schemaType);
>+
>+  nsCOMPtr<nsISchemaSimpleType> type = do_QueryInterface(schemaType);
>+  NS_ENSURE_SUCCESS(rv, rv);

Ummm, you are testing rv again?  When did it change? I think that what you meant to do was do_QueryInterface(schemaType, &rv);

>+
>+  PRUint16 simpleTypeValue;
>+  type->GetSimpleType(&simpleTypeValue);
>+
>+  // XXX We only support simple types defined in the spec but may want to allow
>+  // types derived from those simple types in the future.

I'd move this comment above the QI for nsISchemaSimpleType.

>+  if (nsISchemaSimpleType::SIMPLE_TYPE_BUILTIN == simpleTypeValue) {
>+    nsCOMPtr<nsISchemaBuiltinType> builtinType = do_QueryInterface(type);
>+    NS_ENSURE_SUCCESS(rv, rv);

again, you are ensuring rv, but there is not change to rv since the last test.

>+    builtinType->GetBuiltinType(&mBuiltinType);
>+  }
>+}

don't you need to return something at the end to avoid compile warnings?

> 
> // nsIClassInfo implementation
> 
> static const nsIID sScriptingIIDs[] = {
>   NS_IXFORMSACCESSORS_IID,
>   NS_IXFORMSRANGECONDITIONACCESSORS_IID,
>   NS_IXFORMSRANGEACCESSORS_IID
> };
>Index: nsXFormsRangeAccessors.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.h,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsRangeAccessors.h
>--- nsXFormsRangeAccessors.h	24 May 2006 03:45:45 -0000	1.3
>+++ nsXFormsRangeAccessors.h	25 Jul 2006 19:08:30 -0000
>@@ -66,9 +66,32 @@ public:
> protected:
>   /**
>    * Gets the value of an attribute on the element (mElement).
>    *
>    * @param aAttr        The attribute
>    * @param aVal         The returned value ("DOMNull"s it if it's not there or empty)
>    */
>   nsresult AttributeGetter(const nsAString &aAttr, nsAString &aVal);
>+
>+  /**
>+   * Get the default range start value based on the xsi:type attribute.
>+   *
>+   * @param aMin         The returned value
>+   */
>+  nsresult GetDefaultRangeStart(nsAString &aMin);
>+
>+  /**
>+   * Get the default range end value based on the xsi:type attribute.
>+   *
>+   * @param aMax         The returned value
>+   */
>+  nsresult GetDefaultRangeEnd(nsAString &aMax);
>+
>+  /**
>+   * Get the default range step value based on the xsi:type attribute.
>+   *
>+   * @param aStep         The returned value
>+   */
>+  nsresult GetDefaultRangeStep(nsAString &aStep);
>+

Your comments still have xsi:type in them.  If nothing else, replace xsi:type with mozType:type

>+  PRUint16 mBuiltinType;
> };
>Index: resources/content/range.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/range.xml,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 range.xml
>--- resources/content/range.xml	30 May 2006 09:30:31 -0000	1.4
>+++ resources/content/range.xml	25 Jul 2006 19:08:30 -0000
>@@ -371,16 +371,17 @@
> 
>       <method name="refresh">
>         <body>
>           <![CDATA[
>           if (!this.isInitialized) {
>             if (!this.delegate) {
>               return;
>             }
>+            this.accessors.refreshType();
>             var labelBegin = document.getAnonymousElementByAttribute(this, "anonid", "labelBegin");
>             var labelEnd = document.getAnonymousElementByAttribute(this, "anonid", "labelEnd");
>             var canvas = document.getAnonymousElementByAttribute(this, "anonid", "canvas");
>             this.isInitialized = this.createRange(canvas, labelBegin, labelEnd,
>                                                   this.accessors.getRangeStart(),
>                                                   this.accessors.getRangeEnd(),
>                                                   this.accessors.getRangeStep());
>           }

sorry, I mis-read this function before.  Looks like we are only creating the range once.  Which makes sense.  But since the bound type could change between refreshes, I think we should call refreshType on every refresh (not just when !isInitialized) and if we ever notice the type change, then we need to re-create the range.  So I guess I'm suggesting refreshType now return whether the builtin type changed (as a boolean) and test for that AND !isInitialized.


That's all I noticed and most of my whining just involves nits.  One more patch should do it.  Thanks for your patience Merle!
Comment 13 Steve Speicher 2006-07-27 11:37:49 PDT
Created attachment 230942 [details]
test case with couple more tests (WARNING: will take time to load)

When attempting to verify against the 3 test cases that this was originally written against, I found these 2 problems:
  1) if only start is specified and it is greater than end default, no range is displayed.  Should probably set a default end of start+end_default.
  2) if only end is specified, to a large number, then a wide range is drawn and takes a large amount of time.  For example, end=5000 and no start/step is specified.  Perhaps start should be end-end_default.  This also brings up the question on whether there should be a limit to the number of "steps" drawn, as this consumes a fair amount of resources.

For test cases in the test suite see:
  http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.1/8.1.7/8.1.7.a.xhtml
  http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.1/8.1.7/8.1.7.b.xhtml
  http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt8/8.1/8.1.7/8.1.7.c.xhtml
Comment 14 Merle Sterling 2006-08-09 13:59:48 PDT
Created attachment 232973 [details] [diff] [review]
Calculate values for missing attributes

Created new method in range.xml (CalculateRangeValues) to calculate suitable
values for start/step/end attributes that were omitted from the range control on the form.  The range control is limited to a maximum of 10 'steps' (of any size) and missing start/end/step values are adjusted accordingly.  The only condition which is an error is if the start value is greater than the end value.
Comment 15 Steve Speicher 2006-08-10 10:47:51 PDT
I verified that this does pass the 3 test suite cases that failed previously.  I also verified that it works in my env with attached testcase.  Thanks Merle.
Comment 16 Merle Sterling 2006-08-15 13:54:04 PDT
Created attachment 233847 [details]
testcase: start greater than end (error)
Comment 17 Merle Sterling 2006-08-15 13:55:46 PDT
Created attachment 233848 [details]
testcase: type Decimal
Comment 18 Merle Sterling 2006-08-15 13:56:27 PDT
Created attachment 233849 [details]
testcase: type Double
Comment 19 Merle Sterling 2006-08-15 13:57:09 PDT
Created attachment 233850 [details]
testcase: type Float
Comment 20 Merle Sterling 2006-08-15 14:01:00 PDT
If the patch for this bug is going to be merged with the patch for bug 321311 then that patch must pass the attached test cases as well as those linked in comment #13 in order to pass the tests in the test suite.
Comment 21 aaronr 2006-08-15 14:16:21 PDT
Comment on attachment 232973 [details] [diff] [review]
Calculate values for missing attributes

removing code review request since this will be fixed in a patch for a different bug.
Comment 22 alexander :surkov 2006-08-15 20:24:44 PDT
I'd like to miss xul things. So, I guess it's better to fix this bug and bug 331987 together instead bug 321311.
Comment 23 alexander :surkov 2006-08-15 20:26:05 PDT
(In reply to comment #22)
> I'd like to miss xul things. So, I guess it's better to fix this bug and bug
> 331987 together instead bug 321311.
> 

Sorry, I should say "to fix this bug and bug 348439 together".
Comment 24 alexander :surkov 2006-08-16 10:58:40 PDT
Merle, you said "Calculate suitable values for start/end/step if any are omitted or out of range". Sorry I can't see any specific code for out-of-range case in calculateRangeValues() method. So what's wrong?
Comment 25 Merle Sterling 2006-08-16 12:27:09 PDT
(In reply to comment #24)
> Merle, you said "Calculate suitable values for start/end/step if any are
> omitted or out of range". Sorry I can't see any specific code for out-of-range
> case in calculateRangeValues() method. So what's wrong?

I meant out of range in a generic sense; eg. if start=1 and end=50000, any step other than 5,000 would be 'out of range' because we agreed to limit the total range to 10 steps. The patch mainly deals with missing attributes and calculating reasonable defaults.
Comment 26 alexander :surkov 2006-08-16 20:45:50 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > Merle, you said "Calculate suitable values for start/end/step if any are
> > omitted or out of range". Sorry I can't see any specific code for out-of-range
> > case in calculateRangeValues() method. So what's wrong?
> 
> I meant out of range in a generic sense; eg. if start=1 and end=50000, any step
> other than 5,000 would be 'out of range' because we agreed to limit the total
> range to 10 steps. The patch mainly deals with missing attributes and
> calculating reasonable defaults.
> 

So, I guess it's better don't use 'our of range' term in that case, it can confuse somebody like me :).
And I don't think it makes sense to pass arguments to calculateRangeValues() and probably 'calculate' is not very well-turned name, probably it's better setOmittedValues() or something else?
Comment 27 Merle Sterling 2006-08-17 07:57:42 PDT
Well you can't please everybody. :)

I discussed the name of the method and whether or not to pass the parameters to it with Aaron in an attempt to avoid your very concerns during the review process. 'setDefaultValues' might be a more descriptive name for the method although much 'calculating' is done so CalculateRangeValues seems reasonable to me as well.
Comment 28 alexander :surkov 2006-08-17 18:51:43 PDT
(In reply to comment #27)
> Well you can't please everybody. :)

Sorry? (the word 'please' has many translations for Russian, I'm loosing the sense :).

> I discussed the name of the method and whether or not to pass the parameters to
> it with Aaron in an attempt to avoid your very concerns during the review
> process.

If you and Aaron like it then for sure there are not problems :), that's just thoughts about nits.

 'setDefaultValues' might be a more descriptive name for the method
> although much 'calculating' is done so CalculateRangeValues seems reasonable to
> me as well.
> 

'Calculating' is just calculating but CalculateRangeValues calculates and sets value. I don't like setDefaultValues since it sounds for me like use default values even if they are specified.

But again, it's up to you.
Comment 29 Merle Sterling 2006-09-25 18:02:12 PDT
Created attachment 240081 [details] [diff] [review]
new patch

Merged the code from the original patch with the latest version of range.xml (from bug 352462).
Comment 30 alexander :surkov 2006-09-25 19:51:18 PDT
Comment on attachment 240081 [details] [diff] [review]
new patch

>+  /** Determine the type bound to the range control
>+   *
>+   *  @return PR_TRUE if the type has changed; otherwise PR_FALSE
>+   */
>+  PRBool refreshType();

I'm not sure you should adjust range arguments only if bound type was changed. I guess you should update them if at least one argument is not defined or on every refresh because now you don't handle range arguments changes dynamically.

Btw, I guess adjustRangeValues looks better than previous version :).
Comment 31 aaronr 2006-09-27 15:40:05 PDT
Comment on attachment 240081 [details] [diff] [review]
new patch


>Index: nsXFormsRangeAccessors.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsRangeAccessors.cpp,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsXFormsRangeAccessors.cpp
>--- nsXFormsRangeAccessors.cpp	24 May 2006 03:45:45 -0000	1.3
>+++ nsXFormsRangeAccessors.cpp	26 Sep 2006 00:58:53 -0000

>@@ -62,39 +67,72 @@ nsXFormsRangeAccessors::AttributeGetter(
>   }
> 
>   return NS_OK;
> }
> 
> 
> // nsIXFormsRangeElement
> 
>-// XXX this should do a max(type.minumum, @start)
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeStart(nsAString &aMin)
> {
>   return AttributeGetter(NS_LITERAL_STRING("start"), aMin);
> }
> 
>-// XXX this should do min(type.maximu, @end)
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeEnd(nsAString &aMax)
> {
>   return AttributeGetter(NS_LITERAL_STRING("end"), aMax);
> }
> 
>-// XXX if step is not set, it should be set to something "smart" and also
>-// needs to be something that is valid for the given type. This could be
>-// pushed to the widget though.
> NS_IMETHODIMP
> nsXFormsRangeAccessors::GetRangeStep(nsAString &aStep)
> {
>   return AttributeGetter(NS_LITERAL_STRING("step"), aStep);
> }
> 
>+NS_IMETHODIMP
>+nsXFormsRangeAccessors::RefreshType(PRBool *aResult)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIXFormsControl> control(do_QueryInterface(mDelegate, &rv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIModelElementPrivate> modelPriv =
>+    nsXFormsUtils::GetModel(mElement);
>+  nsISchemaType *schemaType;
>+  modelPriv->GetTypeForControl(control, &schemaType);
>+
>+  // Assume the bound type has not changed.
>+  *aResult = PR_FALSE;
>+
>+  // XXX We only support simple types defined in the spec but may want to allow
>+  // types derived from those simple types in the future.

What if you changed this to use GetBoundBuiltinType (lives in nsXFormsControlStubBase which range inherits from)?  But you won't see a change if the type changes from one thing that inherits from decimal to another thing that inherits from decimal.  GetBoundBuiltinType would return decimal in both cases.


>Index: resources/content/range.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/range.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 range.xml
>--- resources/content/range.xml	25 Sep 2006 09:59:13 -0000	1.7
>+++ resources/content/range.xml	26 Sep 2006 00:58:53 -0000
>@@ -18,16 +18,17 @@
>    - The Initial Developer of the Original Code is
>    - Novell, Inc.
>    - Portions created by the Initial Developer are Copyright (C) 2005
>    - the Initial Developer. All Rights Reserved.
>    -
>    - Contributor(s):
>    -  Allan Beaufour <abeaufour@novell.com>
>    -  Alexander Surkov <surkov.alexander@gmail.com>
>+   -  Merle Sterling <msterlin@us.ibm.com>
>    -
>    - Alternatively, the contents of this file may be used under the terms of
>    - either the GNU General Public License Version 2 or later (the "GPL"), or
>    - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>    - in which case the provisions of the GPL or the LGPL are applicable instead
>    - of those above. If you wish to allow use of your version of this file only
>    - under the terms of either the GPL or the LGPL, and not to allow others to
>    - use your version of this file under the terms of the MPL, indicate your
>@@ -61,16 +62,22 @@
>         <getter>
>           return Components.interfaces.nsIAccessibleProvider.XFormsRange;
>         </getter>
>       </property>
> 
>       <method name="refresh">
>         <body>
>         <![CDATA[
>+          if (this.accessors.refreshType()) {
>+            // The start/end/step attrs are all optional and if any are omitted
>+            // we need to adjust their values.
>+            this.adjustRangeValues(this.start, this.end, this.step);
>+          }
>+
>           this.control.readonly = this.accessors.isReadonly();
> 
>           var value = parseFloat(this.accessors.getValue());
>           if (!value)
>             return;
> 
>           var inrange = this.start >= value && value <= this.end;
>           this.accessors.setInRange(inrange);
>@@ -133,12 +140,99 @@
>         <body>
>           if (!this.accessors.hasBoundNode())
>             return;
> 
>           if (!aIncremental || this.incremental)
>             this.accessors.setValue(this.control.value);
>         </body>
>       </method>
>+
>+      <!-- The range control will have a maximum of 10 steps and the
>+           start/end/step values will be adjusted accordingly if any
>+           are omitted.
>+      -->
>+      <method name="adjustRangeValues">
>+        <parameter name="aStart"/>
>+        <parameter name="aEnd"/>
>+        <parameter name="aStep"/>
>+        <body>

You can do the below now or wait until your next patch for the multiple data type support (but in that case, you should put an XXX here with the bug number where it will be fixed).  You really need to make the adjustRangeValues here in the base binding do nothing or have it return ERROR_NOT_IMPLEMENTED.  It shouldn't default to decimal values.  You really need to force the extending bindings to implement this function.  And to that goal, when you eventually move this code, you'll need to put the rules here that the extending bindings need to enforce.  Mostly generalizing your comments below into some rules/guidance.



>+            else {
>+              // Both Start and End are specified, but the specified Step
>+              // value may not be suitable for maintaining the maximum
>+              // range of 10 steps.
>+              if ((aEnd - aStart) / aStep > 10) {
>+                // Honor the specified start and end values, but adjust
>+                // the Step value.
>+                this.step = Math.round((aEnd - aStart) / 10);
>+              }

The more I think about it, if @start and @end are specified, we should try to honor a user-specified value for @step unless it is some totally unreasonable request.  And I don't think that if @start and @end are 1000 apart that @step of 50 (meaning 20 'ticks' along the slider) would be unreasonable.  At least change the '10' to 100 or 1000.  And put a comment in here why we are capping the value.  It is really only because of slider performance.  If someone comes along and uses a spin button widget instead, what was once unreasonable might not be any longer.

I'm r-'ing for now to get it off of my queue.  If you decide to just add comments and do the other work in later bugs, lemme know and I'll r+ it.  Nothing wrong with the code while we are only supporting decimal types.
Comment 32 alexander :surkov 2006-09-28 09:02:53 PDT
(In reply to comment #31)

> I'm r-'ing for now to get it off of my queue.  If you decide to just add
> comments and do the other work in later bugs, lemme know and I'll r+ it. 
> Nothing wrong with the code while we are only supporting decimal types.
> 

I really don't like refreshType() method. Firstly, it not very usefull since it don't handle @start/@end/@step changes. Second, when type is changed then binding will be changed too, therefore you can adjust values on constructor. I'd suggest to move values adjusting into constructor and start/end/step properties.
Comment 33 aaronr 2006-09-28 15:33:17 PDT
(In reply to comment #32)
> (In reply to comment #31)
> 
> > I'm r-'ing for now to get it off of my queue.  If you decide to just add
> > comments and do the other work in later bugs, lemme know and I'll r+ it. 
> > Nothing wrong with the code while we are only supporting decimal types.
> > 
> 
> I really don't like refreshType() method. Firstly, it not very usefull since it
> don't handle @start/@end/@step changes. 


We can always handle start, end, step changes in the .cpp part of range.  Through XTF.  And force a refresh.

> Second, when type is changed then
> binding will be changed too, therefore you can adjust values on constructor.

Binding won't necessarily be changed if the base builtin type is the same or the CSS selector includes both types, will it?
Comment 34 Merle Sterling 2006-09-29 14:20:19 PDT
(In reply to comment #31)
> You really need to make the adjustRangeValues here in
> the base binding do nothing or have it return ERROR_NOT_IMPLEMENTED.  It
> shouldn't default to decimal values.  You really need to force the extending
> bindings to implement this function.  
There is only one binding (range.xml) for all of the numeric types we support that are implemented as a slider control. adustRangeValues parses everything using parseFloat and works for int, decimal, double, and float.


> >+            else {
> >+              // Both Start and End are specified, but the specified Step
> >+              // value may not be suitable for maintaining the maximum
> >+              // range of 10 steps.
> >+              if ((aEnd - aStart) / aStep > 10) {
> >+                // Honor the specified start and end values, but adjust
> >+                // the Step value.
> >+                this.step = Math.round((aEnd - aStart) / 10);
> >+              }
> The more I think about it, if @start and @end are specified, we should try to
> honor a user-specified value for @step unless it is some totally unreasonable
> request.  And I don't think that if @start and @end are 1000 apart that @step
> of 50 (meaning 20 'ticks' along the slider) would be unreasonable.  At least
> change the '10' to 100 or 1000.  And put a comment in here why we are capping
> the value.  It is really only because of slider performance.  If someone comes
> along and uses a spin button widget instead, what was once unreasonable might
> not be any longer.

We have discussed this issue ad nauseum and decided to limit the range to 10 steps.  If in the future we want to use a spinbutton or some other control for any or all of the numeric types then a range of 10 steps may not be suitable but for now it is a slider and very large sliders don't look or perform well.  As a compromise, I've increased the max to 10 steps.

In order to use a control other than a slider, we would have to implement separate bindings for each numeric type and then adjustRangeValues would have to be added to every one of those new bindings with possibly different values for the maximum number of steps. For now, its a slider for every numeric type and we can't keep going round and round on whether the max range should be 10, 20, or N steps.

Comment 35 Merle Sterling 2006-09-29 14:23:43 PDT
I am now also in agreement that refreshType is not necessary.  We need to call adjustRangeValues on every refresh in case any of the attrs change.

Comment 36 Merle Sterling 2006-09-29 14:25:28 PDT
Created attachment 240664 [details] [diff] [review]
patch

Increased the maximum number of steps from 10 to 20 and added additional comments as to why we need to limit the range in some cases.
Comment 37 alexander :surkov 2006-09-29 18:47:14 PDT
Merle, the one issue I can see is please call adjustRangeValues inside start/end/step properties too.
Comment 38 aaronr 2006-10-01 16:22:14 PDT
(In reply to comment #35)
> I am now also in agreement that refreshType is not necessary.  We need to call
> adjustRangeValues on every refresh in case any of the attrs change.
> 

Ummm, we don't call refresh when these attrs change so this won't happen until our refresh happens to get called.
Comment 39 aaronr 2006-10-01 16:25:13 PDT
(In reply to comment #38)
> (In reply to comment #35)
> > I am now also in agreement that refreshType is not necessary.  We need to call
> > adjustRangeValues on every refresh in case any of the attrs change.
> > 
> 
> Ummm, we don't call refresh when these attrs change so this won't happen until
> our refresh happens to get called.
> 

If we handle attr changes via refresh, then we should do the same with the properties, shouldn't we?  I don't mind if you do both the property handling and attr handling via a seperate bug.
Comment 40 aaronr 2006-10-01 16:55:50 PDT
(In reply to comment #34)
> (In reply to comment #31)
> > You really need to make the adjustRangeValues here in
> > the base binding do nothing or have it return ERROR_NOT_IMPLEMENTED.  It
> > shouldn't default to decimal values.  You really need to force the extending
> > bindings to implement this function.  
> There is only one binding (range.xml) for all of the numeric types we support
> that are implemented as a slider control. adustRangeValues parses everything
> using parseFloat and works for int, decimal, double, and float.
> 
> 

What about xformswidget-range (range-xhtml.xml)?

My point is that you put this numeric-only functionality in range-base which should be inherited by all ranges, not just the numeric ones.  If you have a stub adjustRangeValues in range.xml and then have the different range bindings extend from it, then you can have an adjustRangeValues for xsd:duration, xsd:dateTime, etc.  All of those will also have to provide default values if the user didn't specify a begin, end, etc, right?  Why not have them override this functionality by extending range-base?  Otherwise we'll need something equivalent to a range-base for every datatype that a range can bind to, right?

I don't mind if you leave this functionality here for now so that we can get this bug into 0.7.
Comment 41 aaronr 2006-10-01 16:56:41 PDT
Comment on attachment 240664 [details] [diff] [review]
patch

r=me
Comment 42 alexander :surkov 2006-10-01 22:31:45 PDT
(In reply to comment #39)

> If we handle attr changes via refresh, then we should do the same with the
> properties, shouldn't we?  I don't mind if you do both the property handling
> and attr handling via a seperate bug.
> 

I belive properties handling should go here.



> My point is that you put this numeric-only functionality in range-base which
> should be inherited by all ranges, not just the numeric ones.  If you have a
> stub adjustRangeValues in range.xml and then have the different range bindings
> extend from it, then you can have an adjustRangeValues for xsd:duration,
> xsd:dateTime, etc.  All of those will also have to provide default values if
> the user didn't specify a begin, end, etc, right?  Why not have them override
> this functionality by extending range-base?  Otherwise we'll need something
> equivalent to a range-base for every datatype that a range can bind to, right?
> 
> I don't mind if you leave this functionality here for now so that we can get
> this bug into 0.7.
> 

You're idea is totally right but problem is we don't have proper base widget for ranges now (since xformswidget-range-base is used for numeric types). So, Im guessing we can leave this til bug 316353. Though if Merle likes to add new base binding for all ranges then I'm fine, in any way I guess Merle will it do (here or in bug 316353 ;)).
Comment 43 alexander :surkov 2006-10-01 22:45:02 PDT
I can see one more issue. Should we handle restriction of bound type in values adjusting? For example:
1) if range is bound to xsd:integer and if I specify float 'start' then other values will be float too.
2) if I bound to xsd:positiveInteger and if I specify 'start' = -100 then values will be adjusted in wrong way.
Comment 44 aaronr 2006-10-02 10:02:06 PDT
(In reply to comment #43)
> I can see one more issue. Should we handle restriction of bound type in values
> adjusting? For example:
> 1) if range is bound to xsd:integer and if I specify float 'start' then other
> values will be float too.
> 2) if I bound to xsd:positiveInteger and if I specify 'start' = -100 then
> values will be adjusted in wrong way.
> 

The user is allowed to pick invalid values using the other controls (select[1], datepicker, etc.), so we should allow the same with range.  In the end, the form author needs to know what the heck they are doing.
Comment 45 Merle Sterling 2006-10-02 12:42:10 PDT
(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #35)
> > > I am now also in agreement that refreshType is not necessary.  We need to call adjustRangeValues on every refresh in case any of the attrs change.
> > > 
> > 
> > Ummm, we don't call refresh when these attrs change so this won't happen until our refresh happens to get called.
> > 
> If we handle attr changes via refresh, then we should do the same with the
> properties, shouldn't we?  I don't mind if you do both the property handling
> and attr handling via a seperate bug.

Yes and as you said, we can do that in the .cpp part of range on another bug.  Let's concentrate on this patch actually fixing the problem for which the bug was opened.
Comment 46 Merle Sterling 2006-10-02 12:50:43 PDT
(In reply to comment #42)
> > My point is that you put this numeric-only functionality in range-base which should be inherited by all ranges, not just the numeric ones.  If you have a stub adjustRangeValues in range.xml and then have the different range bindings extend from it, then you can have an adjustRangeValues for xsd:duration, xsd:dateTime, etc.  All of those will also have to provide default values if the user didn't specify a begin, end, etc, right?  Why not have them override this functionality by extending range-base?  Otherwise we'll need something equivalent to a range-base for every datatype that a range can bind to, right?
> You're idea is totally right but problem is we don't have proper base widget
> for ranges now (since xformswidget-range-base is used for numeric types). So,
> Im guessing we can leave this til bug 316353. Though if Merle likes to add new
> base binding for all ranges then I'm fine, in any way I guess Merle will it do> (here or in bug 316353 ;)).

I see your point but there really isn't any common functionality between the 10 new types so we can't have one single base class from which all of the other bindings inherit and have that base implement adustRangeValues because it will be different for each of the new bindings anyway.  We already inherit from the base in range.xml and that does give us the things we do want to inherit - namely handling of the start/step/end attributes.

I will move adjustRangeValues into range-xhtml because it does only apply to numeric types. Just as for the numeric types, the spec says nothing about what to do with invalid or omitted attributes for these new types.  IF we decide to implement some default value setting as we do for the numeric types we can do it after discussion of what is appropriate, but it is not applicable to this particular bug.
Comment 47 aaronr 2006-10-02 13:07:50 PDT
like you said, we can make improvements as we go forward.  But just so that we are on the same page going forward, I believe that we need to make these changes eventually so we need to keep our eyes on the target:

1) xformswidget-range-base does have things that apply to all ranges.
methods - focus, getCurrentValue, updateInstanceData
properties - accessibilityType, start, end, step, incremental, accessors

2) we need to use the same name for the same work.  I don't know that we necessarily need a new interface to enforce it, but I think that every range binding will need to override - adjustRangeValues, refresh, all of the control interfaces

3) as part of #2, refresh should at some point move out of range.xml and into range-xhtml.xml

again, all of this can be done under a seperate bug.
Comment 48 Merle Sterling 2006-10-02 13:18:40 PDT
Agreed, but I am moving refresh and adjustRangeValues out of the base biding in range.xml and into range-xhtml right now. I think we can put this bug to rest and move on to further improvements with follow-on bugs.
Comment 49 Merle Sterling 2006-10-02 14:39:35 PDT
Created attachment 240981 [details] [diff] [review]
patch

Moving refresh and adjustRangeValues methods from range.xml to range-xhtml.
Comment 50 alexander :surkov 2006-10-02 22:05:19 PDT
Comment on attachment 240981 [details] [diff] [review]
patch

No, it's better to add new base binding. Refresh method should be defined there. Also this bidning should define pure virtual methods: adjustValues and isInRange that will be defined by base bindings for specific types. isInRange method will be used in refresh method. I guess this scenario should work.
Comment 51 alexander :surkov 2006-10-02 22:30:58 PDT
(In reply to comment #47)
> like you said, we can make improvements as we go forward.  But just so that we
> are on the same page going forward, I believe that we need to make these
> changes eventually so we need to keep our eyes on the target:
> 
> 1) xformswidget-range-base does have things that apply to all ranges.
> methods - focus, getCurrentValue, updateInstanceData
> properties - accessibilityType, start, end, step, incremental, accessors

accessibleType is now implemented only for slider representation, so it should be defined now inside numeric base bidning for range only.

> 2) we need to use the same name for the same work.  I don't know that we
> necessarily need a new interface to enforce it, but I think that every range
> binding will need to override - adjustRangeValues, refresh, all of the control
> interfaces

> 3) as part of #2, refresh should at some point move out of range.xml and into
> range-xhtml.xml
> 

I think it's better to don't override refresh method if it's possible. At least, other xforms bindings have refresh method inside base binding. It's better to add isInRange method like I proposed in previous comment to have refresh method inside base binding. So it will be like:

Range base binding:

      <method name="refresh">
        <body>
        <![CDATA[
          this.control.readonly = this.accessors.isReadonly();
-         var value = parseFloat(this.accessors.getValue());
-         if (!value)
-           return;
-         var inrange = this.start >= value && value <= this.end;
+         var value = this.accessors.getValue();
+         var inrange = this.isInRange(value);
          this.accessors.setInRange(inrange);
          this.control.set(this.start, this.end, this.step, value);
        ]]>
        </body>
     </method>

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

Numeric range base binding:

+    <method name="isInRange">
+      <parameter name="aValue"/>
+      <body>
+        var value = parseFloat(aValue);
+        if (isNaN(value))
+          return;
+        return this.start >= value && value <= this.end;
+      </body>
+    </method>
Comment 52 Merle Sterling 2006-10-05 09:36:55 PDT
Created attachment 241339 [details] [diff] [review]
patch

Ok, Alex...created a new range-numeric-base that overrides the refresh, isInRange, and adjustRangeValues methods from range-base.
Comment 53 alexander :surkov 2006-10-05 10:11:11 PDT
(In reply to comment #52)
> Created an attachment (id=241339) [edit]
> patch
> 
> Ok, Alex...created a new range-numeric-base that overrides the refresh,
> isInRange, and adjustRangeValues methods from range-base.
> 

Thank you :). I have the one main thing only is I guess you should implement refresh() in 'xformswidget-range-base'. Also, please add comment about 'set' method (since it is used in refresh method) where getControlElement() is described (I forgot to do it myself previously, sorry). And please add comment what isInRange() do.

Also, I'm not sure isInRange is good name since range is xforms element and range is state of xforms element. If you have the name better then I'd like to use it.
Comment 54 alexander :surkov 2006-10-05 10:30:37 PDT
I think it's better to don't copy comment what getControlElement() does from range-base to numberic-range-base. Probably is it better to say getControlElement() returns the same object like range-base makes excepting the one thing: methods works with numeric types. Something like this.
Comment 55 alexander :surkov 2006-10-05 10:32:28 PDT
nit:
instead
if () {
}
else {
}
you should write
if () {
} else {
}
Comment 56 Merle Sterling 2006-10-05 11:21:41 PDT
Created attachment 241348 [details] [diff] [review]
patch
Comment 57 Merle Sterling 2006-10-05 11:25:00 PDT
(In reply to comment #53)
> Thank you :). I have the one main thing only is I guess you should implement
> refresh() in 'xformswidget-range-base'. Also, please add comment about 'set'
> method (since it is used in refresh method) where getControlElement() is
> described (I forgot to do it myself previously, sorry). 
Ok, moved refresh back to range-base.  This really isn't going to matter because the new types in bug 316353 will all have to override refresh and duplicate some of that code because you can't override refresh, do something, and then call the base class method like you would in native code.

> Also, I'm not sure isInRange is good name since range is xforms element and
> range is state of xforms element. If you have the name better then I'd like to use it.
I'm fine with the name.  The native call is SetInRange, so isInRange seems logical to me.


Comment 58 alexander :surkov 2006-10-05 11:36:30 PDT
Comment on attachment 241348 [details] [diff] [review]
patch

Looks ok. Just you should move accessibleType property into numeric base binding since currenly accessibility supports slider-presentation ranges only.
Comment 59 alexander :surkov 2006-10-05 11:38:20 PDT
I can see one more issue. If start is greater than end then error is not fired. Should we file new bug for this instead to fix it here?
Comment 60 alexander :surkov 2006-10-05 11:39:52 PDT
(In reply to comment #58)
> (From update of attachment 241348 [details] [diff] [review] [edit])
> Looks ok. Just you should move accessibleType property into numeric base
> binding since currenly accessibility supports slider-presentation ranges only.
> 

Though never mind, just leave this for 316353.
Comment 61 Merle Sterling 2006-10-05 12:13:46 PDT
(In reply to comment #59)
> I can see one more issue. If start is greater than end then error is not fired.
> Should we file new bug for this instead to fix it here?

I can go ahead and write the error to the console if we agree that it is an error.  The slider will still show although it won't work properly unless we also change the step value to be negative so we step backwards.
Comment 62 Merle Sterling 2006-10-05 13:00:27 PDT
Created attachment 241355 [details] [diff] [review]
patch

Adding error for start > end; moved the accessibleType property to the numeric base binding.
Comment 63 aaronr 2006-10-05 13:32:19 PDT
Comment on attachment 241355 [details] [diff] [review]
patch


>Index: resources/content/range.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/range.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 range.xml
>--- resources/content/range.xml	25 Sep 2006 09:59:13 -0000	1.7
>+++ resources/content/range.xml	5 Oct 2006 19:59:08 -0000
>@@ -36,54 +36,76 @@
>    - the provisions above, a recipient may use your version of this file under
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
> <bindings xmlns="http://www.mozilla.org/xbl"
>           xmlns:html="http://www.w3.org/1999/xhtml">
> 
>-  <!-- RANGE: <NUMBER>
>+  <!-- RANGE: BASE
>     This binding is base for xforms range controls. It assumes successors
>     bindings implement getElementControl() method that returns the object:
>     {
>-      get/set value(); // get/set "number" value
>+      get/set value(); // get/set value
>       set readonly(); // makes range disabled
>-      get/set start(); // get/set @start attribute, type is "number"
>-      get/set end(); // get/set @end attribute, type is "number"
>-      get/set step(); // get/set @step attribute, type is "number"
>+      get/set start(); // get/set @start attribute
>+      get/set end(); // get/set @end attribute
>+      get/set step(); // get/set @step attribute
>       focus() // set the focus
>+      set(); // set start, end, step, value
>     }
>   -->
>   <binding id="xformswidget-range-base"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> 
>-    <implementation implements="nsIAccessibleProvider">
>-      <property name="accessibleType" readonly="true">
>-        <getter>
>-          return Components.interfaces.nsIAccessibleProvider.XFormsRange;
>-        </getter>
>-      </property>
>-
>+    <implementation>
>       <method name="refresh">
>         <body>
>         <![CDATA[
>-          this.control.readonly = this.accessors.isReadonly();
>-
>-          var value = parseFloat(this.accessors.getValue());
>-          if (!value)
>+          // The start/end/step attrs are all optional and if any are omitted
>+          // we need to adjust their values.
>+          this.adjustRangeValues(this.start, this.end, this.step);
>+
>+          // Start > End is an error.
>+          if (this.start > this.end) {
>+            this.delegate.reportError("rangeBeginEndError");
>             return;
>+          }
> 
>-          var inrange = this.start >= value && value <= this.end;
>+          this.control.readonly = this.accessors.isReadonly();
>+
>+          var value = this.accessors.getValue();
>+          var inrange = this.isInRange(value);
>           this.accessors.setInRange(inrange);
>+
>           this.control.set(this.start, this.end, this.step, value);
>         ]]>
>         </body>
>       </method>
> 
>+      <!-- Determine whether or not the current value is between the start
>+           and end values for the range so we can dispatch xforms-in-range
>+           or xforms-out-of-range events.
>+      -->
>+      <method name="isInRange">
>+        <body>
>+          return true;
>+        </body>
>+      </method>
>+
>+       <!-- The start/end/step attrs are all optional and if any are omitted
>+            we need to adjust their values.
>+       -->
>+      <method name="adjustRangeValues">

nit: You need to comment somewhere (probably at the top where we mention getElementControl) that the inheriting bindings for the different datatypes will need to override isInRange and adjustRangeValues.  It is pretty apparent that these are stubs, but we might as well spell it out so that we don't forget if in the future we have to support more/different datatypes.


>@@ -136,9 +158,119 @@
> 
>           if (!aIncremental || this.incremental)
>             this.accessors.setValue(this.control.value);
>         </body>
>       </method>
>     </implementation>
>   </binding>
> 
>+  <!-- RANGE: <NUMBER>
>+    This binding is base for xforms range controls with numeric types.
>+    Successor bindings should implement a getElementControl() method
>+    that returns numeric types.
>+  -->
>+  <binding id="xformswidget-range-numeric-base"
>+           extends="chrome://xforms/content/range.xml#xformswidget-range-base">
>+
>+    <implementation implements="nsIAccessibleProvider">
>+      <property name="accessibleType" readonly="true">
>+        <getter>
>+          return Components.interfaces.nsIAccessibleProvider.XFormsRange;
>+        </getter>
>+      </property>

nit: I'd put a comment here that any range base type that uses a slider widget as the element control should return Components.interfaces.nsIAccessibleProvider.XFormsRange as its accessibleType.

>+
>+      <method name="isInRange">
>+        <parameter name="aValue"/>
>+        <body>
>+        <![CDATA[
>+          var value = parseFloat(aValue);
>+          if (isNaN(value))
>+            return;
>+          return this.start >= value && value <= this.end;
>+        ]]>
>+        </body>
>+      </method>
>+

nit: looks like isInRange here won't return a boolean value if isNaN.  You should probably return false explicitly.

with those nits fixed, r=me
Comment 64 Merle Sterling 2006-10-05 13:46:37 PDT
Created attachment 241367 [details] [diff] [review]
final patch!

Added comments per Aaron.
Comment 65 aaronr 2006-10-05 16:30:11 PDT
checked into trunk for msterlin
Comment 66 aaronr 2006-10-17 14:44:18 PDT
checked into 1.8.0 branch on 2006/10/11
Comment 67 aaronr 2007-01-11 16:03:57 PST
checked into 1.8 branch on 2006/11/21

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