Closed
Bug 343104
Opened 19 years ago
Closed 18 years ago
In RTL UI, tabbrowser's scroll arrows are scrolling in reverse
Categories
(Firefox :: Tabbed Browser, defect, P1)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 2 beta2
People
(Reporter: asaf, Assigned: smontagu)
References
Details
(Keywords: fixed1.8.1, rtl)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
uriber
:
review+
roc
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
Found this while working on bug 343097,
Under RTL UI, the tabbrowser's scroll arrows are scrolling in reverse.
Reporter | ||
Updated•19 years ago
|
Flags: blocking-firefox2?
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Reporter | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Reporter | ||
Comment 1•19 years ago
|
||
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...
Updated•19 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Comment 2•19 years ago
|
||
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?
Reporter | ||
Comment 3•19 years ago
|
||
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•19 years ago
|
||
asaf, to clarify, this is just scrollByIndex (on click) and not scrollByPixels, which can be tested by drag and drop of tabs.
Reporter | ||
Comment 5•19 years ago
|
||
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).
Attachment #227866 -
Flags: superreview?(roc)
Attachment #227866 -
Flags: review?(smontagu)
Assignee | ||
Comment 6•19 years ago
|
||
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.
I think it should be rect.x + rect.width/2 myself...
Reporter | ||
Comment 8•19 years ago
|
||
Comment on attachment 227866 [details] [diff] [review]
patch
Thinking about this again, I managed to get the center of the wrong element.
Attachment #227866 -
Attachment is obsolete: true
Attachment #227866 -
Flags: superreview?(roc)
Attachment #227866 -
Flags: review?(smontagu)
Reporter | ||
Updated•19 years ago
|
Assignee: bugs.mano → smontagu
Status: ASSIGNED → NEW
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #228046 -
Flags: superreview?(roc)
Attachment #228046 -
Flags: review?(uriber)
Comment 10•19 years ago
|
||
Comment on attachment 228046 [details] [diff] [review]
patch
Looks good to me.
Attachment #228046 -
Flags: review?(uriber) → review+
Attachment #228046 -
Flags: superreview?(roc) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #228046 -
Flags: approval1.8.1?
Reporter | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [fixed on trunk]
Updated•18 years ago
|
Attachment #228046 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 12•18 years ago
|
||
Checked in to MOZILLA_1_8_BRANCH, with GetPresShell(PR_FALSE) changed to GetPresShell() to unmerge bug 329181.
Keywords: fixed1.8.1
Comment 13•17 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in
before you can comment on or make changes to this bug.
Description
•