290255 - Slider Tag, <scale>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

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]

Attachment: xpfe and layout changes

Comment 2

[Neil Deakin]

Attachment: 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]

Attachment: Toolkit version

Maybe we should try the jar.mn relative path magic here?

Comment 4

[[e-mail]]

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]

Attachment: 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]

Attachment: xpfe and toolkit changes to add a <scale> tag

Comment 7

[Neil Deakin]

Attachment: accessibility changes

Comment 8

[Mike Connor [:mconnor]]

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]

 // 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

[[e-mail]]

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]

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]

Attachment: 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

[[e-mail]]

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]

Attachment: Add Mac native theme code, update windows code

Don't know who best reviews native theme changes.

Comment 15

[Aaron Leventhal]

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)]

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) (email my personal email if necessary)]

   // 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) (email my personal email if necessary)]

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]

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) (email my personal email if necessary)]

Comment on attachment 216137 [details] [diff] [review]
Add Mac native theme code, update windows code

minusing until questions are addressed

Comment 21

[Neil Deakin]

(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

[[e-mail]]

Attachment: 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]

Attachment: 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]

Comment on attachment 216014 [details] [diff] [review]
Address review comments

This has been reviewed already

Comment 25

[Neil Deakin]

<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]

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]

There isn't any rtl support for scale currently, but it wouldn't be too hard to add.

Comment 28

[Farzaneh Sarafraz]

Is anyone currently working on the RTL support? The wiki says there is the need.

Comment 29

[Steffen Wilberg]

dev-doc is at http://developer.mozilla.org/en/docs/XUL:scale.

Comment 30

[mcdavis941 (sporadically reading bugmail)]

Neil, would you consider something like this

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

for the benefit of custom themes?