485938 - clicking the right scrolling button should snap elements on the right rather than on the left (toolkit.scrollbox.smoothScroll = false)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 370010 [details] [diff] [review]
patch

>             if (scrollLeft)
>-              x = rect.left - (rect.right - rect.left);
>+              x = rect.left - rect.width;
>             else
>-              x = rect.right + (rect.right - rect.left);
>+              x = rect.right + rect.width;

This is just cleanup... .width is new in 1.9.1.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Does this work correctly for RTL? _scrollIndex is set depending on the value of _isLTRScrollbox, but scrollByIndex also makes the same adjustment to the passed in index.

Comment 3

[Dão Gottwald [:dao]]

Yes, it does. Anything else would be a bug in the current code, i.e. RTL would already have been broken or the two scrollByIndex methods would behave inconsistently.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> i.e. RTL would already have been broken or the two scrollByIndex methods would
> behave inconsistently.

Well sure, but the latter is not as severe as the former, so how the behavior is changing is important. There certainly seems to be a bug with your patch applied - I'd like to understand what it is, and understand how it differs from any existing bugs.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> There certainly seems to be a bug with your patch applied

What exactly are you seeing?

Comment 6

[Dão Gottwald [:dao]]

If you're using the force rtl extension, make sure to open a new window, since _isLTRScrollbox is cached.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I haven't tested it, but it doesn't make sense for _isLTRScrollbox to be checked twice. Maybe I'm missing something...

Comment 8

[Dão Gottwald [:dao]]

_isLTRScrollbox is used again after _elementFromPoint(x), because the code below really wants .leftSibling and .rightSibling rather than .previousSibling and .nextSibling.

Comment 9

[Dão Gottwald [:dao]]

Fixed in bug 483552.