285136 - apply bug 221824 "themes should be RTL compatible" to thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Reuven Gonen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.8b2) Gecko/20050302 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.8b2) Gecko/20050302 Firefox/1.0+

currently Firefox can detect if the locale is RTL, and if such, change the theme
for RTL compatibility. this is not yet applied in Thunderbird.

Reproducible: Always




the link for the bug in Firefox:
http://bugzilla.mozilla.org/show_bug.cgi?id=221824

Comment 1

[Asaf Romano (gone)]

Note that as tb doesn't use winstripe, there is nothing we can take from bug
221824...

Comment 2

[Kevin Gerich]

Reuven, can you please identify the Thunderbird icons that need RTL versions?

Comment 3

[Reuven Gonen]

Attachment: list of files to modify

i'm attaching a list of files that can be modified for RTL interface.

Comment 4

[Reuven Gonen]

Attachment: main files to add

this list includes the main files in use that were added in winstripe as well.
can you please add them?

Comment 5

[Reuven Gonen]

Attachment: first patch - mainly changing margin and padding

this patch mainly deals with changing the settings padding(margin)-right(left)
to -moz-margin(padding)-end(start)to be compatible with rtl interface.

Comment 6

[Asaf Romano (gone)]

Reuven, are you working on it these days? we're going to have a real problem in
thunderbird 1.1 (with the new update system)

Comment 7

[Reuven Gonen]

(In reply to comment #6)
> Reuven, are you working on it these days? we're going to have a real problem in
> thunderbird 1.1 (with the new update system)

the patch above resolves a big part of the theme. there are still some issues
that need to get fixed, but these will be easier to identify once this patch is
commited.

also i need kevin to add the files listed in comment 4.

Comment 8

[Reuven Gonen]

Attachment: [fixed on trunk]rtl files to add

these files are needed for the RTL UI.

Comment 9

[Reuven Gonen]

Attachment: adding the files to jar.mn

Comment 10

[Reuven Gonen]

Attachment: fixing margins and padding

this patch fixes the margins and paddings to be compatible with RTL UI.

Comment 11

[Reuven Gonen]

Attachment: fixing the account central direction

this patch fixes the account central icons to be RTL compatible. this patch
requires patch 198606, in order to take effect.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 198606 [details] [diff] [review]
fixing margins and padding 

This was boring ;).

r=mano, need moa from mscott and some trunk baking.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 198610 [details] [diff] [review]
fixing the account central direction


> <page
>   xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>   xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-  onload="OnInit();">
>+  onload="OnInit();"
>+  style="direction: &locale.dir;;">

leave that part for the other bug. r=mano.

Comment 14

[Reuven Gonen]

Attachment: fix account central direction, with page direction removed

same as above, removing the page direction style.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 199023 [details] [diff] [review]
fix account central direction, with page direction removed

r=mano

Comment 16

[Scott MacGregor]

Reuven, can you give me a single cvs diff patch with all of mano's review
comments for the CSS changes + your zip file with the new rtl files to add? That
way I can check this into the trunk after I've finished reviewing it. Thanks.

Comment 17

[Reuven Gonen]

Attachment: complete patch for rtl theme

this is a complete patch, including the changes in the patches above, and
addressing mano's comments.

i added in this patch a fix for the wizard-header in the import wizard(the
initial wizard when a new profile is created).

the RTL files are included in attachment 198147 [details] (fourth attachment).

Comment 18

[Reuven Gonen]

Attachment: [fixed on trunk]complete patch for rtl theme, fixed jar.mn

seems like i removed icon64.png from jar.mn in the previous patch...

Comment 19

[Asa Dotzler [:asa]]

Too late for major theme changes. The time for something like this was in the
alpha cycles, or possibly an early beta, but not right before RC1. 

Comment 20

[Scott MacGregor]

We'll figure something out here. No one panic.

First step is for me to get this on the trunk.

Another question, what is the impact to pinstripe here? is there any?

Comment 21

[Asaf Romano (gone)]

Well, at some point, i plan to work on rtl pinstripe, not a priority though
(there's no native RTL UI in OS X).

Comment 22

[Scott MacGregor]

Comment on attachment 198147 [details]
[fixed on trunk]rtl files to add

I'm about to land this on the trunk.

Comment 23

[Scott MacGregor]

Comment on attachment 199190 [details] [diff] [review]
[fixed on trunk]complete patch for rtl theme, fixed jar.mn

I'm about to land this on the trunk. Will incorporate any additional feedback
from Asaf.

My concern about pinstripe was that some of these non theme specific CSS
changes (like migration.css) would cause things to break in pinstripe because
we don't have the right artwork.

Comment 24

[Asaf Romano (gone)]

Scott, note this doesn't seem to include attachment 198948 [details] [diff] [review].

Comment 25

[Scott MacGregor]

(In reply to comment #24)
> Scott, note this doesn't seem to include attachment 198948 [details] [diff] [review] [edit].

That attachment isn't even part of this bug :)

I'll take a look.

Comment 26

[Asaf Romano (gone)]

You know, I'm really not happy to see the "localized" artworks. This is Scott's
and MoFo/MoCo's decision though.

At the very list, you've to fix the credits background too (and probably
icon64.png, not an issue though since it's only used in Pinstripe).

Comment 27

[Scott MacGregor]

(In reply to comment #26)
> You know, I'm really not happy to see the "localized" artworks. This is Scott's
> and MoFo/MoCo's decision though.
> 


Which images are you worried about here Asaf. I had assumed the menu icons in
the zip file were from the firefox RTL winstripe theme. The official branding
artwork changes looked like our official art but flipping the orientation which
looked ok to me. Can you point me to some images your not happy with?

Comment 28

[Scott MacGregor]

Reuven, how is this looking on the trunk now that it's all checked in? We don't
have the ability to verify these changes ourselves, we need your help. 

Comment 29

[Asaf Romano (gone)]

(In reply to comment #27)

> Which images are you worried about here Asaf. I had assumed the menu icons in
> the zip file were from the firefox RTL winstripe theme. The official branding
> artwork changes looked like our official art but flipping the orientation which
> looked ok to me. Can you point me to some images your not happy with?

I referred to the flipped branding artworks. Esp. the difference this creates
between the about/credits dialog to the other "branded ui" bits (and the
official website).

Comment 30

[Reuven Gonen]

Attachment: patch for jar.mn and msgAccountCentral.xul

seems like i didn't add the fix for jar.mn inside toolkit/themes/qute/global.
I'm also adding the fix for msgAccountCentral.xul from the previous patch.

i tested the theme and it looks O.K from the RTL side as well, once the arrows
are added and the msgAccountCentral.xul is fixed.

Comment 31

[Asaf Romano (gone)]

Comment on attachment 199190 [details] [diff] [review]
[fixed on trunk]complete patch for rtl theme, fixed jar.mn

retroactive r=mano pending a resolution on the artworks thingy.

Comment 32

[Scott MacGregor]

(In reply to comment #29)
> (In reply to comment #27)
> 
> 
> I referred to the flipped branding artworks. Esp. the difference this creates
> between the about/credits dialog to the other "branded ui" bits (and the
> official website).

Are you worried about the images being flipped or you don't think they look as
good as the non RTL branded artwork?

Comment 33

[Asaf Romano (gone)]

The Former.

Comment 34

[Scott MacGregor]

Reuven, can you attach a screen shot of the about dialog with the RTL theme so I
can see what Asaf is worried about. Thanks!

Comment 35

[Asaf Romano (gone)]

Scott, the about dialog is the only place where the image _isn't_ flipped with
this patc.

Comment 36

[Reuven Gonen]

the about page looks quiet well in the hebrew version, and is also a problem to
flip since it contains "Mozilla Thunderbird" in it.

i will also attach the other windows with the images *not* fliped, to see if we
prefer to leave the branding the images untouched.

Comment 37

[Reuven Gonen]

Attachment: start page with branding watermark not flipped

Comment 38

[Reuven Gonen]

Attachment: import dialog with original image

Comment 39

[Reuven Gonen]

Attachment: about dialog

this is the generic about dialog with the original image. seems to fit well
with the hebrew text since it's on top of the text.

Comment 40

[Scott MacGregor]

I'm ok with leaving the about dialog image alone. I think it looks fine in
Reuven's about dialog screen shot.

So the question becomes, do we leave the icon alone for the start page and the
import dialog too? Or do we use the new rtl versions that went into the trunk. 

I'd like to land this on the branch today if we can figure out what the right
thing to do here is. 

Comment 41

[Asaf Romano (gone)]

Reuven, what did you do in the customized 1.0 theme?

Comment 42

[Tsahi Asher]

there is no reason to flip logos for rtl builds. the images that are most
important to be flipped are those that will change their meaning when they are
flipped, which are mostly those with arrows on them (reply, forward etc.).
lightning may also be a reason for flipping, although my position is that the
light source should be from the top-left in rtl builds as well. the logo should
be kept as in ltr builds.

Comment 43

[Reuven Gonen]

Attachment: screenshot of start page with both images

the import dialog image looks pretty good in both directions. the start page
more concers me, since the image goes above the text in some cases with the
original image, like when the window is not maximized, or the resolution is
800X600. i'm attaching a screenshot of the start page with the image in both
directions. with 800X600 resolution the image goes also above the h1, and h2
title backgrounds.

also the start page is used quiet a lot (actually, every time the client is
opened) so it's more important to have it look good.

regarding the import image, if we keep the flipped image in the start page, i
think we can leave the import rtl-flipped-image as well without it being an
exception.

we also need attachment 199680 [details] [diff] [review] checked in, in order for the theme to be RTL
compatible.

Comment 44

[Scott MacGregor]

I checked in the msgAccountCentral change on the trunk. That was just a missed
checkin from earlier.

We need to wrap this up today so I can get this onto the branch.

How can we resolve a decision on the branding art? I don't have an opinion one
way or the other really. Although life would be easier if we didn't have to have
rtl specific versions of any of the branded art. 

We could leave the official artwork alone everywhere but the start page (i.e.
make that the only rtl image we use). Or we could leave that alone too and let
some of the text overlap like the screen shot shows?

Comment 45

[Benjamin Smedberg]

My position/opinion is that the branding art should not be  altered for RTL.

Comment 46

[Tsahi Asher]

reuven, what's the connection between flipping the image and having it overlap
the text? its just a bitmap, and a pretty symetric one too (circular). if it's
not overlapping the text in the "rtl" (top) form in your lat attachment, there
is no reason for it to overlap in it's original form.

Comment 47

[Scott MacGregor]

Attachment: back out the official branding RTL artwork from the trunk

This patch removes the official branded rtl artwork, making the rtl theme use
our regular official branded artwork.

As benjamin and Tsahi pointed out, I think this is the direction we should go
in.

Comment 48

[Reuven Gonen]

(In reply to comment #47)
> Created an attachment (id=200141) [edit]
> back out the official branding RTL artwork from the trunk
> 
> This patch removes the official branded rtl artwork, making the rtl theme use
> our regular official branded artwork.

in migration.css we still need to align the image to the left in the RTL theme
like this:

.wizard-header {
background: url("chrome://branding/content/thunderbird-wizard-badge.png")
no-repeat right center Window;
}

.wizard-header[chromedir="rtl"] {
  background: url("chrome://branding/content/thunderbird-wizard-badge.png")
no-repeat left center Window;
}

Comment 49

[Reuven Gonen]

(In reply to comment #47)
> Created an attachment (id=200141) [edit]
> back out the official branding RTL artwork from the trunk
> 
> This patch removes the official branded rtl artwork, making the rtl theme use
> our regular official branded artwork.

this patch removes the startpage-back-rtl.png from start.xhtml instead of the
thunderbird-watermark-rtl.png image. I've added attachment 200193 [details] [diff] [review] in Bug 311241
that removes the branding reference and also uses the -moz styles instead of
margin-right-left styles.

Comment 50

[Scott MacGregor]

Attachment: updated fix to remove official branding RTL artwork from the trunk

Ok, this patch should address your comments. I took your patch from the other
bug. 

Let's keep all the patches here in this bug so I can preserve my sanity. I'm
already having a hard time keeping track of all the different RTL patches
across the bugs :)

Comment 51

[Scott MacGregor]

Ok, I landed everything on the branch for thunderbird 1.5. Please help test this
for me on an hourly windows build when it comes out or with tomorrow's nightly
build. With all these patches, I'm sure I forgot something somewhere and we'll
need to react quickly if I did. :)

Comment 52

[Scott MacGregor]

How's this looking on today's branch build? Reuven, are you happy with what made
it in? Did we miss anything?

Comment 53

[Reuven Gonen]

(In reply to comment #52)
> How's this looking on today's branch build? Reuven, are you happy with what made
> it in? Did we miss anything?

tested on trunk - version 1.6a1 (20051022)
tested on branch - version 1.5 (20051021)

and looks o.k in both directions (RTL and LTR)

it seems like actually everything is there  :-)

Comment 54

[Reuven Gonen]

> tested on trunk - version 1.6a1 (20051022)
> tested on branch - version 1.5 (20051021)
> 
> and looks o.k in both directions (RTL and LTR)

this also includes the start page :-)

Comment 55

[Reuven Gonen]

Attachment: fix for previous and next arrows

this patch fixes the back and next arrows that are on the primary toolbar.

Comment 56

[Reuven Gonen]

(In reply to comment #55)
> Created an attachment (id=200501) [edit]
> fix for previous and next arrows
> this patch fixes the back and next arrows that are on the primary toolbar.

any chance to get this on the branch?

Comment 57

[Scott MacGregor]

sorry man this patch came in after the deadline, it's too late for 1.5. 

Comment 58

[Asaf Romano (gone)]

Comment on attachment 200501 [details] [diff] [review]
fix for previous and next arrows

r=mano if this is still relevant.

Comment 59

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).