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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.2
People
(Reporter: tomer, Assigned: cpollett)
References
(Blocks 1 open bug, )
Details
(Keywords: rtl)
Attachments
(3 files, 2 obsolete files)
144.59 KB,
image/png
|
Details | |
13.30 KB,
patch
|
clouserw
:
review+
|
Details | Diff | Splinter Review |
8.13 KB,
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.
Reporter | ||
Comment 1•17 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•17 years ago
|
Assignee: nobody → cpollett
Updated•17 years ago
|
Assignee: cpollett → nobody
Component: Localization → Public Pages
QA Contact: l10n → web-ui
Target Milestone: --- → 3.2
Updated•17 years ago
|
Assignee: nobody → cpollett
Assignee | ||
Comment 2•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Attachment #308790 -
Attachment is patch: true
Attachment #308790 -
Attachment mime type: application/octet-stream → text/plain
Comment 4•17 years ago
|
||
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•17 years ago
|
||
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 6•17 years ago
|
||
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•17 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•17 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•17 years ago
|
||
It's live at:
http://remora.stage.mozilla.com/he/firefox/
Reporter | ||
Comment 10•17 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•17 years ago
|
||
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•17 years ago
|
Attachment #309548 -
Attachment is patch: true
Attachment #309548 -
Attachment mime type: application/octet-stream → text/plain
Comment 12•17 years ago
|
||
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•17 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•17 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 15•17 years ago
|
||
Comment on attachment 309548 [details] [diff] [review]
This patch incorporates the changes tomer suggested.
wfm
Attachment #309548 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 16•17 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?
Comment 17•17 years ago
|
||
go for it, this is looking really good. Nice job.
Updated•17 years ago
|
Blocks: Persian-Fx3.5
Assignee | ||
Comment 18•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Blocks: fx35-l10n-fa
Updated•17 years ago
|
No longer blocks: Persian-Fx3.5
Comment 19•17 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Updated•17 years ago
|
Blocks: Persian-Fx3.5
Updated•16 years ago
|
No longer blocks: fx35-l10n-fa
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•