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)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: vingtetun, Assigned: vingtetun)
Details
Attachments
(1 file, 1 obsolete file)
1.30 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #530302 -
Flags: review?(mark.finkle)
Attachment #530302 -
Flags: review?(linux.anas)
Comment 1•14 years ago
|
||
Comment on attachment 530302 [details] [diff] [review]
Patch
Is this just a rename patch?
Assignee | ||
Comment 2•14 years ago
|
||
(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•14 years ago
|
||
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?
Assignee | ||
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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+
Assignee | ||
Comment 7•14 years ago
|
||
(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•14 years ago
|
||
r=me also for this patch.
@mfinkle the only scrollbars test we have, AFAICT, doesn't test their positions.
Assignee | ||
Comment 9•14 years ago
|
||
(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...
Assignee | ||
Comment 10•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 11•14 years ago
|
||
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.
Description
•