[RTL] Wizard widget should get mirrored buttons on GTK

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: tomer, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {fixed1.9.1, polish, rtl})

Trunk
mozilla1.9.2a1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy] [polish-interactive][polish-p2])

Attachments

(2 attachments)

Reporter

Description

11 years ago
The RTL Profile Wizard on Linux has at the bottom two buttons with arrows for next and backward. We should get them mirrored in RTL mode.
Assignee

Updated

11 years ago
Blocks: fx35-l10n-fa
Assignee

Updated

11 years ago
Assignee

Updated

11 years ago
No longer blocks: Persian-Fx3.5
Reporter

Updated

11 years ago
Component: he-IL / Hebrew → Theme
Product: Mozilla Localizations → Firefox
QA Contact: hebrew.he → theme
Assignee

Comment 1

11 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Assignee

Comment 2

11 years ago
Posted patch Patch (v1)Splinter Review
This patch causes the wizard widget to use the RTL or LTR specific versions of the go-back and go-forward icons.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #359994 - Flags: review?(dao)
Assignee

Updated

11 years ago
Summary: [RTL] Profile Wizard should get mirrored buttons on GTK → [RTL] Wizard widget should get mirrored buttons on GTK
Version: unspecified → Trunk
Assignee

Updated

11 years ago
Duplicate of this bug: 471112
Assignee

Updated

11 years ago
Component: Theme → XUL Widgets
Product: Firefox → Toolkit
QA Contact: theme → xul.widgets
Attachment #359994 - Flags: review?(dao) → review+
Comment on attachment 359994 [details] [diff] [review]
Patch (v1)

>               <xul:button label="&button-next-unix.label;" accesskey="&button-next-unix.accesskey;"
>
>                           class="wizard-button" dlgtype="next" icon="go-forward" 
>
>-                          default="true" flex="1"/> 
>
>+                          chromedir="&locale.dir;" default="true" flex="1"/> 

>               <xul:button label="&button-next-win.label;" accesskey="&button-next-win.accesskey;"
>
>                           class="wizard-button" dlgtype="next" icon="go-forward" 
>
>-                          default="true" flex="1"/> 
>
>+                          chromedir="&locale.dir;" default="true" flex="1"/> 

superfluous spaces after />
Component: XUL Widgets → Themes
QA Contact: xul.widgets → themes
Assignee

Comment 5

11 years ago
http://hg.mozilla.org/mozilla-central/rev/3d2cbfcf9217
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee

Comment 6

11 years ago
Comment on attachment 359994 [details] [diff] [review]
Patch (v1)

Another theme-only and low risk patch which is nice to have for RTL locales.
Attachment #359994 - Flags: approval1.9.1?
Keywords: polish
Whiteboard: [polish-easy] [polish-interactive]
Attachment #359994 - Flags: approval1.9.1? → approval1.9.1+
Assignee

Updated

10 years ago
Blocks: 289886
I don't like what was done here - you've basically checked in code which was denied long ago by "gtk peers" (see bug 289886) without even getting a toolkit peer review (there's no such thing "theme only", esp. not when toolkit/content/widgets is in your way).
For future reference: http://www.mozilla.org/projects/toolkit/review.html

Theme-*only* changes policy is a little less strict in certain cases (clean up, and in cases like this I would still indeed for a first-review from someone who can test it).
It's technically different code. Bug 289886 used gtk-go-back where this uses gtk-go-forward-rtl.
That's all good. I think though that part of the sr- was on using chromedir for choosing a -moz-apperance rather than detrcing the direction on the back-end side). Having all that said Dão, my last comments stands - peer reviews are required for all toolkit/content/widgets checkins.
Assignee

Comment 13

10 years ago
Sorry for messing up here.  I backed out the patch on both trunk and 1.9.1 branch.
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
Assignee

Updated

10 years ago
Attachment #359994 - Flags: review?(mano)
Assignee

Updated

10 years ago
Attachment #359994 - Flags: approval1.9.1+
Assignee

Comment 15

10 years ago
Landed again on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/5c8cf652ca93>
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Assignee

Comment 16

10 years ago
Comment on attachment 359994 [details] [diff] [review]
Patch (v1)

Requesting approval again.  This is the same patch which was approved before for 1.9.1.
Attachment #359994 - Flags: approval1.9.1?
Comment on attachment 359994 [details] [diff] [review]
Patch (v1)

a191=beltzner, please add a Litmus test?
Attachment #359994 - Flags: approval1.9.1? → approval1.9.1+
Assignee

Comment 18

10 years ago
(In reply to comment #17)
> (From update of attachment 359994 [details] [diff] [review])
> a191=beltzner, please add a Litmus test?

We're in the process of adding Litmus tests for many of the RTL bugs including this one.
Assignee

Updated

10 years ago
No longer blocks: fx35-l10n-fa
Assignee

Updated

10 years ago
Blocks: Persian
This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.
Whiteboard: [polish-easy] [polish-interactive] → [polish-easy] [polish-interactive][polish-p2]
Assignee

Updated

6 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.