Closed Bug 655005 Opened 14 years ago Closed 14 years ago

[RTL] left scrollbar is mispositioned when the control sidebar is visible

Categories

(Firefox for Android Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #530302 - Flags: review?(mark.finkle)
Attachment #530302 - Flags: review?(linux.anas)
Comment on attachment 530302 [details] [diff] [review] Patch Is this just a rename patch?
(In reply to comment #1) > Comment on attachment 530302 [details] [diff] [review] [review] > Patch > > Is this just a rename patch? Nope that's a different bug than the other rtl issue, this one is for "scrollbar" while the other concern "sidebar"
Comment on attachment 530302 [details] [diff] [review] Patch >- x = Math.round(Math.max(0, rightW * rightVis)); >+ x = Math.round(tabsW * tabsVis); Those appear to be the only non-rename part of the patch. It would be helpful to point out real changes versus name changes in a bugzilla comment :) Does this mean we are allowing x to be negative now? Or can't that happen?
(In reply to comment #3) > Comment on attachment 530302 [details] [diff] [review] [review] > Patch > > > >- x = Math.round(Math.max(0, rightW * rightVis)); > > >+ x = Math.round(tabsW * tabsVis); > > Those appear to be the only non-rename part of the patch. It would be helpful > to point out real changes versus name changes in a bugzilla comment :) Oh sorry I didn't understand sooner what you meant! > Does this mean we are allowing x to be negative now? Or can't that happen? Good catch. It can't happen as far as I can tell, I can probably remove the Math.max call for the ltr case.
Attached patch Patch v0.2Splinter Review
The patch renames a few variables from [left/right]Sidebar to [tabs/controls]Sidebar to be more understandable (initial idea for that come from Anas, but I'm not confident every cases need to be renamed). The only real changes are: * Math.max has been removed since it seems unuseful * add a direction variable that can be 1 or -1 depending on the page direction to alter the 'x' change.
Assignee: nobody → 21
Attachment #530302 - Attachment is obsolete: true
Attachment #530302 - Flags: review?(mark.finkle)
Attachment #530302 - Flags: review?(linux.anas)
Attachment #530321 - Flags: review?(mark.finkle)
Comment on attachment 530321 [details] [diff] [review] Patch v0.2 Do we have any tests for this code? Even LTR tests?
Attachment #530321 - Flags: review?(mark.finkle) → review+
(In reply to comment #6) > Comment on attachment 530321 [details] [diff] [review] [review] > Patch v0.2 > > Do we have any tests for this code? Even LTR tests? Yes we have browser_sidebars.js (around 88 tests) and they still pass.
r=me also for this patch. @mfinkle the only scrollbars test we have, AFAICT, doesn't test their positions.
(In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 530321 [details] [diff] [review] [review] [review] > > Patch v0.2 > > > > Do we have any tests for this code? Even LTR tests? > > Yes we have browser_sidebars.js (around 88 tests) and they still pass. Err, we have browser_scrollbar.js but I don't think they check for sidebar positions I will fix that...
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110510 Firefox/6.0a1 Fennec/6.0a1 Device: LG Optimus 2X (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: