594369 - Message header is not aligned to left on RTL Thunderbird

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Tomer Cohen :tomer]

Attachment: Message panel on Tb3.1 Hebrew build - Message Sender should be sticked to the right side

On Thunderbird 3.1, we've merged the sender header with the reply/forward buttons. A forum post on mozilla-il [1] hints that we have forgot to make sure it is aligned to the right window edge like other message headers. 

I'll be attaching screenshots to demonstrate the issue. These images were captured by Nethaneel. 


[1] http://mozilla.org.il/board/viewtopic.php?t=9292

Comment 1

[Tomer Cohen :tomer]

Attachment: Message panel on Tb3.1 English build - All headers are aligned to the left

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

This seems like a pretty painful usability stain.  Tomer, do you have any feel for how bad the hebrew community would perceive this to be?  Do you have any feel for how much it's likely to effect willingness to use the product?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Tomer, another question: do you have any suggestions for ways in which we could get more users of the Hebrew locale using beta and release candidate versions and giving us feedback earlier?

Comment 4

[Andreas Nilsson (:andreasn)]

Attachment: patch to fix the issue

This should do the trick. Who wants to review? Do we need a ui-r?

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: patch in action (screenshot)

Comment 6

[netanel_h]

I would like to test the patch, Is there any Hebrew build available for Win / Linux with this patch? if not, i can build it myself from source on Debian with manual adding of the patch, it will just take some more time.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> I would like to test the patch, Is there any Hebrew build available for Win /
> Linux with this patch? if not, i can build it myself from source on Debian with
> manual adding of the patch, it will just take some more time.

I think I should be able to get some test builds out with it later today.

Comment 8

[Mark Banner (:standard8)]

The builds will be available from here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-7d428b786c74/

Linux builds are already available, Windows builds will be there in about an hour.

Comment 9

[(no longer active)]

Comment on attachment 477099 [details] [diff] [review]
patch to fix the issue

Here's an unsolicited RTL-ui-r+ for you!  Thanks!

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #8)
> Linux builds are already available, Windows builds will be there in about an
> hour.

The windows build isn't going to complete without some tweaking, do you need it?

Comment 11

[netanel_h]

(In reply to comment #10)
> (In reply to comment #8)
> > Linux builds are already available, Windows builds will be there in about an
> > hour.
> 
> The windows build isn't going to complete without some tweaking, do you need
> it?

I use Win as testing OS, so when there is Linux version it is enough, and obviously the patch fixed the issue, thanks!

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 477099 [details] [diff] [review]
patch to fix the issue

>+++ b/mail/themes/gnomestripe/mail/messageHeader.css
>@@ -679,3 +679,11 @@
> .hdrTrashButton > .button-box > .button-icon {
>   list-style-image: url("moz-icon://stock/gtk-delete?size=menu");
> }
>+
>+#header-view-toolbox:-moz-locale-dir(rtl) {
>+  float: left;
>+}
>+
>+#header-view-toolbox:-moz-locale-dir(ltr) {
>+  float: right;
>+}

I would rather the selectors were in the opposite order, and we removed the -moz-locale-dir(ltr), like this:
#header-view-toolbox {
  float: right;
}

#header-view-toolbox:-moz-locale-dir(rtl) {
  float: left;
}

But that's pretty minor, so I'll say r=me with that changed and tested.

Thanks,
Blake.

Comment 13

[Mark Banner (:standard8)]

Andreas, can you update the patch sometime soon please?

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: updated patch

Updated patch to reflect Blake's review. Ready for checkin!

Comment 15

[rsx11m]

Is this the correct patch? It looks more like a Migration Assistant RTL fix...

Comment 16

[Mark Banner (:standard8)]

Yes, this seems to need a new patch...

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: Andreas' patch with my suggested fixes. (backed out)

I think this is basically the patch Andreas wanted to post.
p=andreasn, r=bwinton, ui-r=ehsan.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 485725 [details] [diff] [review]
Andreas' patch with my suggested fixes. (backed out)

Cause it's small, and it's a 3.1.6 blocker.

(We might want to let this bake on comm-central for a while first.  Or maybe not.  Your call.  :)

Thanks,
Blake.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Checked in as http://hg.mozilla.org/comm-central/rev/65d2e55e4313

Comment 20

[rsx11m]

Comment on attachment 485725 [details] [diff] [review]
Andreas' patch with my suggested fixes. (backed out)

This brakes qute in recent tinderbox builds, the toolbar is always flushed to the left in Windows even in en-US locale.

>-                   style="float: right;">

That appears to be the culprit: removed in msgHdrViewOverlay.xul but then you've patched only gnomestripe and not the other default themes, right?

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Ooh, good catch, rsx11m!

Backed out as http://hg.mozilla.org/comm-central/rev/868231407a2b

And on that note, I think I'm going to let Andreas post his patch instead.  (One mess up per commit is enough for me.  ;)

Thanks,
Blake.

Comment 22

[Andreas Nilsson (:andreasn)]

Attachment: new patch

Lets hope that this is the right patch this time :)
This addresses the issue on Windows and OS X too.

Comment 23

[Andreas Nilsson (:andreasn)]

It seems it works as it should on Windows. My OSX build is busted right now, but I expect the same thing to happen there.

Blake wondered why the patch carried the following snippet:
-.hdrArchiveButton {
+.hdrArchiveButton {
   list-style-image: url("chrome://messenger/skin/icons/mail-toolbar.png");
   -moz-image-region: rect(0px 480px 24px 456px);
}

Apparently it's because the file used to have dos line endings, but now it has unix line endings, and we prefer that.

He also said it got an r+, so checkin-needed.

Comment 24

[Andreas Nilsson (:andreasn)]

Comment on attachment 491824 [details] [diff] [review]
new patch

This carries over the earlier review +'es.

Comment 25

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/191d148a57e0

Please can someone check this in tomorrow's builds.

Comment 26

[rsx11m]

Looks ok to me in today's Windows builds for LTR locale, thus no regression
this time. I don't have any RTL builds though to test the issue to be fixed.

Comment 27

[Simon Montagu :smontagu]

You can simulate an RTL build by setting intl.uidirection.en-US (or any other locale) to "rtl" in about:config

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 491824 [details] [diff] [review]
new patch

a=Standard8 for 3.1.7.

Comment 29

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/ccbd827a5141

Comment 30

[rsx11m]

Verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20101126 Lanikai/3.1.7pre with pref-based rtl per comment #27.

Comment 31

[rsx11m]

Is there any standardized way to inform theme developers that this change is effective with 3.1.7 and that they need to do something? See bug 617306.

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #31)
> Is there any standardized way to inform theme developers that this change is
> effective with 3.1.7 and that they need to do something? See bug 617306.

Hang on, this affected themes? I didn't realise that when I approved it - we don't normally take add-on compatibility affecting changes on stable branches. I'll re-open that bug and discuss with drivers later.

Comment 33

[Mark Banner (:standard8)]

Backed out from the 3.1.7 builds and branches due to the regressions for add-ons (the security & stability branches shouldn't affect add-ons).

http://hg.mozilla.org/releases/comm-1.9.2/rev/0f12f5133c67
http://hg.mozilla.org/releases/comm-1.9.2/rev/f85a2c5971a3

At the moment this will mean that the sender header will remain incorrectly aligned on RTL builds of 3.1.x, unless we can find a solution that doesn't affect themes.

The next major version of Thunderbird will pick up this fix regardless.

Comment 34

[netanel_h]

As this issue won't be fixed for 3.1.x branch, can it be fixed manually and temporary by UserChrome.css or by addon? Should this issue be added to the release notes?