undetermined progress meter goes left to right in RTL interface

RESOLVED FIXED in mozilla1.9alpha3

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Reuven Gonen, Assigned: sciguyryan)

Tracking

(Blocks: 1 bug, {rtl, testcase})

unspecified
mozilla1.9alpha3
x86
Windows XP
rtl, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.8b2) Gecko/20050614 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.8b2) Gecko/20050614 Firefox/1.0+

In rtl interface the progressmeter goes from left to right when it's looking for
updates/plugins. when downloading the update/plugin, the progress meter appears
right.

Reproducible: Always

Steps to Reproduce:
1. download an he nightly from
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk-l10n/
2. go to help->check for updates... 
3. the progress meter goes from left to right.



my guess is that the code for this is here: 
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/progressmeter.xml#57
(Reporter)

Updated

13 years ago
Assignee: nobody → bugs.mano
QA Contact: general → smontagu
Component: General → XUL Widgets
Keywords: helpwanted
Priority: -- → P3
Product: Firefox → Toolkit
QA Contact: smontagu → xul.widgets
Version: Trunk → unspecified
Status: NEW → ASSIGNED
Keywords: helpwanted
Priority: P3 → P2
Target Milestone: --- → mozilla1.8beta3
Target Milestone: mozilla1.8beta3 → mozilla1.9alpha1
(Reporter)

Updated

12 years ago
Blocks: 306980
Keywords: helpwanted
Priority: P2 → P4
(Assignee)

Comment 1

11 years ago
I'll try and take a look at this :) Assigning to me so I remember.
Assignee: mano → bugs
Status: ASSIGNED → NEW
(Assignee)

Comment 2

11 years ago
Created attachment 253517 [details] [diff] [review]
Patch v1

Patch v1

* This patch allows the direction of the progress bar to be altered by setting the direction via css. This seems to work so I'll request a first-review from Gavin.
Attachment #253517 - Flags: first-review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #253517 - Flags: second-review?(gavin.sharp)
Attachment #253517 - Flags: first-review?(mano)
Attachment #253517 - Flags: first-review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Attachment #253517 - Flags: second-review?(gavin.sharp)
Comment on attachment 253517 [details] [diff] [review]
Patch v1

just some nits

>Index: toolkit/content/widgets/progressmeter.xml
>===================================================================
>     <implementation>
>       <method name="init">
>         <body><![CDATA[
>           var stack = document.getAnonymousElementByAttribute(this, "anonid", "stack");
>           var spacer = document.getAnonymousElementByAttribute(this, "anonid", "spacer");
>-          var position = -1;
>+
>+          var is_rtl = 
>+            (window.getComputedStyle(stack, null).direction == "rtl");

* your code path would be simpler if you make that isLTR.
* pass |this| (the progressmeter element) instead of the stack node to getComputedStyle
* prefer document.defaultView over the global window object
* remove the extra parentheses.

>+
>+          var position = (!is_rtl) ? -1 : 4;

here too.
Attachment #253517 - Flags: first-review?(mano) → first-review-
Priority: P4 → --
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha3
(Assignee)

Comment 4

11 years ago
Created attachment 254981 [details] [diff] [review]
Path v1.1

Patch v1.1

* Addresses Mano's nits.
Attachment #253517 - Attachment is obsolete: true
Attachment #254981 - Flags: first-review?(mano)
Comment on attachment 254981 [details] [diff] [review]
Path v1.1

>Index: toolkit/content/widgets/progressmeter.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/progressmeter.xml,v
>retrieving revision 1.8
>diff -u -8 -p -r1.8 progressmeter.xml
>--- toolkit/content/widgets/progressmeter.xml	11 Sep 2006 13:55:19 -0000	1.8
>+++ toolkit/content/widgets/progressmeter.xml	13 Feb 2007 19:57:12 -0000
>@@ -61,26 +61,37 @@
>       </xul:stack>
>     </content>
> 
>     <implementation>
>       <method name="init">
>         <body><![CDATA[
>           var stack = document.getAnonymousElementByAttribute(this, "anonid", "stack");
>           var spacer = document.getAnonymousElementByAttribute(this, "anonid", "spacer");
>-          var position = -1;
>+
>+          var isLTR = 
>+           document.defaultView.getComputedStyle(this, null).direction != "rtl";
>+

make that == "ltr" (this is not the dir attribute, the only possible values are "ltr" and "rtl").

r=mano otheriwse.
Attachment #254981 - Flags: first-review?(mano) → first-review+
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> make that == "ltr" (this is not the dir attribute, the only possible values are
> "ltr" and "rtl").
> 
> r=mano otheriwse.
> 

Ah OK. I wasn't sure if null (as in not set) would constitute either of those but as it defaults to ltr then that's easily fixed.

Thanks for reviewing Mano, final patch coming up.
(Assignee)

Comment 7

11 years ago
Created attachment 254990 [details] [diff] [review]
For checkin (v1.2)

Patch v1.2 (for checkin)

* Updated to Mano's final nit: check |direction == "ltr"| directly instead of |direction != "rtl"|.
Attachment #254981 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]
Summary: progress meter goes left to right in RTL interface → undetermined progress meter goes left to right in RTL interface

Comment 8

11 years ago
Created attachment 255030 [details]
unit test

Most of the time I spent writing this test was devoted to looking up XUL and XBL trivia--everyone CCed on this bug probably knows that stuff better than I do, so

Comment 9

11 years ago
Checked in:
mozilla/toolkit/content/widgets/progressmeter.xml  1.9

The testcase needs to be checked in, still.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Keywords: helpwanted → testcase
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [checkin needed - testcase]
Is this any different than bug 143344?
(Assignee)

Comment 11

11 years ago
(In reply to comment #10)
> Is this any different than bug 143344?
> 

Seems to be the same bug.

Comment 12

11 years ago
(In reply to comment #8)
> Created an attachment (id=255030) [details]
> unit test
> 
This is not an automated test though, so I don't supposed it should be checked in as a mochitest, right?
Whiteboard: [checkin needed - testcase]

Comment 13

11 years ago
(In reply to comment #12)
> (In reply to comment #8)
> > Created an attachment (id=255030) [details] [details]
> > unit test
> > 
> This is not an automated test though, so I don't supposed it should be checked
> in as a mochitest, right?
> 

not as is, but it would be a one line change

Comment 14

11 years ago
OK, I suppose one needs to hook the doTest function to onload and add the proper ok() checks (which are not obvious for someone who didn't read the bug). I guess I'll leave this to someone else.

Comment 15

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