271044 - Implement xforms:range element

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Robert Marcano]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041111 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041111 Firefox/1.0

Implement the xforms range element

Reproducible: Always
Steps to Reproduce:

Comment 1

[Robert Marcano]

Attachment: Initial implementation of the element

This implementation uses an XUL scrollbar element. currently it only supports
positive integer values from the model. 

This ui element needs to be tigthly integrated with the schema of the value
being edited, in order to get the min, max and step value. For non positive
integer values, the idea is to scale the values to generate the required min,
max and step size because it only accept integer values.

How can i get the schema type of a node, if it has range restrictions? Is that
part implemented?

Comment 2

[aaronr]

I don't think that there is any function written, yet, to easily determine the
type of a node.  But the information is there.  The MDG has that information,
and also I think that you can get the MIP's (model item properties, of which
"type" is one) off of the instance node that you bind to.  Looks like in
nsXFormsModelElement::ProcessBind it sets the MIP's as properties off of each
node of the nodeset that the bind applies to.  Also in ProcessBind, it sets the
MIP's in the MDG for each node.

For your case, I think that you should forget the MDG approach, because that
won't have "types" of instance nodes that aren't part of a binding (i.e.
instance nodes that have the "type" attribute right on them).  I think it would
be a better approcach for you to get the instance node that the range is
single-node bound to and get the type directly from it.

Comment 3

[Allan Beaufour]

The MDG does not care much about the type. IMHO, the schema support that Doron
is working on, is the place to get that information.

Comment 4

[Doron Rosenberg (IBM)]

I managed to get this to build on trunk.

Comment 5

[Allan Beaufour]

[I'm guessing that Robert is going nowhere with this, reassigning to bug owner.]

My thoughts are that it should be done in XBL, and we need to define an
interface exposed by the XForms engine, something with at least:
readonly attribute integer min
readonly attribute integer max
readonly attribute integer step
(though min and max can also be unbound also.)

Note that the spec. says:
"In the event of overlapping restrictions between the underlying datatype and
the start and end hints, the most restrictive range should be used."
(http://www.w3.org/TR/xforms/slice8.html#ui-range)

For step, there's no specification, but something like "max-min/20" and then 
restricted/converted to the type (ie. if days, step shouldn't be "1.3 day").

Comment 6

[Allan Beaufour]

Attachment: _rough_ XBL version of range

Here's a go at an XBL version of range. It's crappy crap, it just shows the
interface, etc.. Only shows the bound value, does not set it (events disappear
somewhere with an assertion about not being in a XUL document).

Comment 7

[Allan Beaufour]

Attachment: Patch using canvas

Works ok for me. Needs some more refining though, especially support for
in/out-of-range. There's a TODO in range.xml

Comment 8

[Allan Beaufour]

Status update: I have it supporting out of range values, and dispatches the
corresponding events, etc., so it actually passes two of the test suite tests now.

I still need to handle:
* handle @incremental="true"
* handle undefined step
* handle undefined begin / end

Undefined step is fairly easy I think. But how about undefined begin and/or end?
If both are undefined we should probably use a "knob control" instead, but what
about only one of them?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Are some files missing from the canvas-using patch?  I don't see any canvas usage..

Comment 10

[Allan Beaufour]

(In reply to comment #9)
> Are some files missing from the canvas-using patch?  I don't see any canvas
usage..

What the? It's a totally out-dated patch I've uploaded. Argh. I'll upload a
correct version asap.

Comment 11

[Allan Beaufour]

Attachment: Patch using canvas (the correct patch...)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

hmm, I wasn't CC'd.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Allan, what is still missing here?
The patch doesn't apply cleanly to trunk anymore.

Comment 14

[Allan Beaufour]

(In reply to comment #13)
> Allan, what is still missing here?

1. Acknowledgement on that we want to use canvas

2. Handle ranges without start or end. I would go for a followup bug, which
implements a "knob" control or something like that.

> The patch doesn't apply cleanly to trunk anymore.

Merde. I'll get to this, my request queue, etc. after tomorrow.

Comment 15

[Allan Beaufour]

Attachment: Updated patch

There are still todo's, which are listed in the header of
resources/content/range.xml.

Comment 16

[aaronr]

Comment on attachment 200057 [details] [diff] [review]
Updated patch


>--- xforms/nsXFormsRangeElement.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.range/nsXFormsRangeElement.cpp	2005-10-19 09:50:05.000000000 +0200

>+// nsXFormsRangeElement
>+nsresult
>+nsXFormsRangeElement::AttributeGetter(const nsAString &aAttr, nsAString &aVal)
>+{
>+  PRBool hasAttr;
>+  nsAutoString val;
>+  if (mElement
>+      && NS_SUCCEEDED(mElement->HasAttribute(aAttr, &hasAttr))
>+      && hasAttr) {
>+    mElement->GetAttribute(aAttr, val);
>+  }
>+  if (val.IsEmpty()) {
>+    SetDOMStringToNull(aVal);
>+  } else {
>+    aVal = val;
>+  }
>+
>+  return NS_OK;
>+}
>+
>+// nsIXFormsRangeElement
>+NS_IMETHODIMP
>+nsXFormsRangeElement::GetRangeStart(nsAString &aMin)
>+{
>+  return AttributeGetter(NS_LITERAL_STRING("start"), aMin);
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsRangeElement::GetRangeEnd(nsAString &aMax)
>+{
>+  return AttributeGetter(NS_LITERAL_STRING("end"), aMax);
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsRangeElement::GetRangeStep(nsAString &aStep)
>+{
>+  return AttributeGetter(NS_LITERAL_STRING("step"), aStep);
>+}

Can't you do this.getAttribute in JS for this?	Or put AttributeGetter
on delegate and do the others in JS?  Nothing range specific in here
and could be used by others.



>+      <method name="calcPos">
>+	<parameter name="val"/>
>+	<body>
>+	  var pos = val - this.rBegin;
>+	  if (this.rStep) {
>+	    pos = (pos / this.rStep) * this.stepsp;
>+	  } else {
>+	    pos = (pos / (this.rEnd - this.rBegin)) * this.barwidth;
>+	  }
>+	  return Math.round(pos) + this.margin;
>+	</body>
>+      </method>

comment, please.

>+
>+      <!-- sets the slider to a new value -->
>+      <method name="setSlider">
>+	<!-- The new value -->
>+	<parameter name="aVal"/>
>+	<!-- The mode: move, set -->
>+	<parameter name="aMode"/>

Comment, please, what it means to setSlider if aMode = "set" versus "move".  Or
if there is no aMode.

>+
>+	<body>
>+	  <![CDATA[
>+
>+	  aVal = parseFloat(aVal);
>+	  if (aMode != "set" && isNaN(aVal)) {
>+	    return alert("NaN passed to setSlider()!");
>+	  }
>+
>+          var outOfRange = false;

nit: alignment.  Some all through this file.

>+
>+          // clear old slider
>+	  this.ctx.clearRect(this.calcPos(this.rVal) - this.sliderwidth - 1, this.tickheight - 1,
>+	                     this.sliderwidth * 2 + 2, this.tickheight * 3 + 2);
>+
>+          // (re)draw horisontal bar
>+          this.ctx.lineWidth = 1;
>+          this.ctx.fillStyle = this.fillStyle;
>+          this.ctx.strokeStyle = this.strokeStyle;
>+          mid = Math.round(this.height / 2);
>+          // XXX only needs to be redrawn for old slider pos
>+          this.ctx.fillRect(this.margin, mid - 1, this.barwidth, 3);
>+          this.ctx.strokeRect(this.margin, mid - 1, this.barwidth, 3);
>+
>+          // check whether out-of-range state has changed, and dispatch event if it has
>+          if (outOfRange != this.outOfRange) {
>+            this.outOfRange = outOfRange;
>+            var event = outOfRange ? "xforms-out-of-range" : "xforms-in-range";
>+            this.delegate.dispatchXFormsNotificationEvent(event, this);
>+          }
>+
>+          // if out-of-range, we cannot represent the value
>+          if (outOfRange) {
>+	    this.rVal = null;
>+            return null;
>+          }

Need to style it as out-of-range too?


>+      <!-- create new range object -->
>+      <method name="createRange">
>+        <parameter name="aCanvas"/>
>+        <parameter name="aLabelBegin"/>
>+        <parameter name="aLabelEnd"/>
>+        <parameter name="aBegin"/>
>+        <parameter name="aEnd"/>
>+        <parameter name="aStep"/>
>+        <body>
>+          <![CDATA[
>+          if (!(aCanvas && aLabelBegin && aLabelEnd)) {
>+            alert("One or more of the passed objects were null");
>+            return false;
>+          }
>+
>+          this.rBegin = parseFloat(aBegin);
>+          this.rEnd = parseFloat(aEnd);
>+          this.rStep = parseFloat(aStep);
>+          this.rVal = this.rBegin;
>+	  this.isIncremental = this.getAttribute("incremental") == "true";
>+	  this.justMoved = false;
>+
>+          if (isNaN(this.rBegin) || isNaN(this.rEnd)) {
>+            alert("One or more init() parameters is NaN!");
>+            return false;
>+          }
>+
>+          // XXX should we handle this?
>+          if (this.rBegin >= this.rEnd) {
>+            alert("begin is higher than end?");
>+            return false;
>+          }

Ummm, alerts?  These are temporary, I assume, since start and end are 
strings, not numbers.

> 
>diff -X patch-excludes -uprN -U8 xforms/resources/content/xforms.xml xforms.range/resources/content/xforms.xml
>--- xforms/resources/content/xforms.xml	2005-10-19 09:49:14.000000000 +0200
>+++ xforms.range/resources/content/xforms.xml	2005-10-19 09:56:29.000000000 +0200
>@@ -99,24 +99,41 @@
>       </method>
> 
>       <method name="focus">
>         <body>
>           return false;
>         </body>
>       </method>
> 
>+      <!-- Dispatch DOMActivate to the control itself -->
>       <method name="dispatchDOMActivate">
>         <body>
>           var ev = document.createEvent("UIEvents");
>           ev.initUIEvent("DOMActivate", true, true, window, 1);
>           this.dispatchEvent(ev);
>           return true;
>         </body>
>       </method>
>+
>+      <!--
>+	  Dispatch an xforms notification event to a node.
>+
>+          See http://www.w3.org/TR/xforms/slice4.html#evt-notify
>+      -->
>+      <method name="dispatchXFormsNotificationEvent">
>+        <parameter name="aEventName"/>
>+        <parameter name="aTarget"/>
>+        <body>
>+          var ev = document.createEvent("Events");
>+          ev.initEvent(aEventName, true, false);
>+          aTarget.dispatchEvent(ev);
>+          return true;
>+        </body>
>+      </method>
>     </implementation>
>   </binding>

You might want to add comment here as to what messages this will work for and
which it won't.
Can't really predict who will use this for what, so might want to at least give
them
a heads up that bubbles="true" and cancelable="false" and these aren't
necessarily the
values for all XForms events.  Maybe what we should really have is a way to
call
nsXFormsUtils::DispatchEvent from JS so that the bubble and cancelable events
are
set up automatically.


This is a good stab at a beginning for range.  Good to get this patch in before
it grows much
more.  I didn't make any comments in here that I really need to follow up on, I
don't think.
so r=me with my comments addressed.

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> (From update of attachment 200057 [details] [diff] [review] [edit])
> >+NS_IMETHODIMP
> >+nsXFormsRangeElement::GetRangeStart(nsAString &aMin)
> >+{
> >+  return AttributeGetter(NS_LITERAL_STRING("start"), aMin);
> >+}
> >+
> >+NS_IMETHODIMP
> >+nsXFormsRangeElement::GetRangeEnd(nsAString &aMax)
> >+{
> >+  return AttributeGetter(NS_LITERAL_STRING("end"), aMax);
> >+}
> >+
> >+NS_IMETHODIMP
> >+nsXFormsRangeElement::GetRangeStep(nsAString &aStep)
> >+{
> >+  return AttributeGetter(NS_LITERAL_STRING("step"), aStep);
> >+}
> 
> Can't you do this.getAttribute in JS for this?	Or put AttributeGetter
> on delegate and do the others in JS?  Nothing range specific in here
> and could be used by others.

Yeah, they look quite stupid. I had a todo in the start of the file, I've moved
it down here to the functions. The XTF range needs to check the type of the
instance data node and then for GetRangeStart() it needs to return
"max(type.minimum, @start)", and vice versa for GetRangeEnd()

For the step, it needs to set the step to something "start" which also accounts
for the type, if @step is not set.
 

> nit: alignment.  Some all through this file.

> >+          // check whether out-of-range state has changed, and dispatch
event if it has
> >+          if (outOfRange != this.outOfRange) {
> >+            this.outOfRange = outOfRange;
> >+            var event = outOfRange ? "xforms-out-of-range" : "xforms-in-range";
> >+            this.delegate.dispatchXFormsNotificationEvent(event, this);
> >+          }
> >+
> >+          // if out-of-range, we cannot represent the value
> >+          if (outOfRange) {
> >+	    this.rVal = null;
> >+            return null;
> >+          }
> 
> Need to style it as out-of-range too?

Hmm, yes, good catch. This means more functionality on the XTF interface... hmm.
If it's ok, I'll follow up on that.
 
> 
> >+      <!-- create new range object -->
> >+      <method name="createRange">
> >+        <parameter name="aCanvas"/>
> >+        <parameter name="aLabelBegin"/>
> >+        <parameter name="aLabelEnd"/>
> >+        <parameter name="aBegin"/>
> >+        <parameter name="aEnd"/>
> >+        <parameter name="aStep"/>
> >+        <body>
> >+          <![CDATA[
> >+          if (!(aCanvas && aLabelBegin && aLabelEnd)) {
> >+            alert("One or more of the passed objects were null");
> >+            return false;
> >+          }
> >+
> >+          this.rBegin = parseFloat(aBegin);
> >+          this.rEnd = parseFloat(aEnd);
> >+          this.rStep = parseFloat(aStep);
> >+          this.rVal = this.rBegin;
> >+	  this.isIncremental = this.getAttribute("incremental") == "true";
> >+	  this.justMoved = false;
> >+
> >+          if (isNaN(this.rBegin) || isNaN(this.rEnd)) {
> >+            alert("One or more init() parameters is NaN!");
> >+            return false;
> >+          }
> >+
> >+          // XXX should we handle this?
> >+          if (this.rBegin >= this.rEnd) {
> >+            alert("begin is higher than end?");
> >+            return false;
> >+          }
> 
> Ummm, alerts?  These are temporary, I assume, since start and end are 
> strings, not numbers.

I only support floats for now, so they are numbers. An alert is wrong though.
I'll emit a scripterror instead.
 
> >+
> >+      <!--
> >+	  Dispatch an xforms notification event to a node.
> >+
> >+          See http://www.w3.org/TR/xforms/slice4.html#evt-notify
> >+      -->
> >+      <method name="dispatchXFormsNotificationEvent">
> >+        <parameter name="aEventName"/>
> >+        <parameter name="aTarget"/>
> >+        <body>
> >+          var ev = document.createEvent("Events");
> >+          ev.initEvent(aEventName, true, false);
> >+          aTarget.dispatchEvent(ev);
> >+          return true;
> >+        </body>
> >+      </method>
> >     </implementation>
> >   </binding>
> 
> You might want to add comment here as to what messages this will work for
> and which it won't. Can't really predict who will use this for what, so
> might want to at least give them a heads up that bubbles="true" and
> cancelable="false" and these aren't necessarily the values for all XForms
> events.  Maybe what we should really have is a way to call
> nsXFormsUtils::DispatchEvent from JS so that the bubble and cancelable
> events are set up automatically.

Ok, admittedly, I'm being a bit sneaky here, but I explicitly write "xforms
notification event", which all have cancelable="false" and bubbles="true". I've
underlined it, dunno if that helps.

Comment 18

[Allan Beaufour]

Attachment: Commens fixed and exposing ReportError()

I fixed your comments and exposed nsXFormsUtils::ReportError(), you better ok
the last one.

Comment 19

[Allan Beaufour]

Note to myself: the css rule should be more specific, it should be something like
range {
  -moz-binding: url('chrome://xforms/content/xforms.xml#xformswidget-input');
}

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

so the actual range control is only bound to to the datatypes that it support.
Instead of input we could have a "xformswidget-disabled-control". Dunno, how it
should look though.

Comment 20

[aaronr]

(In reply to comment #19)
> Note to myself: the css rule should be more specific, it should be something like
> range {
>   -moz-binding: url('chrome://xforms/content/xforms.xml#xformswidget-input');
> }
> 
> range[mozType|type="http://www.w3.org/2001/XMLSchema#decimal"] {
>   -moz-binding: url('chrome://xforms/content/xforms.xml#xformswidget-range');
> }
> 
> so the actual range control is only bound to to the datatypes that it support.
> Instead of input we could have a "xformswidget-disabled-control". Dunno, how it
> should look though.


I'd say if it isn't bound to a supported node that you output a warning to the
JS console and treat it like upload... gray out the range and also 'disable' it.

Comment 21

[aaronr]

Comment on attachment 200345 [details] [diff] [review]
Commens fixed and exposing ReportError()

Excellent!  I LOVE having ReportError exposed.	Will make debugging events
(esp. focus events) much easier from JS when you only care about the sequence
of things and don't want to dismiss a bunch of alerts.

Comment 22

[Allan Beaufour]

(In reply to comment #21)
> (From update of attachment 200345 [details] [diff] [review] [edit])
> Excellent!  I LOVE having ReportError exposed.	Will make debugging events
> (esp. focus events) much easier from JS when you only care about the sequence
> of things and don't want to dismiss a bunch of alerts.

Beware though that it only handles strings from bundles. We need to tweak
ReportError to allow for arbitrary strings, which would be very good in a Custom
Control world.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200345 [details] [diff] [review]
Commens fixed and exposing ReportError()

>+class nsXFormsRangeElement : public nsXFormsDelegateStub,
>+                             public nsIXFormsRangeElement
>+{
>+private:
>+  /// @note (XXX) use NS_FORWARD to make range inherit delegate
>+  /// 
>+  // NS_FORWARD_NSIDOMELEMENT(nsGenericHTMLElement::)

Could you explain this?



>+NS_IMETHODIMP
>+nsXFormsRangeElement::SetValue(const nsAString& aValue)
>+{
>+  if (!mBoundNode || !mModel)
>+    return NS_OK;
>+
>+  PRBool changed;
>+  nsresult rv = mModel->SetNodeValue(mBoundNode, aValue, &changed);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (changed) {
>+    nsCOMPtr<nsIDOMNode> model = do_QueryInterface(mModel);
>+ 
>+    if (model) {
>+      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Recalculate);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Revalidate);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Refresh);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+  }
>+
>+  return NS_OK;
>+}

This method is copy-pasted from nsXFormsDelegateStub. Why?
Please remove, if it is not needed.




>+
>+// nsXFormsRangeElement
>+nsresult
>+nsXFormsRangeElement::AttributeGetter(const nsAString &aAttr, nsAString &aVal)
>+{
>+  PRBool hasAttr;
>+  nsAutoString val;
>+  if (mElement
>+      && NS_SUCCEEDED(mElement->HasAttribute(aAttr, &hasAttr))
>+      && hasAttr) {
>+    mElement->GetAttribute(aAttr, val);
>+  }
>+  if (val.IsEmpty()) {
>+    SetDOMStringToNull(aVal);
>+  } else {
>+    aVal = val;
>+  }
>+
>+  return NS_OK;
>+}

No need to use HasAttribute. GetAttribute(aVal) and if (aVal.IsEmpty()) {...}
is enough.



>diff -X patch-excludes -uprN -U8 xforms/resources/content/range.xml xforms.range/resources/content/range.xml
>--- xforms/resources/content/range.xml	1970-01-01 01:00:00.000000000 +0100
>+++ xforms.range/resources/content/range.xml	2005-10-21 17:02:15.000000000 +0200
>@@ -0,0 +1,480 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<!--
>+   ASSUMPTIONS:
>+   *> @begin is valid, @end and @init value might not be
>+      this means that steps and ticks are calculated with begin as starting point
>+   *> Takes integers and floats
>+
>+   TODO: XXX
>+   *> limit amount of ticks
>+   *> handle undefined begin / end
>+   *> handle end < begin (including negative steps)
>+   *> @incremental should round if it is bound to integer
>+
>+   BUGS: XXX
>+   *> leaves a trace behind, hor.bar gets darker, etc... fix transparency
>+-->

I think this comment should be after the Licence.


With those r=me

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #23)
> 
> No need to use HasAttribute. GetAttribute(aVal) and if (aVal.IsEmpty()) {...}
> is enough.
> 

Should be GetAttribute(aAttr, aVal), I guess ;)

Comment 25

[Allan Beaufour]

(In reply to comment #23)
> (From update of attachment 200345 [details] [diff] [review] [edit])
> >+class nsXFormsRangeElement : public nsXFormsDelegateStub,
> >+                             public nsIXFormsRangeElement
> >+{
> >+private:
> >+  /// @note (XXX) use NS_FORWARD to make range inherit delegate
> >+  /// 
> >+  // NS_FORWARD_NSIDOMELEMENT(nsGenericHTMLElement::)
> 
> Could you explain this?

Yes, and I should have fixed it too, but aparently forgot. I wanted to make nsIXFormsRangeElement inherit from nsIXFormsDelegate, ran in to inheritance problems so I skipped that. But then I stumbled upon NS_FORWARD... noticed it, but didn't get further. I'll fix that now.
(it should be NS_FORWARD_NSIXFORMSDELEGATE(nsXFormsDelegateStub::))

Comment 26

[Allan Beaufour]

Attachment: Patch with smaug's comments

Ok, this one fixes your comments, but I think you should look at it once more, as I know let nsIXFormsRangeElement inherit from nsIXFormsDelegate.

I've also fixed a problem in range.xml in setSlider(): If the bound node value was set out of range, it adjusted the value instead of just being outofrange.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200769 [details] [diff] [review]
Patch with smaug's comments

>+
>+<bindings xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml">
>+      
>+  <binding id="xformswidget-range"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <children includes="label"/>
>+      <html:span anonid="labelBegin" style="margin-right: 3px;"></html:span>
>+      <!-- width and height set by CSS? -->
>+      <html:canvas tabindex="0" anonid="canvas" width="260" height="40"
>+                   onkeydown="this.parentNode.handleKey(event)"
>+                   onmousedown="this.parentNode.handleMouseDown(event)"
>+                   onmouseup="this.parentNode.handleMouseUp(event)"
>+                   onmouseout="this.parentNode.handleMouseOut(event)"
>+                   onmousemove="this.parentNode.handleMouseMove(event)">
>+      </html:canvas>
>+      <html:span anonid="labelEnd" style="margin-left: 3px;"> </html:span>
>+      <children/>
>+    </content>
>+

>+      <!-- handle mouse down -->
>+      <method name="handleMouseDown">
>+        <parameter name="event"/>
>+        <body>
>+          <![CDATA[
>+          if (event.button == 0) {
>+            this.currentOffset = this.getOffset(event);
>+            this.originalVal = this.rVal;
>+            // we've got to save the range somewhere global so the mousemove event can access it
>+            document.currentrange = this;

Sorry, I didn't notice this earlier, but document.currentrange looks strange.
I don't quite understand why it is needed. Are you sure 'this' is not the 
object you need in handleMouseMove. Or if there is something wrong with that, perhaps
you could add second parameter and use the right object in canvas' onmousemove.

Comment 28

[Allan Beaufour]

(In reply to comment #27)
> Sorry, I didn't notice this earlier, but document.currentrange looks strange.
> I don't quite understand why it is needed. Are you sure 'this' is not the 
> object you need in handleMouseMove. Or if there is something wrong with that,
> perhaps
> you could add second parameter and use the right object in canvas' onmousemove.

A very valid comment, I've never liked it myself. I'll investigate it.

Comment 29

[Allan Beaufour]

Attachment: Patch without document.currentRange

(In reply to comment #27)
> Sorry, I didn't notice this earlier, but document.currentrange looks strange.
> I don't quite understand why it is needed. Are you sure 'this' is not the 
> object you need in handleMouseMove. Or if there is something wrong with that,
> perhaps you could add second parameter and use the right object in canvas'
> onmousemove.

It was leftovers from being a cross-browser implementation... I've removed it now.

Question is wheter reportError should live on the delegate or the accessor?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 202239 [details] [diff] [review]
Patch without document.currentRange


>+
>+  /**
>+   * Report an error
>+   *
>+   * @param errorMsg          The error message id
>+   *
>+   * @todo XXX this should be extended to allow for "raw strings", not
>+   * necessarily kept in bundles.
>+   */
>+  void reportError(in DOMString errorMsg);

I think this can be in nsIXFormsDelegate, at least for now.
So no need to change that.

>+      <method name="refresh">
>+        <body>
>+	  <![CDATA[
>+          if (!this.isInitialized) {
>+            if (!this.delegate) {
>+              return;
>+            }
>+            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());
>+          }
>+
>+          // XXX: does not clear range if bound node "disappears"
>+          if (this.isInitialized && this.accessors.hasBoundNode()) {
>+            this.setSlider(this.accessors.getValue(), "set");
>+          }
>+	  ]]>
>+        </body>
>+      </method>

Remove tab before <![CDATA[ and ]]>.
Also, could try to split overlong lines.
With those r=me

I think we can still improve this in some cases, but maybe in separate bugs.
(And there are already XXX for the issues.)

Comment 31

[Allan Beaufour]

Checked in to trunk

Comment 32

[Allan Beaufour]

Filed follow-up bugs: bug 316353, bug 316354, and bug 316355.

Comment 33

[Allan Beaufour]

Attachment: Checked in version

Comment 34

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 35

[aaronr]

verfied fixed on MOZILLA_1_8_BRANCH