Last Comment Bug 290255 - Slider Tag, <scale>
: Slider Tag, <scale>
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
http://developer.mozilla.org/en/docs/...
Depends on:
Blocks: 321311 336781
  Show dependency treegraph
 
Reported: 2005-04-13 18:23 PDT by Neil Deakin
Modified: 2008-07-31 03:17 PDT (History)
20 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xpfe and layout changes (29.44 KB, patch)
2005-04-13 18:26 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
GTK Theme changes (21.43 KB, patch)
2005-04-13 18:28 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Toolkit version (10.51 KB, patch)
2005-04-13 18:29 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
native theme and layout changes (61.80 KB, patch)
2005-09-30 19:01 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
xpfe and toolkit changes to add a <scale> tag (47.65 KB, patch)
2005-09-30 19:03 PDT, Neil Deakin
mconnor: review+
Details | Diff | Splinter Review
accessibility changes (11.85 KB, patch)
2005-09-30 19:04 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Address review comments (43.36 KB, patch)
2006-03-23 08:19 PST, Neil Deakin
neil: superreview+
Details | Diff | Splinter Review
Add Mac native theme code, update windows code (67.57 KB, patch)
2006-03-24 11:41 PST, Neil Deakin
roc: review-
Details | Diff | Splinter Review
Windows Classic sliders (2.38 KB, image/gif)
2006-05-02 05:40 PDT, neil@parkwaycc.co.uk
no flags Details
Address review comments (77.96 KB, patch)
2006-05-03 08:40 PDT, Neil Deakin
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Neil Deakin 2005-04-13 18:23:42 PDT
XUL should have a <slider> tag for selecting from a range. I implemented a
simple one a while ago, but never added it. There's still things that could be
added in the future, like a numeric label, but this is a good start.
Comment 1 Neil Deakin 2005-04-13 18:26:02 PDT
Created attachment 180660 [details] [diff] [review]
xpfe and layout changes
Comment 2 Neil Deakin 2005-04-13 18:28:27 PDT
Created attachment 180661 [details] [diff] [review]
GTK Theme changes

GTK-only theme changes for now. Don't have a Mac to build with and the Windows
theme API is non-cooperative.
Comment 3 Neil Deakin 2005-04-13 18:29:26 PDT
Created attachment 180662 [details] [diff] [review]
Toolkit version

Maybe we should try the jar.mn relative path magic here?
Comment 4 neil@parkwaycc.co.uk 2005-05-06 07:19:06 PDT
Comment on attachment 180660 [details] [diff] [review]
xpfe and layout changes

I can't really comment on the native theme stuff.

>+:not(scrollbar) > slider {
>+  -moz-binding: url(chrome://global/content/bindings/slider.xml#slider);
>+}
You don't actually have to call your new widget a slider to make it use
nsSliderFrame.cpp - the binding already extends xul:slider - this will save on
all the various :not(scrollbar)-type rules (which I must say I'm not keen on).

>+      <children/>
I don't see the point of this.

>+          var value = this.getAttribute(attr);
>+          if (value){
You didn't do an if (value) check in _setIntegerAttribute and I don't see why
you need it here either.

>+        event.preventBubble();
This is probably not very useful. What may be useful is event.preventDefault();
to stop the arrow keys scrolling a containing scrollbox.

>+      <handler event="keypress" keycode="VK_LEFT" phase="target">
At some point this will need to be fixed to work in RTL...

>+slider[orient="vertical"] sliderthumb {
You need to make sliderthumb inherit the orient too, so that you can write a
simpler CSS rule. (This one is particularly bad, because it checks the ancestor
hierarchy in case it finds a vertical slider).
Comment 5 Neil Deakin 2005-09-30 19:01:20 PDT
Created attachment 198095 [details] [diff] [review]
native theme and layout changes

- add support for scale in windows and gtk native theme code
- add minpos for slider so that scales can set a minimum value
Comment 6 Neil Deakin 2005-09-30 19:03:12 PDT
Created attachment 198098 [details] [diff] [review]
xpfe and toolkit changes to add a <scale> tag
Comment 7 Neil Deakin 2005-09-30 19:04:19 PDT
Created attachment 198099 [details] [diff] [review]
accessibility changes
Comment 8 Mike Connor [:mconnor] 2005-10-07 09:40:10 PDT
Comment on attachment 198098 [details] [diff] [review]
xpfe and toolkit changes to add a <scale> tag

>+          <![CDATA[
>+            var accService = Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);

please split this line so it doesn't overflow quite so much

>+      <method name="_getIntegerAttribute">
>+        <parameter name="attr"/>
>+        <parameter name="defaultvalue"/>

general: we try to follow the style guide so that args/param names are prefaced
with a/an (anAttr, aDefaultValue).  Yes, there's stuff that doesn't do this,
but new code should enforce this.

>+    <handlers>
>+      <handler event="DOMAttrModified">
>+        if (event.originalTarget != this._slider) return;

general: always put a return statement on a new line.

>+        var attrname = event.attrName;
>+        if (attrname == "curpos") {
>+          this.setAttribute("value", event.newValue);
>+
>+          var event = document.createEvent("Events");
>+          event.initEvent("change", false, true);
>+          this.dispatchEvent(event);
>+        }
>+        else if (attrname == "minpos") {
>+          this.setAttribute("min", event.newValue);
>+        }
>+        else if (attrname == "maxpos") {
>+          this.setAttribute("max", event.newValue);
>+        }

typically we'd use a switch statment once we get to three, and brackets around
single lines are bad too.

>+      <handler event="keypress" keycode="VK_UP">
>+        this.decrease();
>+        event.preventDefault();
>+      </handler>

general: instead of event.preventDefault() please use preventdefault="true" on
these handlers.  This ensures that it'll work in content when JS is disabled,
etc.

>Index: toolkit/themes/pinstripe/global/scale.css
>===================================================================
>RCS file: toolkit/themes/pinstripe/global/scale.css
>diff -N toolkit/themes/pinstripe/global/scale.css
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/themes/pinstripe/global/scale.css	18 Sep 2005 20:32:30 -0000
>@@ -0,0 +1,89 @@
>+/* ***** BEGIN LICENSE BLOCK *****
<snip>
>+ * ***** END LICENSE BLOCK ***** */

we're starting to preprocess CSS to cut down disk size even further.  Since
this is trunk, you can use the following style to cut out all of the text from
the jarred chrome.  Then add * to the jar.mn line to call the PP.

>+/* ***** BEGIN LICENSE BLOCK *****
%if 0
(license text)
%endif
>+ * ***** END LICENSE BLOCK ***** */

anyway, looks pretty good, with those bits addressed, r=me for the toolkit bits
(and I'd like to see the xpfe/toolkit impls stay in sync)
Comment 9 Erik Arvidsson 2005-10-09 22:50:31 PDT
 // If the platform supports it, the left/right chunks
 // of the slider thumb
-#define NS_THEME_SLIDER_THUMB_START                        113
-#define NS_THEME_SLIDER_THUMB_END                          114
+#define NS_THEME_SCALE_THUMB_START                        115
+#define NS_THEME_SCALE_THUMB_END                          116
 
 // The ticks for a slider.
-#define NS_THEME_SLIDER_TICK                               115
+#define NS_THEME_SCALE_TICK                               117

Doesn't these need to have both horizontal and vertical rendering?
Comment 10 neil@parkwaycc.co.uk 2005-11-02 15:31:48 PST
Comment on attachment 198098 [details] [diff] [review]
xpfe and toolkit changes to add a <scale> tag

Sorry for the delay; this is just a skim through, I'll try it out later.

>+thumb.scale-thumb {
I would have thought .scale-thumb should suffice here.

>+scale[disabled] .scale-slider {
>+  -moz-user-focus: ignore;
>+}
This doesn't work automatically? Perhaps you need to implement nsIDOMXULControlElement.

>+      <property name="pageIncrement" onget="return this._getIntegerAttribute('pagencrement', 10);"
Typo in attribute name.

>+        if (attrname == "curpos") {
>+          this.setAttribute("value", event.newValue);
Hmm... scrollbars automatically get their curpos synchronized to the slider. Maybe it would be easier if you could hook in to that mechanism. You'd also be able to use phase="target" on your DOMAttrChanged handler that fires the change event. You'd also be able to set your other attributes and let XBL copy them to the slider.

>+      <handler event="focus" phase="capturing">
>+        <![CDATA[
>+          if (!this.hasAttribute('focused'))
>+            this.setAttribute('focused','true');
>+        ]]>
>+      </handler>
>+      
>+      <handler event="blur" phase="capturing">
>+        <![CDATA[
>+          this.removeAttribute('focused');
>+        ]]>
>+      </handler>
I don't see any focus rules at all, either using the attribute or :focus.

>+  skin/classic/global/scale.css                                 (global/mac/scale.css)
>+  skin/classic/global/scale.css                               (global/win/scale.css)
The files appear to be identical?

>+  cursor: default;
What's this overriding?

>+  min-height: 18px;
>+  min-width: auto;
>+  min-height: auto;
Typo?
Comment 11 Gary van der Merwe 2006-01-24 06:07:23 PST
I get this warning when I move the slider.

Warning: variable event hides argument
Source file: chrome://global/content/bindings/scale.xml
Line: 103, Column: 14
Source code:
          var event = document.createEvent("Events"); 
Comment 12 Neil Deakin 2006-03-23 08:19:07 PST
Created attachment 216014 [details] [diff] [review]
Address review comments

> Hmm... scrollbars automatically get their curpos synchronized to the slider.
> Maybe it would be easier if you could hook in to that mechanism. You'd also be
> able to use phase="target" on your DOMAttrChanged handler that fires the change
> event. You'd also be able to set your other attributes and let XBL copy them to
> the slider.

I tried that method originally, but for some reason I didn't like it, a reason I don't remember now. I could revisit it if desired.
Comment 13 neil@parkwaycc.co.uk 2006-03-24 04:10:57 PST
Comment on attachment 216014 [details] [diff] [review]
Address review comments

>+            this.dispatchEvent(changeEvent);
>+          
>+            break;
>+          case "minpos":
Nit: The blank line is in the wrong place.

>+.scale-thumb {
>+  border: 3px solid;
>+  -moz-border-top-colors: #000000 #E4EBF2 #C3CAD2;
>+  -moz-border-right-colors: #000000 #8F9DAD #A4AFBB;
>+  -moz-border-bottom-colors: #000000 #8F9DAD #A4AFBB;
>+  -moz-border-left-colors: #000000 #E4EBF2 #C3CAD2;
>+  background-image: url("chrome://global/skin/scrollbar/thumb-vrt-grip.gif");
>+  min-width: 18px;
>+}
>+
>+.scale-thumb[orient="vertical"] {
>+  background-image: url("chrome://global/skin/scrollbar/thumb-hrz-grip.gif");
>+  min-height: 18px;
>+  min-width: auto;
>+}
It looks like you've got the two images swapped by mistake. Do you have the Classic images (or alternatively a Classic screenshot)?
Also, min-width: auto; is invalid CSS. IIRC sliders always layout their thumbs to fit their minor dimension so you probably don't need to override it here.
Comment 14 Neil Deakin 2006-03-24 11:41:35 PST
Created attachment 216137 [details] [diff] [review]
Add Mac native theme code, update windows code

Don't know who best reviews native theme changes.
Comment 15 Aaron Leventhal 2006-04-06 11:46:53 PDT
Comment on attachment 198099 [details] [diff] [review]
accessibility changes

Mark, if this looks reasonable on your first pass you can hit me up for 2nd review.
Comment 16 Mark Pilgrim (inactive) 2006-04-07 13:56:46 PDT
Comment on attachment 198099 [details] [diff] [review]
accessibility changes

Aaron, my understanding of the DHTML Accessibility code is that xhtml2:role="wairole:slider" is already being exposed properly, so this patch does not need to change any of the DHTML Accessibility code.

If that's correct, r=me.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-09 14:57:23 PDT
   // Draw focus rectangles for XP HTML checkboxes and radio buttons
   // XXX it'd be nice to draw these outside of the frame

Hmm, the latter is possible now (by overriding GetWidgetOverflow).
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-09 15:06:43 PDT
nsNativeThemeWin::DrawWidgetBackground
+      contentRect.left += adjustment - 1;

Why is this different from the horizontal case? At least explain why in a comment.

Why doesn't ClassicDrawWidget have code for drawing scales?

+  if ((pageIncrement + maxpospx - minpospx) != 0)

> 0

+       nsAutoString snap;
+       scrollbar->GetAttr(kNameSpaceID_None, nsXULAtoms::snap, snap);
+       if (snap.EqualsLiteral("true")) {

+  nsAutoString disabled;
+  mContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, disabled);
+  if (disabled.EqualsLiteral("true"))
+    return NS_OK;

AttrValueIs?

What happens if minpos > maxpos?
Comment 19 Aaron Leventhal 2006-04-10 07:16:06 PDT
Comment on attachment 198099 [details] [diff] [review]
accessibility changes

Yes but this isn't using the DHTML slider -- this implements accessible slider support via C++.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-01 17:21:55 PDT
Comment on attachment 216137 [details] [diff] [review]
Add Mac native theme code, update windows code

minusing until questions are addressed
Comment 21 Neil Deakin 2006-05-01 19:06:25 PDT
(In reply to comment #18)

> Why doesn't ClassicDrawWidget have code for drawing scales?
> 

I don't know what a classic scale looks like. But in this case, it should fall back to the styles defined in css, which draw a scale which looks somewhat like a scrollbar.

> What happens if minpos > maxpos?

I will fix it such that maxpos will be considered to have the value minpos in that case. It won't make the scale useful for anything but at least it will be intentionaly so.

Comment 22 neil@parkwaycc.co.uk 2006-05-02 05:40:10 PDT
Created attachment 220500 [details]
Windows Classic sliders

The top horizontal slider demonstrates optional pointing and end and page tick marks. The middle vertical slider only demonstrates end and page tick marks.
Comment 23 Neil Deakin 2006-05-03 08:40:48 PDT
Created attachment 220649 [details] [diff] [review]
Address review comments

- adds Windows Classic theme support
- makes nsSliderFrame handle the case when minpos > maxpos. I'll file a follow up bug to make the binding do some better bounds checking
Comment 24 Neil Deakin 2006-05-05 11:27:52 PDT
Comment on attachment 216014 [details] [diff] [review]
Address review comments

This has been reviewed already
Comment 25 Neil Deakin 2006-05-31 08:19:08 PDT
<scale> has been checked in. See http://wiki.mozilla.org/XUL:Slider_Tag for info about how it is used
Comment 26 Simon Montagu :smontagu 2006-07-09 00:15:50 PDT
What is the status of rtl support in <scale>? It looks like left and right arrow keys aren't bidi-aware. Is the visual representation reversed when chrome is right-to-left?
Comment 27 Neil Deakin 2006-07-09 07:52:34 PDT
There isn't any rtl support for scale currently, but it wouldn't be too hard to add.
Comment 28 Farzaneh Sarafraz 2007-06-20 06:11:21 PDT
Is anyone currently working on the RTL support? The wiki says there is the need.
Comment 29 Steffen Wilberg 2007-08-02 13:19:53 PDT
dev-doc is at http://developer.mozilla.org/en/docs/XUL:scale.
Comment 30 mcdavis941 (sporadically reading bugmail) 2007-08-12 02:58:57 PDT
Neil, would you consider something like this

https://bugzilla.mozilla.org/show_bug.cgi?id=391655

for the benefit of custom themes?

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