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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 370010 [details] [diff] [review]
patch
Attachment #370010 - Flags: review?(gavin.sharp)
(Assignee)

Comment 1

9 years ago
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.
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.
(Assignee)

Comment 3

9 years ago
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.
(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.
(Assignee)

Comment 5

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

What exactly are you seeing?
(Assignee)

Comment 6

9 years ago
If you're using the force rtl extension, make sure to open a new window, since _isLTRScrollbox is cached.
I haven't tested it, but it doesn't make sense for _isLTRScrollbox to be checked twice. Maybe I'm missing something...
(Assignee)

Comment 8

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

Updated

9 years ago
Depends on: 483552
(Assignee)

Updated

9 years ago
Attachment #370010 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

9 years ago
Fixed in bug 483552.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.