Closed Bug 421680 Opened 17 years ago Closed 17 years ago

Changes for AMO v3.2 RTL theme support

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect)

Product:
addons.mozilla.org Graveyard
Component:
Public Pages
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tomer, Assigned: cpollett)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: rtl)

Attachments

(3 files, 2 obsolete files)

Screenshot - the current display of [staging] Hebrew AMO
(deleted), image/png
Details
There were a bunch of css changes made after I made patch, so svn upped and resolved
(deleted), patch
clouserw
: review+
Details | Diff | Splinter Review
This patch incorporates the changes tomer suggested.
(deleted), patch
clouserw
: review+
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → cpollett
Assignee: cpollett → nobody
Component: Localization → Public Pages
QA Contact: l10n → web-ui
Target Milestone: --- → 3.2
Assignee: nobody → cpollett
Attached patch a first pass at getting a rtl styles working (obsolete) (deleted) — — Splinter Review
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)
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)
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
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+
(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)
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.
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.
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)
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).
(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]").
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+
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.
Blocks: Persian-Fx3.5
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
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: fx35-l10n-fa
No longer blocks: Persian-Fx3.5
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
No longer blocks: fx35-l10n-fa
Blocks: Persian
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: