GNOME Bugzilla – Bug 791802
Fix direction value moves on scroll/keypress over RTL/inverted range
Last modified: 2018-04-18 19:34:35 UTC
* Open widget-factory * Set RTL text direction * Scroll horizontally over one of the horizontal Scales * The slider/value moves in the opposite direction than you scrolled The problem is that the final delta is only negate if :inverted is TRUE, but we should also negate it for RTL, i.e. using the existing should_invert() helper.
Created attachment 365769 [details] [review] Range: Fix horizontal scrolling on horiz+RTL Range * Open widget-factory * Set RTL text direction * Scroll horizontally over one of the horizontal Scales * The slider/value moves in the opposite direction than you scrolled The problem is that the final delta is only negate if :inverted is TRUE, but we should also negate it for RTL, i.e. using the existing should_invert() helper.
Review of attachment 365769 [details] [review]: This fixes one problem, but there are still others, e.g. horizontal scrolling on !inverted, RTL vertical scrolling on inverted, !RTL and inverted, !RTL afaict, we need a complete patch that will basically ignore should_invert() if the scroll is along the orthogonal orientation to that of the range.
Ignore the above description of the problem cases; they're pretty nonsensical. I'll post a big truth table later once I've got a patch that I think improves it. For now, I'm proceeding on the basis that a 'good' result there would mean that: * A parallel scroll should move the slider in the direction that is scrolled. This is the crux of this bug and, I think, is obviously correct to be fixed. * A perpendicular scroll should ignore :invert and simply take Up/Down and End/Start always to mean increase/decrease respectively. It doesn't make much sense, nor use, that :invert would result in opposite behaviour here. * However, I think this should not just use should_invert(), in that it should honour RTL on vert range, horz scroll: i.e. in RTL, Left should mean ++value. NB: Fine scroll ignores horz scrolls on vert ranges, and it's all GTK+ 4 has.
for posterity and to show that I didn't just pull the latter out of nowhere... [dboles] garnacho: but then maybe we should also then stop caring about direction for left/right on vertical ranges, i.e. make them always dec/increase the value resp. [garnacho] dboles: hmm, that should maybe still follow rtl conventions though [dboles] garnacho: so the fine scroll branch simply ignores horz scrolls on vert ranges garnacho: (and that's all that's used in GTK+ 4) garnacho: which means I'm looking at making the other branch scroll the right way in RTL on vert ranges (i.e. "start" always means "decrease value") then started wondering whether there's even any point! but i guess there is [garnacho] dboles: I think there is :). would be nice to eventually have a "physical" flag, that can rely on https://lists.freedesktop.org/archives/wayland-devel/2017-August/034711.html for wayland, and is at least guaranteed to be right for non-natural scroll on other backends
(In reply to Daniel Boles from comment #3) > I'll post a big truth table later once I've got a patch that I think > improves it. > > For now, I'm proceeding on the basis that a 'good' result there would mean > that: > > * A parallel scroll should move the slider in the direction that is > scrolled. > This is the crux of this bug and, I think, is obviously correct to be > fixed. > > * A perpendicular scroll should ignore :invert and simply take Up/Down and > End/Start always to mean increase/decrease respectively. It doesn't make > much sense, nor use, that :invert would result in opposite behaviour > here. According to this definition of "ok", here is a table identifying which cases are OK and which ones are not, abbreviated to NG for Not Good: range orient invert RTL scroll orient OK w/ delta OK w/ scrolldir horz no no horz ok ok horz no no vert ok ok horz yes no horz ok ok horz yes no vert NG NG horz no yes horz NG NG horz no yes vert ok ok horz yes yes horz NG NG horz yes yes vert NG NG vert no no horz -- ok vert no no vert ok ok vert yes no horz -- NG vert yes no vert ok ok vert no yes horz -- NG vert no yes vert ok ok vert yes yes horz -- ok vert yes yes vert ok ok In particular: * Vertically scrolling over a horizontal range that's :inverted has the effect on the value inverted too, but this doesn't seem relevant/desirable to me * Horizontal scrolling over a horizontal+RTL range results in the button/value moving in the opposite direction than scrolled, which seems clearly wrong. * The same is true for vertical scrolling over a horizontal+RTL range. * Vertically scrolling over horizontal ranges with :inverted XOR RTL results in the value moving oppositely to the start/end directionality of the scroll. I think I've devised an elegant solution to this, and crucially, it'll also easily resolve Bug 407242 into the bargain.
Created attachment 366149 [details] [review] Range: Add should_invert_move() for scrolls & keys This will be used in subsequent commits to fix the sign by which the value is changed in response to directional scroll or keypress events. The idea is: you have a movement to make – in the form of a delta that follows widget directions, i.e. −1 means left or up, +1 means right or down – and you want to know whether that delta needs to be inverted in order to produce the intuitively expected directional change of :value. The existing should_invert() is not sufficient: it just determines whether to invert visually, but we need more nuance than that for input. To answer that – while not doubling up the work for scrolls and keys – I add should_invert_movement(), which considers more factors, as follows: • A parallel movement on priv->orientation should just use the existing should_invert(), which already worked OK for this case (not others). • Movements on the other orientation now depend on priv->orientation: ◦ For a horizontal Range, always invert, so up (i.e. −ve in terms of widget coords) always means increase value & vice-versa. This was done in get_wheel_delta(), but move it here for use with keys too. ◦ For a vertical Range, ignore :invert as it’s only relevant to the parallel orientation. But *do* account for text direction, so we can make movements toward start/end always dec/increase the value. This returns TRUE if the delta should be inverted before applying to the value, and we can now use this function in both scroll and key handlers. https://bugzilla.gnome.org/show_bug.cgi?id=407242
Created attachment 366150 [details] [review] Range: Fix/improve dir of value change from scroll chiefly: • Honour text direction for H Ranges. We were only checking :inverted, so H-scrolling an RTL Range made the value move the opposite way. By using should_invert_move(), that incorporates both properties for us. • On a V range, make H scrolling to start/end always -/+ the value. It has until now been ignoring the text dir and always using left/right (but V Ranges in GTK+ 4 ignore H scrolls, so only GTK+ 3 gets this). This is simple: just let the new should_invert_move() handle it all. The local special case for H range/V scroll is no longer needed and dropped.
Created attachment 366151 [details] [review] Range: Make down/up keys act like down/up scrolls Before now, down/up keys respectively would increase/decrease the value, which is unintuitive & worse, contradicts what we already do for scrolls Fix simply by moving to the new should_invert_move() as scrolls just did – which gets us the other benefits explained in the prior 2 commits too. https://bugzilla.gnome.org/show_bug.cgi?id=407242
CCing Carlos, as we were discussing this on IRC. This seemed to go too well, so please tell me what I've missed. :P I should point out that, while these are based on gtk-3-22, that's only really because it was easier (jhbuild woes) and more comprehensive (old non-smooth scroll events) to test. I'm aware that changing how pressed directions affect the value at this late a point in GTK+ 3 - even if you'd agree with me that it's fixing wrong behaviours and improving others - may still be considered disruptive and lead to complaints. Personally, I'd want to see this fixed in GTK+ 3, but then maybe other users who might've once felt the same have since gotten used to the current behaviours. I think we have 3 related issues here: * the inverted H scrolling on H+RTL ranges. This is pretty clearly just a bug, and I'm very surprised (or bad at searching) not to find a report about it. * the fact that moving up/down doesn't do the same thing when scrolling as it does with keys. The responses in Bug 407242 indicate this is generally seen as counterintuitive, and more so, it contradicts what we do for scrolling. * H movement on V ranges doesn't honour the locale's start/end dirs, instead always taking left/right to dec/increase value. This may be a rarely used feature, but I figure we might as well make it consistent while we're here. I'd welcome thoughts on the desirability/feasibility of fixing these in GTK+ 3. Anyway, if it's a no for 3, hopefully we can get it into 4, and the patches only require minimal rebasing to apply there.
Any thoughts on this one yet, for either version?
from Benjamin in IRC: > anyway, I agree with your assessment in comment 3 > though I can't speak for what RTL people want > because I don't know if "increase" means left or right for them > I just know that https://he.wikipedia.org/wiki/%D7%A6%D7%99%D7%A8_%D7%94%D7%9E%D7%A1%D7%A4%D7%A8%D7%99%D7%9D has the number line increasing to the right I has reasoned that we should treat the locale's start/end as meaning the range's min/max over vertical ranges - because we do that when visually inverting horizontal ranges - but this may not follow. AFAICS, such locales don't invert the x-axis in Cartesian space, and that may be a better predictor of what users expect (absent the visual feedback of an inverted horizontal range). So, I think that part of the patch should probably get removed. > i didn't actually read the patches > also I'm not sure we want to change these things in a stable branch > but i'm not keeping track of stable anyway > so i don't know if point releases changing behavior are generally considered okay So it seems the rest stands to reason, barring any specific changes in code review, but we must decide how the balance tips, at least in gtk-3-22, between * how wrong/buggy the current behaviours are considered to be * how likely people are to have (A) noticed and (B) internalised them * and whether anyone who did will actively dislike the fix... summaring my PoV from Comment 9: they did complain about the keys, and as for horz scrolls on horz RTL/inverted ranges, I can't imagine many users noticing the bug & going (A) 'this is fine' & (B) 'in fact I'll be mad if they change it' In conclusion: meh. I guess I'll let this sit for a bit longer and see whether any other feedback arrives, in terms of the code and its prospects for 3.22 Feedback there would be very welcome!
Review of attachment 366150 [details] [review]: ::: gtk/gtkrange.c @@ +3076,3 @@ + { + move_orientation = GTK_ORIENTATION_HORIZONTAL; + delta = dx * scroll_unit; Nit: these 2 lines are indented too far.
Created attachment 371111 [details] [review] Range: Add should_invert_move() for scrolls & keys This will be used in subsequent commits to fix the sign by which the value is changed in response to directional scroll or keypress events. The idea is: you have a movement to make – in the form of a delta that follows widget directions, i.e. −1 means left or up, +1 means right or down – and you want to know whether that delta needs to be inverted in order to produce the intuitively expected directional change of :value. The existing should_invert() is not sufficient: it just determines whether to invert visually, but we need more nuance than that for input. To answer that – while not doubling up the work for scrolls and keys – I add a helper should_invert_move(), which considers other relevant state: • A parallel movement on priv->orientation should just use the existing should_invert(), which already worked OK for this case (not others). • Movements on the other orientation now depend on priv->orientation: ◦ For a horizontal Range, always invert, so up (i.e. −ve in terms of widget coords) always means increase value & vice-versa. This was done in get_wheel_delta(), but move it here for use with keys too. ◦ For a vertical Range, ignore :invert as it’s only relevant to the parallel orientation. Do not care about text direction here either as RTL locales do not invert number lines, Cartesian plots, etc. This returns TRUE if the delta should be inverted before applying to the value, and we can now use this function in both scroll and key handlers. https://bugzilla.gnome.org/show_bug.cgi?id=407242
Created attachment 371112 [details] [review] Range: Use should_invert_move() to scroll value This fixes RTL and/or :inverted Ranges responding to a horizontal scroll by moving the value/slider button in the opposite direction... See prev.
Created attachment 371113 [details] [review] Range: Make down/up keys act like down/up scrolls Before now, down/up keys on H Ranges would increase/decrease value resp, which is unintuitive & worse, contradicts what we already do for scrolls Fix simply by moving to the new should_invert_move() as scrolls just did – which also gets us the other benefits explained in the last 2 commits. https://bugzilla.gnome.org/show_bug.cgi?id=407242
*** Bug 407242 has been marked as a duplicate of this bug. ***
Attachment 371111 [details] pushed as bc2a38a - Range: Add should_invert_move() for scrolls & keys Attachment 371113 [details] pushed as 45c8c8f - Range: Make down/up keys act like down/up scrolls
I'm leaving GTK+ 3 alone for now (against my every selfish instinct!) to see if anyone else has a strong opinion either way about whether these should go there.
> dboles: > garnacho: did you have any opinion for these patches in gtk+ 3? imo the > scrolling one is a clear bug, but not sure how you feel about the up/down > keypresses? they're 'wrong' in that they contradict up/down scrolling, but is > that a strong enough reason, or will other people have gotten used to that and > be annoyed if it changes... > > garnacho: > dboles: that's my biggest fear with that for gtk3 overall, it will probably > just feel "wrong" for another group of people > dboles: but yeah, probably best to tackle the scroll events one and so: https://gitlab.gnome.org/GNOME/gtk/compare/0c46d94ec3f93c2b43bc8629d7c064d6c9732ac7...7ff9222600a8ca8b84da8cc5ec7a351ff1689cb8 marking as fixed as there's not much point keeping it open, since no one except me ever comments... and we can start a discussion about cherry-picking the key handling another time if anyone wants one.