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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 530302 [details] [diff] [review]
Patch

Is this just a rename patch?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 530321 [details] [diff] [review]
Patch v0.2

Do we have any tests for this code? Even LTR tests?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 8

[Anas Husseini [:linostar]]

r=me also for this patch.

@mfinkle the only scrollbars test we have, AFAICT, doesn't test their positions.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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...

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/35979ec6850a

Comment 11

[Andreea Pod]

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)