372047 - support for reversing the direction of the scale widget

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

The wiki page on the scale widget <http://wiki.mozilla.org/XUL:Slider_Tag> suggests that it should be possible to reverse the direction of the widget so that the maximum value is on the left or the top.

The page suggests that this should be done via chromedir="rtl", but I don't think this is an RTL issue, since real-life sliders often put their highest values at the top, as John P. Baker notes in a comment on a blog post of mine <http://www.melez.com/mykzilla/2007/02/content-preferences-take-2.html#c7676629009339952265>, and one might want to emulate that real-world convention even in an LTR locale.

In any case, whether via chromedir or some other mechanism, it would be useful to be able to specify the direction of the scale widget.

And it should be trivial to implement.  The scale widget just needs to compute its value as it currently does and then, if its direction is the opposite of the default, apply the following computation to reverse the value:

val = abs(val - max - min)

Comment 1

[Robert Sayre]

should be easy to test :)

Comment 2

[John P Baker]

As Myk said the mouse use is fairly trivial; My problem is with the Keyboard
assignments, currently they are

Home -> minimum, End -> maximum (OK for both orientations and also in RTL)

PgUp -> decrement by page value, PgDn -> increment by page value (OK for vertical)

Up -> decrement, Dn -> increment (OK for vertical)

Left -> decrement, Right -> increment (OK for horizontal)

which means that quite a few horizontal ones seem counter-intuitive now and adding RTL will exacerbate the situation. The only combinations that I can see working are (vertical and LTR horizontal) using the keys above and, the one that seems most intuitive to me, (horizontal and LTR vertical) where Up/PgUp increment and Dn/PgDn decrement; At the moment we have one from each combination.

Some of this can be resolved by making Up, Dn, Left and Right only active in their appropriate orientations which then just leaves the problem of simulating PgRight/PgLeft actions with the PgUp/PgDn keys. 

Comment 3

[John P Baker]

Bother! That should have been |(vertical and RTL horizontal)| and |(horizontal and RTL vertical)|.

Rereading Myk's comment #0, I am not sure we read the paragraph

 Does there need to be support for dir='rtl' where the maximum value is on the left or top?
 Answer: yes, except through chromedir='rtl'

in the same way, but I am talking about |dir='rtl'|.

Comment 4

[Neil Deakin]

Attachment: support scales in reverse

This patch uses dir="reverse" to have the values of the scale go from right to left or bottom to top, as well as some minor cleanup.
 - the changes in nsSliderFrame::CurrentPositionChanged are just cleanup to make it match the code in DoLayout (maybe this can be shared somehow)
 - the xbl constuctor ensures that the value is initialized properly, which also fixes a bug on Mac where <scale min="20"/> is used
 - change events should bubble
 - just made the xpfe version point to the toolkit one

Neil, feel free to review the parts you can.

Comment 5

[[e-mail]]

Comment on attachment 257076 [details] [diff] [review]
support scales in reverse

>+         pospx = GetMaxPosition(scrollbar) - GetMinPosition(scrollbar) - pospx;
This looks ugly but I suppose moving the reversal code into SetCurrentPosition would make the snap multiplier code uglier instead?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: testcase that demonstrates "min" attribute triggering "change" event

One consequence of the patch in attachment 257076 [details] [diff] [review] is that the widget fires a "change" event upon instantiation (i.e. when the parser parses the tag, before the XUL window/page has finished loading) if the tag has a "min" attribute set to a positive value, presumably because of the change bubbling fix or the value initialization fix (or both).

Here's a simple testcase that demonstrates the problem.

Comment 7

[Neil Deakin]

(In reply to comment #5)

> This looks ugly but I suppose moving the reversal code into SetCurrentPosition
> would make the snap multiplier code uglier instead?
> 

Likely, as well as the code in PageUpDown.

> One consequence of the patch in attachment 257076 [details] [diff] [review] is that the
> widget fires a "change" event upon instantiation

Yes, although I can add a _suppressChangeEvents flag for this if desired. A number of other widgets fire select events on initialization as well.
 

Comment 8

[Myk Melez [:myk] [@mykmelez]]

> > One consequence of the patch in attachment 257076 [details] [diff] [review] [details] is that
> > the widget fires a "change" event upon instantiation
> 
> Yes, although I can add a _suppressChangeEvents flag for this if desired. A
> number of other widgets fire select events on initialization as well.

That sounds like a bug to me.  I think a better approach would be to suppress the initial change/select event for widgets that trigger such events when initializing themselves to the value specified by their "value" attribute (or the like).

Vlad mentioned to me, however, that there may be an IE-parity reason for firing those events.

Comment 9

[Neil Deakin]

> Vlad mentioned to me, however, that there may be an IE-parity reason for firing
> those events.

This is a XUL issue so I'm not sure what that means.

Comment 10

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 257076 [details] [diff] [review])
>>+         pospx = GetMaxPosition(scrollbar) - GetMinPosition(scrollbar) - pospx;
>This looks ugly but I suppose moving the reversal code into SetCurrentPosition
>would make the snap multiplier code uglier instead?
It turns out that the mouse outside thumb code doesn't work, and needs to be reversed too. In fact, only PageUpDown doesn't need to be reversed.

So, to summarise:
HandleEvent - isDraggingThumb - isMouseOutsideThumb
 - converts from twips to scrollbar units, but needs reversing
HandleEvent - isDraggingThumb - snap="true"
 - converts from twips to scrollbar units to force snapping, reverses
HandleEvent - middle mouse click
 - converts from twips to scrollbar units and reverses
PageUpDown
 - already has scrollbar units
MouseDown
 - converts from twips to scrollbar units and reverses

So, were it not for snap="true" I'd want something along the following lines:
SetCurrentPositionInternal takes bounds-checked scrollbar units
PageUpDown bounds-checks its position and calls SCPI
SetCurrentPosition takes twips; converts, reverses and bound-checks for SCPI.

Remind me, why do scale widgets need snap?

Comment 11

[[e-mail]]

Comment on attachment 257076 [details] [diff] [review]
support scales in reverse

r- because of the mouse outside thumb bug.

Sorry for the delay in reviewing this.

Comment 12

[Neil Deakin]

(In reply to comment #10)
> It turns out that the mouse outside thumb code doesn't work, and needs to be
> reversed too. In fact, only PageUpDown doesn't need to be reversed.

What doesn't work about it?

> Remind me, why do scale widgets need snap?

So that the scale can only be set to multiples of the increment. 

Comment 13

[Neil Deakin]

Attachment: rework SetCurrentPosition

This fixed up the SetCurrentPosition function as described above.

I don't have a problem with the snapping back if the mouse is moved away from the slider/scale. What is slider.snapMultiplier set to?

Comment 14

[[e-mail]]

(In reply to comment #13)
>I don't have a problem with the snapping back if the mouse is moved away from
>the slider/scale. What is slider.snapMultiplier set to?
It defaults to 6 here.

Comment 15

[[e-mail]]

Comment on attachment 260813 [details] [diff] [review]
rework SetCurrentPosition

OK, this fixes the thumb snap-back issue.

One thing troubles me though: should the snap multiplier affect middle or shift+clicks? It doesn't look as if it does. Maybe the pixel to scrollbar unit conversion and snap code should be moved into a new method?

Comment 16

[Neil Deakin]

Comment on attachment 260813 [details] [diff] [review]
rework SetCurrentPosition

Some Cocoa changes to support reverse direction scales

Comment 17

[John P Baker]

Can a better writer than me - ie almost anybody - add this to the document at http://developer.mozilla.org/en/docs/XUL:scale (referenced from http://developer.mozilla.org/en/docs/Firefox_3_for_developers#New_XUL_Elements ); Or, at least, add a dev-doc-needed keyword to this bug. Thanx.

Comment 18

[Neil Deakin]

Tests is bug 390610

Comment 19

[Eric Shepherd [:sheppy]]

documented here:

http://developer.mozilla.org/en/docs/XUL:scale#Attributes