Closed Bug 297780 Opened 19 years ago Closed 17 years ago

undetermined progress meter goes left to right in RTL interface

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha3

People

(Reporter: linxspider, Assigned: sciguyryan)

References

(Blocks 1 open bug)

Details

(Keywords: rtl, testcase)

Attachments

(2 files, 2 obsolete files)

For checkin (v1.2)
1.75 KB, patch
Details | Diff | Splinter Review
unit test
2.40 KB, application/vnd.mozilla.xul+xml
Details 
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
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
Blocks: 306980
Keywords: helpwanted
Priority: P2 → P4
I'll try and take a look at this :) Assigning to me so I remember.
Assignee: mano → bugs
Status: ASSIGNED → NEW
Attached patch Patch v1 (obsolete) — — Splinter Review 
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)
Status: NEW → ASSIGNED
Attachment #253517 - Flags: second-review?(gavin.sharp)
Attachment #253517 - Flags: first-review?(mano)
Attachment #253517 - Flags: first-review?(gavin.sharp)
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
Attached patch Path v1.1 (obsolete) — — Splinter Review 
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+
(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.
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
Whiteboard: [checkin needed]
Summary: progress meter goes left to right in RTL interface → undetermined progress meter goes left to right in RTL interface
Attached file 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
Checked in:
mozilla/toolkit/content/widgets/progressmeter.xml  1.9

The testcase needs to be checked in, still.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Keywords: helpwanted → testcase
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [checkin needed - testcase]
Is this any different than bug 143344?
(In reply to comment #10)
> Is this any different than bug 143344?
> 

Seems to be the same bug.
(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]
(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
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: