343104 - In RTL UI, tabbrowser's scroll arrows are scrolling in reverse

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Asaf Romano (gone)]

Found this while working on bug 343097,

Under RTL UI, the tabbrowser's scroll arrows are scrolling in reverse.

Comment 1

[Asaf Romano (gone)]

So other than scrolling in reverse, which might be something the caller should take care of (i.e. calling scrollByIndex with foo*-1). In some cases it doesn't scroll at all.

nsScrollBoxObject::ScrollByIndex doesn't look rtl-sensitive to me, but i'm not going to pretend I know the layout xul code...

Comment 2

[(not reading, please use [e-mail] instead)]

I could have sworn that I made this for for rtl (and that I even tested it!) but on closer inspection of scrollbox.xml I see it only for scrollByPixels and not scrollByIndex.

in scrollByPixels (which should work on drag of tabs) the caller does the foo*-1 work.  I think scrollByIndex (which will get called on clicking) should do the same.

asaf, what do you think?

Comment 3

[Asaf Romano (gone)]

Not that XUL is frozen or something, but scrollByIndex isn't a new method (unlike scrollByPixels), so this would be a behavior change (which might break existing callers, probably not though); no strong opinion from me on this. :)

Anyway, doing the foo*=-1 thing isn't enough here, see comment 1 :-/

Comment 4

[(not reading, please use [e-mail] instead)]

asaf, to clarify, this is just scrollByIndex (on click) and not scrollByPixels, which can be tested by drag and drop of tabs.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

The code in ScrollByIndex, which finds the current rect per the scroll position did wrong calculations in the RTL case.

(No need to flip the index in scrollbox.xml).

Comment 6

[Simon Montagu :smontagu]

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

>+        if (scrolledBox->IsNormalDirection()) {
>+          diff = rect.x + rect.width/2; // use the center, to avoid rounding errors
>+          if (diff > cp.x) {
>+            break;
>+          }
>+        } else {
>+          // In right-to-left mode, the elements are ordered
>+          // in reverse (visually)
>+          diff = rect.x - rect.width/2;

At first reading I thought this was wrong, because rect.x is still the left edge of the rectangle, so the center is still rect.x + rect.width/2. On mentally stepping through it seems OK, but I think it needs some documentation of what exactly is going on. For example, I am still unclear what scrollableView->GetScrollPosition() is returning in the RTL case.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it should be rect.x + rect.width/2 myself...

Comment 8

[Asaf Romano (gone)]

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

Thinking about this again, I managed to get the center of the wrong element.

Comment 9

[Simon Montagu :smontagu]

Attachment: patch

Comment 10

[Uri Bernstein]

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

Looks good to me.

Comment 11

[Simon Montagu :smontagu]

Patch checked in on trunk

Comment 12

[Simon Montagu :smontagu]

Checked in to MOZILLA_1_8_BRANCH, with GetPresShell(PR_FALSE) changed to GetPresShell() to unmerge bug 329181.

Comment 13

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).