Closed Bug 655005 Opened 9 years ago Closed 9 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

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...
http://hg.mozilla.org/mozilla-central/rev/35979ec6850a
Status: NEW → RESOLVED
Closed: 9 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.