Closed
Bug 297780
Opened 20 years ago
Closed 18 years ago
undetermined progress meter goes left to right in RTL interface
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha3
People
(Reporter: linxspider, Assigned: sciguyryan)
References
(Blocks 1 open bug)
Details
(Keywords: rtl, testcase)
Attachments
(2 files, 2 obsolete files)
|
1.75 KB,
patch
|
Details | Diff | Splinter Review | |
|
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
|
Reporter | |
Updated•20 years ago
|
Assignee: nobody → bugs.mano
QA Contact: general → smontagu
|
||
Updated•20 years ago
|
Component: General → XUL Widgets
Keywords: helpwanted
Priority: -- → P3
Product: Firefox → Toolkit
QA Contact: smontagu → xul.widgets
Version: Trunk → unspecified
|
|
||
Updated•20 years ago
|
|
|
||
Updated•20 years ago
|
Target Milestone: mozilla1.8beta3 → mozilla1.9alpha1
|
|
||
Updated•18 years ago
|
Keywords: helpwanted
Priority: P2 → P4
|
Assignee | |
Comment 1•18 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•18 years ago
|
||
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•18 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Updated•18 years ago
|
Attachment #253517 -
Flags: second-review?(gavin.sharp)
Attachment #253517 -
Flags: first-review?(mano)
Attachment #253517 -
Flags: first-review?(gavin.sharp)
|
|
Assignee | |
Updated•18 years ago
|
Attachment #253517 -
Flags: second-review?(gavin.sharp)
|
|
||
Comment 3•18 years ago
|
||
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-
|
|
||
Updated•18 years ago
|
Priority: P4 → --
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha3
|
|
Assignee | |
Comment 4•18 years ago
|
||
Patch v1.1
* Addresses Mano's nits.
Attachment #253517 -
Attachment is obsolete: true
Attachment #254981 -
Flags: first-review?(mano)
|
|
||
Comment 5•18 years ago
|
||
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•18 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•18 years ago
|
||
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•18 years ago
|
Whiteboard: [checkin needed]
|
||
Updated•18 years ago
|
Summary: progress meter goes left to right in RTL interface → undetermined progress meter goes left to right in RTL interface
|
||
Comment 8•18 years ago
|
||
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•18 years ago
|
||
Checked in:
mozilla/toolkit/content/widgets/progressmeter.xml 1.9
The testcase needs to be checked in, still.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite?
Keywords: helpwanted → testcase
Resolution: --- → FIXED
Whiteboard: [checkin needed] → [checkin needed - testcase]
|
||
Comment 10•18 years ago
|
||
Is this any different than bug 143344?
|
|
Assignee | |
Comment 11•18 years ago
|
||
(In reply to comment #10)
> Is this any different than bug 143344?
>
Seems to be the same bug.
|
|
||
Comment 12•18 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•18 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•18 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•17 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.
Description
•