Changes for AMO v3.2 RTL theme support

RESOLVED FIXED in 3.2

Status

addons.mozilla.org Graveyard
Public Pages
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: tomer, Assigned: Chris Pollett)

Tracking

(Blocks: 1 bug, {rtl})

Dependency tree / graph

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 308146 [details]
Screenshot - the current display of [staging] Hebrew AMO

As I said in bug 375055, AMO v3.2 looks terrible in RTL (Hebrew/Arabic/Persian). This bug should track the CSS changes required to make it usable for these audiences. 

It is up to your choice of including the changes in the screen.css or creating yet another file for RTL.
(Reporter)

Comment 1

10 years ago
Changes required for the upper Mozilla logo -

#moz {
  left: 0;
}

h1#moz a {
  padding-right: 10px; /* Instead of padding-left */
  padding-left: inherit; 
}

I think that it is not the best solution as it make the image about 1 pixel right to the en-us version.

Updated

10 years ago
Assignee: nobody → cpollett

Updated

10 years ago
Assignee: cpollett → nobody
Component: Localization → Public Pages
QA Contact: l10n → web-ui
Target Milestone: --- → 3.2

Updated

10 years ago
Assignee: nobody → cpollett
(Assignee)

Comment 2

10 years ago
Created attachment 308787 [details] [diff] [review]
a first pass at getting a rtl  styles working

This patch includes my stuff to fix Bug 417409 and Bug 415504 as well. Patches for these seemed to get a positive response on the individual bugs but wasn't sure if I could apply them yet or not. Also, to get RTL support for the search bar involved re-working the code for the search bar anyway. The basic problem
to be solved with the original search bar was that it was implemented using a styled unordered list. Why this was done I was not sure, as there was little semantic meaning being captured by the list. Worse, it meant when you tried to do things rtl it appeared backwards. So I got rid of the list to fix this.

My general approach for the rest of the page was to add a class attribute to the
root html tag. This was done in the same place where the dir attribute for the page was set. So the PHP variable used should be always correctly set. For a rtl language one might get an html tag that looks like:
<html lang="he" dir="rtl" class="html-rtl"> 
similarly for an ltr language one might get a tag that looks like: 
<html lang="en-US" dir="ltr" class="html-ltr">
The reason why I added a class attribute is that I don't think IE support attribute selectors. I then went through screen.css and color.css and if there was a rule that was ltr specific I converted it to three rules:

label {common styles}
.html-ltr label {styles for this label that are ltr specific}
.html-rtl label {styles for this label that are rtl specific}

Some additional minor tweaks to the mozilla.thtml layout had to be done to
get the magnifying class to appear correctly.

My thought is that if this first patch is happy for people we could apply it and then make incremental further patches to fix other problems as they come up.
Attachment #308787 - Flags: review?(clouserw)
(Assignee)

Comment 3

10 years ago
Created attachment 308790 [details] [diff] [review]
Notice html tag only support i18n attributes so moved my class to body tag which supports coreattr

This is essentially the same patch as before except I noticed that the class attribute can't be applied to the html tag in HTML 4.01 so I moved it to the body
tag. This is slightly less convenient as before the rtl-ltr settings were done in the same place. Still, otherwise, it works the same as before. Also, I meant to mention in my last post that I noticed in footer.thtml explicitly sets the dir attribute on the language selector. Since, I was guessing this was done for some purpose I didn't touch it.
Attachment #308787 - Attachment is obsolete: true
Attachment #308790 - Flags: review?(clouserw)
Attachment #308787 - Flags: review?(clouserw)

Updated

10 years ago
Attachment #308790 - Attachment is patch: true
Attachment #308790 - Attachment mime type: application/octet-stream → text/plain
This doesn't patch cleanly for me.  Can you make sure you're up to date and re-upload a patch?



patching file webroot/css/screen.css
Hunk #1 FAILED at 34.
1 out of 1 hunk FAILED -- saving rejects to file webroot/css/screen.css.rej
patching file webroot/css/color.css
Hunk #1 FAILED at 56.
1 out of 1 hunk FAILED -- saving rejects to file webroot/css/color.css.rej
patching file views/layouts/mozilla.thtml
Hunk #2 succeeded at 231 with fuzz 1 (offset 1 line).
patching file views/elements/search.thtml
Hunk #1 FAILED at 63.
Hunk #2 succeeded at 77 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file views/elements/search.thtml.rej
(Assignee)

Comment 5

10 years ago
Created attachment 308980 [details] [diff] [review]
There were a bunch of css changes made after I made patch, so svn upped and resolved

I guess there were I bunch of css changes made after I made the patch. So I svn upped and resolved them like you asked.
Attachment #308790 - Attachment is obsolete: true
Attachment #308980 - Flags: review?(clouserw)
Attachment #308790 - Flags: review?(clouserw)
Comment on attachment 308980 [details] [diff] [review]
There were a bunch of css changes made after I made patch, so svn upped and resolved

Thanks.  It's a big improvement over what we have.  Tomer, can you have a look as well?
Attachment #308980 - Flags: review?(clouserw) → review+
(Reporter)

Comment 7

10 years ago
(In reply to comment #6)
> (From update of attachment 308980 [details] [diff] [review])
>  Tomer, can you have a look as well?
> 

I can't see it live on remora.stage.mozilla.com/he. Is there another place where I can see it? (I can't find how to load it directly into firebug)

(Assignee)

Comment 8

10 years ago
Okay, I svn checked-in my changes. I suspect there will be additional things that need to be modified, so I'll leave this bug open.
(Assignee)

Comment 9

10 years ago
It's live at:

http://remora.stage.mozilla.com/he/firefox/
(Reporter)

Comment 10

10 years ago
A. The Mozilla logo, imho should be justified to the right, much like the other languages. This will make it more similar to the look of other Mozilla websites, and will remove the requirement to recreate the image to make rounded borders at the left side. (See comment #1)

B. Other languages at the bottom is not RTL'ed. It should be "[Languages drop down] :Other_languages". In other words, it has no dir=rtl.

C. All the arrows should be flipped. "category_extra_see_all >" should be "< category_extra_see_all". 

D. "Register | Login" - The pipe in the middle is our of sync. Currently it is "הרשמה כניסה |" ("Login Register |").


As for translation, I should look more deeply into that. I understood that we are 100% with Hebrew but probably we still not.
(Assignee)

Comment 11

10 years ago
Created attachment 309548 [details] [diff] [review]
This patch incorporates the changes tomer suggested.

The patch incorporates what tomer suggested.I debated with myself (clouserw) a little over how other languages should appear, but went with what tomer suggested.
Attachment #309548 - Flags: review?(clouserw)

Updated

10 years ago
Attachment #309548 - Attachment is patch: true
Attachment #309548 - Attachment mime type: application/octet-stream → text/plain
I left the "Other Languages" dropdown as dir="ltr" to fix rendering problems.  By removing it you get problems with parenthesis, like:

(English (US

I don't know of a way to fix this (this is why we no longer display the username in the top right of the pages).
(Reporter)

Comment 13

10 years ago
(In reply to comment #12)
> I don't know of a way to fix this (this is why we no longer display the
> username in the top right of the pages).
> 

This actually can be solved by adding LRM (U+200E) character right after the parenthesis close tag (think about it as kind of invisible English character), but I think it will be better to serve it as dir="ltr" and in case it will be needed we'll add the right character manually (i.e. "Hebrew (Israel)[RLM]"). 
(Assignee)

Comment 14

10 years ago
On my how machine have switched the select as desired. Is the patch otherwise okay, or should I submit a new version with the change?
Comment on attachment 309548 [details] [diff] [review]
This patch incorporates the changes tomer suggested.

wfm
Attachment #309548 - Flags: review?(clouserw) → review+
(Assignee)

Comment 16

10 years ago
Gahh...
I forgot to make one change:

#nav-user li { border-left: 1px solid #666; }
to
.html-ltr #nav-user li { border-left: 1px solid #666; }
.html-rtl #nav-user li { border-right: 1px solid #666; }

is it okay to check this in?
go for it, this is looking really good. Nice job.

Updated

10 years ago
Blocks: 412273
(Assignee)

Comment 18

10 years ago
I think stuff is mainly working now. As this bug seems to be blocking other stuff and is pretty much resolved, I am marking it as such.

Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 415597

Updated

10 years ago
No longer blocks: 412273

Comment 19

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

Updated

10 years ago
Blocks: 412273

Updated

9 years ago
No longer blocks: 415597

Updated

9 years ago
Blocks: 285718
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.