588735 - rtl text in UI displayed as mirror-image

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Avi L]

User-Agent:       Opera/9.80 (Windows NT 5.1; U; en) Presto/2.6.30 Version/10.61
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

firefox 4.0 beta 4 does not display rtl interface correctly, the text and graphics are reversed in several places in the interface, mainly the tabs, main menu & toolbar, which causes the menus and awesomebar to align incorrectly but display correctly (i.e: not affected by the bug).

this is true for the two builds currently available in the trunk (beta4 candidates build 2+3)

Reproducible: Always

Steps to Reproduce:
1. install firefox 4 beta 4 in hebrew
2. run firefox
3.



i've changed the language pack to fit all the betas, while beta 1 & latest trunk were unable to show hebrew (various xul issues), beta 3 also suffers from this bug.

Comment 1

[Avi L]

Attachment: firefox 4.0beta4 ui in hebrew

Comment 2

[Jo Hermans]

bug 587247 is one of them

Comment 3

[Simon Montagu :smontagu]

Does this affect other rtl localizations? This is a very weird and serious bug: the text is not just displayed in reverse order but with mirror-images of the glyphs.

Comment 4

[Avi L]

as far as i know, hebrew is currently the only RTL language in the ff4 beta 4 trunk.

i think the problem is only in windows, i just tested it on latest ubuntu and it works fine.
it might be related to user32.dll's SetProcessDefaultLayout() function:
http://msdn.microsoft.com/en-us/library/ms633542(VS.85).aspx

Comment 5

[Simon Montagu :smontagu]

Confirming with Gecko/20100818 Minefield/4.0b5pre.

This affects the Hebrew and Persian localizations on Windows. If it's too late to fix, we need to pull them from beta 4 -- we certainly can't release them in this state.

(In reply to comment #4)
> it might be related to user32.dll's SetProcessDefaultLayout() function:
> http://msdn.microsoft.com/en-us/library/ms633542(VS.85).aspx

As far as I know, SetProcessDefaultLayout shouldn't create this mirror-imaged text effect. I remember seeing something similar the first time I ever tried to write a RTL application, way back on Windows 3.1, by using SetMapMode(MM_ANISOTROPIC) with different signs for logical and device coordinates.

Comment 6

[(no longer active)]

Attachment: On Win7

This is a screenshot of what I see on beta4 candidate Hebrew builds on Windows 7.  I can't read Hebrew so I don't know if the text is being displayed correctly or not, but the control placements seem correct, although the display is horribly broken.

Simon, feel free to let me know if you need testing help on Windows 7.  I can test builds on XP, Vista and 7 at the office.

Comment 7

[(no longer active)]

FWIW, there is not going to be a Persian release of Firefox 4 beta 4.  I'm not sure about Arabic.

And I totally agree that we should pull off Hebrew b4 builds.

Comment 8

[Simon Montagu :smontagu]

You don't need to read Hebrew to recognize the bug, since English text is affected too -- note the tab title, address bar, and search bar in attachment 467379 [details].

So Windows 7 is not affected, but XP is.

The problem seems to go away if I set intl.uidirection.he (or .fa) to "ltr", though some other parts of the UI are broken if I do that.

Comment 9

[Anas Husseini [:linostar]]

Arabic is not opting in beta4 too. I guess RTL-locale versions can't be released until this is fixed.

Comment 10

[Simon Montagu :smontagu]

Still on XP, I can reproduce the bug on an en-US trunk build by setting intl.uidirection.en-US to rtl and restarting. I can't work out the principle by which some elements are mirrored and some aren't.

Comment 11

[Simon Montagu :smontagu]

I had problems finding a regression range. The build from 2010-07-16 (rev 96de199027d7) has the bug. In the previous nightly, rev 5fda39cd703c, there are black rectangles in all the places where the bug would be.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5fda39cd703c&tochange=96de199027d7

Bug 564991 presumably had some effect here, but it's impossible to tell whether it caused this bug or not

Comment 12

[Avi L]

win7 is affected in some way, since the toolbar is displayed incorrectly
and also the new firefox button is displayed with the menu bar simultaneously which shouldn't happen.

i've searched the code, couldn't find a call to SetProcessDefaultLayout but i did find a test in the beta3 trunk that uses "-moz-tranform: scaleX(-1);" on ui elements. there might be a similar change in xul that causes this bug.

btw, since 3.6 branch is unaffected, the regression range might be wider...

Comment 13

[Avi L]

Attachment: ff 4 alpha 5 rtl UI (unaffected)

Comment 14

[Avi L]

Attachment: ff 4 beta 1 rtl UI (affected, no display)

Comment 15

[Avi L]

Attachment: ff 4 beta 2 rtl UI (affected, mirrored)

Comment 16

[Avi L]

i've added several screenshots of different trunk versions.
my guess is the bug started after alpha 5, this is probably why beta 1 didn't display any controllers (the black bars) and the fix in beta 2 seems to cause the problem...

Comment 17

[Simon Montagu :smontagu]

Ehsan, if you have time to bisect within  the range in comment 11 to find which changeset is responsible, that would be great.

Comment 18

[Simon Montagu :smontagu]

BTW, I did a search for "-moz-tranform: scaleX(-1);" and didn't find anything that looked as if would affect the UI elements where we're seeing the bug

Comment 19

[Avi L]

Attachment: ff4b4 with intl.uidirection.he changed to ltr

Comment 20

[Avi L]

Simon, i found out that if you change intl.uidirection.he to ltr the ui works ok and right-to-left as it should be (attached image above).
there are still little quirks but they could be fixed easily by you.

i also found out that several css files in browser.jar have "-moz-transform: scaleX(-1)" in related ui elements like toolbar icons. removing the transform (without changing intl.uidirection.he) didn't help though which seems weird.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> i also found out that several css files in browser.jar have "-moz-transform:
> scaleX(-1)" in related ui elements like toolbar icons. removing the transform
> (without changing intl.uidirection.he) didn't help though which seems weird.

Not weird at all. The scaleX(-1) stuff in the theme isn't responsible for this bug.

Comment 22

[Avi L]

Dão, yes, but the toolbar icons still appear in weird order & direction (i removed the scaleX in attachment 467771 [details] when i tested this solution)

i've started working with the code, from what i saw, several images have rtl versions, including the toolbar icons so it makes no sense to reverse them (with scaleX transform) if they're already from a reversed source (the images)

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> Dão, yes, but the toolbar icons still appear in weird order & direction (i
> removed the scaleX in attachment 467771 [details] when i tested this solution)

It's not expected to look right if you remove stuff... Did you test without your modifications?

Comment 24

[(no longer active)]

I managed to reproduce this bug on Win7 as well under the classic theme.  I'll do a bisect to see if I can find the culprit here.

Comment 25

[Avi L]

(In reply to comment #23)
> It's not expected to look right if you remove stuff... Did you test without
> your modifications?

yes, same result, but i only tested on the hebrew version.

Comment 26

[(no longer active)]

(In reply to comment #11)
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5fda39cd703c&tochange=96de199027d7

So, at the beginning of this range, I was getting a window with black chrome areas.  I considered that to be the "good" state.  At the end of this range, I got a window with the chrome areas rendered with text being mirrored, etc.  I called that the "bad" state.  Bisecting based on that resulted in:

ehsan@BEHEMOTH ~/moz/src
$ hg bis -b
The first bad revision is:
changeset:   47738:c6ecff6b8a91
user:        Robert O'Callahan <[e-mail]>
date:        Fri Jul 16 09:07:51 2010 +1200
summary:     Bug 564991. Part 11: Start retaining layer trees. r=mats

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #26)
> (In reply to comment #11)
> > http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5fda39cd703c&tochange=96de199027d7
> 
> So, at the beginning of this range, I was getting a window with black chrome
> areas.  I considered that to be the "good" state. 
[...]
> summary:     Bug 564991. Part 11: Start retaining layer trees. r=mats

I'm not sure this makes sense. If we backed out that patch, theoretically, and got back the black chrome, that would be pretty pointless. Shouldn't we first identify what broke the UI originally?

Comment 28

[(no longer active)]

(In reply to comment #27)
> I'm not sure this makes sense. If we backed out that patch, theoretically, and
> got back the black chrome, that would be pretty pointless. Shouldn't we first
> identify what broke the UI originally?

We _should_, but with the UI being all black on revisions before this, I think this is as good of a bisect result that we can get.

Comment 29

[Dão Gottwald [:dao]]

When did the UI become black?

Comment 30

[(no longer active)]

(In reply to comment #29)
> When did the UI become black?

That needs its own bisect analysis...

Comment 31

[(no longer active)]

(In reply to comment #30)
> (In reply to comment #29)
> > When did the UI become black?
> 
> That needs its own bisect analysis...

Well, actually, someone could just download and test the nightlies to figure out a rough range for bisecting.  I'm afraid that I won't have enough time to do this myself today...

Comment 32

[Simon Montagu :smontagu]

From nightlies, the black rectangles began in the range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788, which again includes a number of possibilities.

The mirrored text might have been caused by any check from when the black rectangles began to when they were fixed, i.e. within http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=96de199027d7.

I have discovered that the mirrored text goes away if I remove the following lines from http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#563
563   if (aInitData->mRTL) {
564     extendedStyle |= WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT;
565   }

This is strange: that change was checked in back in bug 224547, so it didn't cause the regression, but it must somehow be interacting with some other change to cause it.

Comment 33

[(no longer active)]

(In reply to comment #32)
> From nightlies, the black rectangles began in the range
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788,
> which again includes a number of possibilities.

I'm bisecting this range.  Maybe if we can revert the changeset which causes the blackness we can bisect with more accuracy...

> This is strange: that change was checked in back in bug 224547, so it didn't
> cause the regression, but it must somehow be interacting with some other change
> to cause it.

I'm sort of suspecting that this has something to do with painting the title bar manually.  We'll see...

Comment 34

[(no longer active)]

(In reply to comment #33)
> (In reply to comment #32)
> > From nightlies, the black rectangles began in the range
> > http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788,
> > which again includes a number of possibilities.
> 
> I'm bisecting this range.  Maybe if we can revert the changeset which causes
> the blackness we can bisect with more accuracy...

Bisecting this range results in:

The first bad revision is:
changeset:   46194:a053227f5f96
user:        Jim Mathies <[e-mail]>
date:        Thu Jun 24 21:01:06 2010 -0500
summary:     Bug 513162 - Document viewer to widget glue. r=bz.

At this revision, the entire window is not painted at all, and the background windows and desktop show through it.  At the next revision, the window's chrome area is drawn black.

> > This is strange: that change was checked in back in bug 224547, so it didn't
> > cause the regression, but it must somehow be interacting with some other change
> > to cause it.
> 
> I'm sort of suspecting that this has something to do with painting the title
> bar manually.  We'll see...

This bisection confirms my suspicion.  :-)

Comment 35

[Jim Mathies [:jimm]]

WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT implies we mirror the top level widget, but none of the child widgets. Prior to bug 513162, content sat in a child widget within the top level chrome widget. After that landing the child is gone and content sits in the top level. With WS_EX_LAYOUTRTL it's mirrored for us by windows. Is there any way we can rely on windows mirroring rather than doing it ourselves?

Comment 36

[(no longer active)]

Attachment: Part 1: Don't use a mirrored DC

This patch causes us not to use a mirrored DC for any window.  It fixes the mirroring problem.  But there are at least two more major problems yet to be solved:

1. Hit testing is completely broken.  I think hit testing returns the mirrored X coordinate.  This makes actually interacting with the UI sort of fun (s/fun/horrible/).

2. Some of the images drawn on the screen look corrupted.  Examples include the tab candy image on the main browser window, or the sync image in the Options window.

Asking for Simon's feedback before asking Bas to review.

Comment 37

[Jim Mathies [:jimm]]

I believe we still have inner children for popups and dialogs, so we would either have to remove the inner child in those cases (not hard to do via code in nsDocumentViewer, but untested) or special case this to top level windows. I would expect with this applied that dialogs and popups would be reversed.

Comment 38

[(no longer active)]

(In reply to comment #37)
> I believe we still have inner children for popups and dialogs, so we would
> either have to remove the inner child in those cases (not hard to do via code
> in nsDocumentViewer, but untested) or special case this to top level windows. I
> would expect with this applied that dialogs and popups would be reversed.

Hmm, now that you mentioned it, I don't get any popups at all (like the location bar popup, Inspect popups, etc) but the dialogs look fine.

Also, testing this patch on an LTR profile (d'oh!), I can still see the image corruptions.

Comment 39

[Simon Montagu :smontagu]

From http://msdn.microsoft.com/en-us/library/ms632599%28v=VS.85%29.aspx#layout_mirroring, the hit testing issue should be solvable by using MapWindowPoints instead of ScreenToClient (I guess in nsWindow::InitEvent)

Comment 40

[Simon Montagu :smontagu]

(In reply to comment #38)
> Hmm, now that you mentioned it, I don't get any popups at all (like the
> location bar popup, Inspect popups, etc) but the dialogs look fine.

When testing before I was seeing the mirroring in the downloads manager and in some popups from the Options dialog, e.g. Exceptions under Security.

Comment 41

[(no longer active)]

Good news!  The image corruption seemed to be a problem in my build.  A clobber build fixed the issue.

(In reply to comment #40)
> (In reply to comment #38)
> > Hmm, now that you mentioned it, I don't get any popups at all (like the
> > location bar popup, Inspect popups, etc) but the dialogs look fine.
> 
> When testing before I was seeing the mirroring in the downloads manager and in
> some popups from the Options dialog, e.g. Exceptions under Security.

Both those windows look fine now (sans hit testing).  I'm going to work on the hit testing issue now.

Comment 42

[(no longer active)]

Attachment: Part 2: Hit testing WIP

This patch basically replaces all of our ScreenToClient uses with MapWindowPoints.  But unfortunately it doesn't seem to have any kind of effect...

Comment 43

[Avi L]

Attachment: 4.0b5pre mirrored ui (affected inc. content)

Comment 44

[Avi L]

ehsan, that's because the ui is ltr oriented or mixed (ltr+rtl), afaik it works when using SetProcessDefaultLayout() on the main window

simon, i've checked the latest trunk (4.0b5pre) and now it's a total mess, if previous versions only reversed the main ui, now content is mirrored as well (see attachment 470321 [details] )

i've also checked ff 3/3.5/3.6/4 branches on winxp and 3.6/4 on ubuntu 10.04, and if you change intl.uidirection.he to ltr, the ui is completely fine and in rtl display, it just needs a fix to the back/fwd buttons.
i think removing reference and code related to intl.uidirection.* might solve the problem.

Comment 45

[Simon Montagu :smontagu]

Attachment: Workaround: go back to inner child widgets for RTL

Thoughts?

Comment 46

[Jim Mathies [:jimm]]

Comment on attachment 470323 [details] [diff] [review]
Workaround: go back to inner child widgets for RTL

This breaks the titlebar, you can't drag the window or interact with the windows caption buttons on glass desktops because the child window overlaps everything. We removed this sub window for these reasons.

What was the problem with flipping the view using SetProcessDefaultLayout? If we set that and the main view works ok, all we have to do then is remove the sub window for dialogs and popups by adding  

if ((winType == eWindowType_toplevel ||
     winType == eWindowType_dialog ||
     winType == eWindowType_popup) &&
..

Which I think should enable this for every top level window we create. This probably should have been in the original patch anyway. I was just being cautious.

Comment 47

[Jim Mathies [:jimm]]

* SetProcessDefaultLayout -> SetLayout

Comment 48

[Jim Mathies [:jimm]]

Hmm, I was trying to test this using ForceRTL in yesterdays nightly. In the first window when I select the menu option, everything reverses except the caption buttons. (That's probably expected since we don't reset the window styles on that window.) But on opening up a second window, I get the caption buttons in the right place, but a completely blank white window. Anybody else seeing that? This worked a week ago, maybe bug 130078 regressed something?

Comment 49

[Jim Mathies [:jimm]]

Attachment: remove sub windows

Here's a patch that removes the remaining sub windows in popups and dialogs.

Comment 50

[Dão Gottwald [:dao]]

Can we get some basic tests for rtl chrome and content in rtl chrome? It seems like none of the multiple stacked regressions we've seen here should have been allowed to happen in the first place.

Comment 51

[(no longer active)]

(In reply to comment #50)
> Can we get some basic tests for rtl chrome and content in rtl chrome? It seems
> like none of the multiple stacked regressions we've seen here should have been
> allowed to happen in the first place.

Yes, but it's very tricky.  I'm not sure how to test this.  The XUL DOM is fine, it's just the rendering on screen that is broken.  Do you have any idea how we could effectively test this?

Comment 52

[Dão Gottwald [:dao]]

(In reply to comment #51)
> (In reply to comment #50)
> > Can we get some basic tests for rtl chrome and content in rtl chrome? It seems
> > like none of the multiple stacked regressions we've seen here should have been
> > allowed to happen in the first place.
> 
> Yes, but it's very tricky.  I'm not sure how to test this.  The XUL DOM is
> fine, it's just the rendering on screen that is broken.  Do you have any idea
> how we could effectively test this?

drawWindow would be able to catch at least some regressions, wouldn't it?

Comment 53

[(no longer active)]

(In reply to comment #52)
> drawWindow would be able to catch at least some regressions, wouldn't it?

The problem is what to compare the result of drawWindow against...

Comment 54

[Simon Montagu :smontagu]

Attachment: another hit testing w-i-p

Avi is right and mirroring coordinates doesn't help. We are getting mirrored coordinates from the system and we need to un-mirror them. It also turns out that the documentation lies (or is out-of-date) and ScreenToClient does mirror coordinates in RTL windows.

This patch goes a long way towards fixing the hit-testing issues, but there are still a lot of problems. Child windows inside the content window (e.g. YouTube videos) are positioned wrongly, and I am seeing all sorts of problems with invalidation. For example, when selecting text, the highlight sometimes doesn't appear and sometimes appears in the wrong place.

I am beginning to think that the only thing we can do at this stage is back out bug 224547, unless we can find a way to make Windows mirror the title bar without mirroring the rest of the window layout. The title bar mirroring itself is not such a big deal, but it will be a shame to lose the fix to common dialogs mentioned in bug 224547 comment 13. However, this is not a regression since the last release, so it is probably the least evil thing that we can do for Firefox 4.

Comment 55

[Dão Gottwald [:dao]]

(In reply to comment #53)
> (In reply to comment #52)
> > drawWindow would be able to catch at least some regressions, wouldn't it?
> 
> The problem is what to compare the result of drawWindow against...

With the result of some other chrome window. For instance, the rtl window <window><label value="foo"/></window> should be different from the ltr window <window style="-moz-transform:scaleX(-1)"><label value="foo"/></window>.

Comment 56

[Jim Mathies [:jimm]]

(In reply to comment #54)
> Created attachment 470787 [details] [diff] [review]
> another hit testing w-i-p
> 
> I am beginning to think that the only thing we can do at this stage is back out
> bug 224547, unless we can find a way to make Windows mirror the title bar
> without mirroring the rest of the window layout. The title bar mirroring itself
> is not such a big deal, but it will be a shame to lose the fix to common
> dialogs mentioned in bug 224547 comment 13. However, this is not a regression
> since the last release, so it is probably the least evil thing that we can do
> for Firefox 4.

The original problem in bug 224547 is becoming less of an issue. On composited desktops, we rely on windows to render caption buttons. In all other cases though, we now render the titlebar ourselves.

For glass, we could consider removing the caption buttons on rtl systems and rendering them ourselves in content. The code to do that is already present for non-glass desktops, we just need some graphics and a little bit of code in widget.

Maybe the dialog problem in bug 224547 can be fixed some other way? For example we might be able to use a surrogate parent with the proper style set. Not sure how invasive that would need to be.

If we do back out bug 224547, and decide to continue to use windows caption buttons for glass, we would also need to reverse the fx button, since it is drawn in content and would be mirrored.

Comment 57

[(no longer active)]

Comment on attachment 470787 [details] [diff] [review]
another hit testing w-i-p

I think this patch itself is fine, but I'm also starting to think about backing bug 224547 out.  As far as I can tell, there is no way to ask windows to only draw the title bar in reverse direction, since the title bar is really just the non-client area of the window.

Is it possible to use a hidden child window as the parent of the file open/save dialogs, and set the RTL flag on that Window?  (I don't have a Hebrew version of Windows so I can't test it myself).

Comment 58

[(no longer active)]

(In reply to comment #56)
> The original problem in bug 224547 is becoming less of an issue. On composited
> desktops, we rely on windows to render caption buttons. In all other cases
> though, we now render the titlebar ourselves.

Simon is talking about Windows such as the Save File dialog which are created by the OS.

> For glass, we could consider removing the caption buttons on rtl systems and
> rendering them ourselves in content. The code to do that is already present for
> non-glass desktops, we just need some graphics and a little bit of code in
> widget.

Is there a downside to this?  (I think that there should be, given the fact that we're not doing this for LTR windows already).

> Maybe the dialog problem in bug 224547 can be fixed some other way? For example
> we might be able to use a surrogate parent with the proper style set. Not sure
> how invasive that would need to be.

It would be great if Simon can test this.

> If we do back out bug 224547, and decide to continue to use windows caption
> buttons for glass, we would also need to reverse the fx button, since it is
> drawn in content and would be mirrored.

By reversing the Firefox button, you mean displaying it on the left side?

Comment 59

[(no longer active)]

(In reply to comment #55)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > drawWindow would be able to catch at least some regressions, wouldn't it?
> > 
> > The problem is what to compare the result of drawWindow against...
> 
> With the result of some other chrome window. For instance, the rtl window
> <window><label value="foo"/></window> should be different from the ltr window
> <window style="-moz-transform:scaleX(-1)"><label value="foo"/></window>.

Such a test wouldn't have caught this regression, because the comparison result would be not equal whether or not there was a regression.

Comment 60

[Dão Gottwald [:dao]]

(In reply to comment #59)
> > With the result of some other chrome window. For instance, the rtl window
> > <window><label value="foo"/></window> should be different from the ltr window
> > <window style="-moz-transform:scaleX(-1)"><label value="foo"/></window>.
> 
> Such a test wouldn't have caught this regression, because the comparison result
> would be not equal whether or not there was a regression.

Can you be more specific? There was a regression causing the text to be mirrored, wasn't there? Is this different from what scaleX(-1) would do?

As another example, I'd expect these windows to produce equal results:

[rtl] <window><label value="foo"/></window> 
[ltr] <window orient="horizontal"><spacer flex="1"/><label value="foo"/></window>

Comment 61

[(no longer active)]

(In reply to comment #60)
> (In reply to comment #59)
> > > With the result of some other chrome window. For instance, the rtl window
> > > <window><label value="foo"/></window> should be different from the ltr window
> > > <window style="-moz-transform:scaleX(-1)"><label value="foo"/></window>.
> > 
> > Such a test wouldn't have caught this regression, because the comparison result
> > would be not equal whether or not there was a regression.
> 
> Can you be more specific? There was a regression causing the text to be
> mirrored, wasn't there? Is this different from what scaleX(-1) would do?
> 
> As another example, I'd expect these windows to produce equal results:
> 
> [rtl] <window><label value="foo"/></window> 
> [ltr] <window orient="horizontal"><spacer flex="1"/><label
> value="foo"/></window>

In theory yes, but this regression causes some stuff being drawn in the correct coordinates, and other things being drawn in the reverse coordinate.  I don't really have a clear idea on why some things are drawn correctly but others are not, but the whole thing is a total mess in this specific case.

Comment 62

[Jim Mathies [:jimm]]

(In reply to comment #58)
> (In reply to comment #56)
> > The original problem in bug 224547 is becoming less of an issue. On composited
> > desktops, we rely on windows to render caption buttons. In all other cases
> > though, we now render the titlebar ourselves.
> 
> Simon is talking about Windows such as the Save File dialog which are created
> by the OS.

The creation of those should be pretty well isolated somewhere in the code base. nsILocalFileWin maybe or something else in xpcom/io? The number of uses cases is probably pretty small as well.
 
> 
> > For glass, we could consider removing the caption buttons on rtl systems and
> > rendering them ourselves in content. The code to do that is already present for
> > non-glass desktops, we just need some graphics and a little bit of code in
> > widget.
> 
> Is there a downside to this?  (I think that there should be, given the fact
> that we're not doing this for LTR windows already).

non-default custom drawn caption buttons on ltr systems. (Similar to the way chrom does there,s although our would look much better because our ui folks have skills. ;) We actually considered implementing this for personas but punted.

I'd think if we had to choose between our own buttons and default buttons in the wrong position, custom buttons would win. 

> 
> > Maybe the dialog problem in bug 224547 can be fixed some other way? For example
> > we might be able to use a surrogate parent with the proper style set. Not sure
> > how invasive that would need to be.
> 
> It would be great if Simon can test this.
> 
> > If we do back out bug 224547, and decide to continue to use windows caption
> > buttons for glass, we would also need to reverse the fx button, since it is
> > drawn in content and would be mirrored.
> 
> By reversing the Firefox button, you mean displaying it on the left side?

Yes. This presents more problems which tend to make the custom button solution more appealing. We can turn these on with just a little css tweaking:

displaying the button-box -

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser-aero.css#19

and skin the buttons -

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#315

currently theme code doesn't paint anything for these when glass is on, so I think we'd need to nix the current moz styles and use our own styling.
 
The only thing left to do is nix the default buttons in widget when the compositor is on and the window is rtl. We already have that info in widget thanks to bug 224547 which we'll be caching pretty soon via the patch in bug 574859. Removing the buttons should be possible by tweak the window styles we pass when we create the window.

Comment 63

[Simon Montagu :smontagu]

(In reply to comment #56)

> Maybe the dialog problem in bug 224547 can be fixed some other way? For example
> we might be able to use a surrogate parent with the proper style set. Not sure
> how invasive that would need to be.

(In reply to comment #57)
> Is it possible to use a hidden child window as the parent of the file open/save
> dialogs, and set the RTL flag on that Window?  (I don't have a Hebrew version
> of Windows so I can't test it myself).

Good call, both of you :) That seems to work very well. I'll attach a patch after some more testing.

What I'm thinking of doing is just changing the extended style in RTL windows from WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT to WS_EX_RTLREADING. Will that be  OK as a flag to identify RTL windows, Jim?

Comment 64

[(no longer active)]

(In reply to comment #63)
> What I'm thinking of doing is just changing the extended style in RTL windows
> from WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT to WS_EX_RTLREADING. Will that be 
> OK as a flag to identify RTL windows, Jim?

Why do we need to set a window style?  Can't we just store the state in our own code?  I'm wary of using window styles which might come back and bite us in the future (like the WS_EX_LAYOUTRTL style did!).

Comment 65

[Jim Mathies [:jimm]]

(In reply to comment #63)
> (In reply to comment #56)
> 
> Good call, both of you :) That seems to work very well. I'll attach a patch
> after some more testing.
> 
> What I'm thinking of doing is just changing the extended style in RTL windows
> from WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT to WS_EX_RTLREADING. Will that be 
> OK as a flag to identify RTL windows, Jim?


Not sure I understand the question. Is the identification issue for us or 3rd parties?

I'd like to keep the two patches in bug 224547, and just change

>+  if (aInitData->mRTL) {
>+    extendedStyle |= WS_EX_LAYOUTRTL | WS_EX_NOINHERITLAYOUT;
>+  }
>+

Comment 66

[Simon Montagu :smontagu]

Attachment: Use member instead of window style [landed in comment 73]

Comment 67

[Simon Montagu :smontagu]

Attachment: Use temporary window as parent of common dialogs

This is a bit inelegant. At first I tried creating the temporary window only for RTL parents and caching it, but that had the problem that the main window didn't regain focus after closing the dialog, also that a second dialog window wasn't positioned correctly if the main window had been resized or moved in the meanwhile.

Comment 68

[Avi L]

simon, why do you bother finding a fix when removing the "intl.uidirection.*" prefs fixes the issue?
i've compiled 4.0b3 with this pref removed (in nsChromeRegistryChrome.cpp,  nsChromeRegistryChrome::IsLocaleRTL() always returns false) and this fixed the bug (hebrew is working correctly and rtl) and you just need to adjust the fwd/back buttons.
removing "intl.uidirection.*" works for every beta i've tested, including latest trunk. it's a redundant pref anyway (at least on windows and ubuntu) like i wrote in comment 44

Comment 69

[Jim Mathies [:jimm]]

Attachment: mirror caption buttons on glass desktops for mIsRTL windows

We can flip the glass caption buttons using a dwm call:

http://msdn.microsoft.com/en-us/library/aa969530(v=VS.85).aspx

This flips the buttons but doesn't trigger mirroring of the content on my english system.

I think between the dialog fixes, this, and the new titlebar rendering we have this pretty much fixes all the issues. Am I missing anything?

Comment 70

[Jim Mathies [:jimm]]

(In reply to comment #67)
> Created attachment 471552 [details] [diff] [review]
> Use temporary window as parent of common dialogs
> 
> This is a bit inelegant. At first I tried creating the temporary window only
> for RTL parents and caching it, but that had the problem that the main window
> didn't regain focus after closing the dialog, also that a second dialog window
> wasn't positioned correctly if the main window had been resized or moved in the
> meanwhile.

Seems reasonable to me. We don't create these often and we're already dealing with the overhead of creating the dialog. This is a minor additional hit.

Comment 71

[Jim Mathies [:jimm]]

Simon, can we put all these patches together and fire them off to try for some test builds?

Comment 72

[Jim Mathies [:jimm]]

(In reply to comment #71)
> Simon, can we put all these patches together and fire them off to try for some
> test builds?

hmm, I guess try doesn't build localized builds. maybe not.

Comment 73

[(no longer active)]

Comment on attachment 471545 [details] [diff] [review]
Use member instead of window style [landed in comment 73]

http://hg.mozilla.org/mozilla-central/rev/1cf41b2181da

Comment 74

[Simon Montagu :smontagu]

I pushed my last two patches to try already, and Tomer said he could create a Hebrew language pack which people could test with.

Comment 75

[Simon Montagu :smontagu]

I made another try push including attachment 471577 [details] [diff] [review], and remembered to rebase this time. Builds should appear in http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-aa37c042f1d3/ in due course.

Comment 76

[(no longer active)]

(In reply to comment #74)
> I pushed my last two patches to try already, and Tomer said he could create a
> Hebrew language pack which people could test with.

That's not really necessary.  You could just set intl.uidirection.en to "rtl" in a normal build to test this (you'll need to open a new window, apparently).

Comment 77

[(no longer active)]

(In reply to comment #68)
> simon, why do you bother finding a fix when removing the "intl.uidirection.*"
> prefs fixes the issue?
> i've compiled 4.0b3 with this pref removed (in nsChromeRegistryChrome.cpp, 
> nsChromeRegistryChrome::IsLocaleRTL() always returns false) and this fixed the
> bug (hebrew is working correctly and rtl) and you just need to adjust the
> fwd/back buttons.
> removing "intl.uidirection.*" works for every beta i've tested, including
> latest trunk. it's a redundant pref anyway (at least on windows and ubuntu)
> like i wrote in comment 44

Avi, the uidirection prefs are required for our RTL locales to work.  We default it to rtl for he, ar and fa.  Removing them would mean breaking RTL support in our UI on all platforms.

Comment 78

[(no longer active)]

Comment on attachment 471552 [details] [diff] [review]
Use temporary window as parent of common dialogs

Simon, have you tested this on non-libxul builds as well?  I'm asking because I expect nsToolkit::mDllInstance to be null in those builds...  But other than that, it seems fine to me.

Comment 79

[(no longer active)]

Comment on attachment 471577 [details] [diff] [review]
mirror caption buttons on glass desktops for mIsRTL windows

This is amazing, Jim!

Comment 80

[Simon Montagu :smontagu]

(In reply to comment #76)
> (In reply to comment #74)
> > I pushed my last two patches to try already, and Tomer said he could create a
> > Hebrew language pack which people could test with.
> 
> That's not really necessary.  You could just set intl.uidirection.en to "rtl"
> in a normal build to test this (you'll need to open a new window, apparently).

That's the way I've been testing, but I'd like to get some test coverage for the actual localized version also, as well as from people not running localized Windows.
(In reply to comment #78)
> Comment on attachment 471552 [details] [diff] [review]
> Use temporary window as parent of common dialogs
> 
> Simon, have you tested this on non-libxul builds as well?  I'm asking because I
> expect nsToolkit::mDllInstance to be null in those builds...  But other than
> that, it seems fine to me.

I've been testing on libxul and non-libxul, but only on Hebrew Windows XP.

Comment 81

[Jim Mathies [:jimm]]

(In reply to comment #79)
> Comment on attachment 471577 [details] [diff] [review]
> mirror caption buttons on glass desktops for mIsRTL windows
> 
> This is amazing, Jim!

Ack, 

nsUXThemeData::::CheckForCompositor()

should be 

nsUXThemeData::CheckForCompositor()

in that if statement. hope I didn't wreck a try run with that.

Comment 82

[Jim Mathies [:jimm]]

Attachment: mirror caption buttons on glass desktops for mIsRTL windows

updated

Comment 83

[Tomer Cohen :tomer]

Attachment: Screenshot of Firefox 4b5pre using smontagu's try-server build

Seems that the try-server builds produced by smontagu gives better results than the nightly builds.

Steps to reproduce
a. Download the en-us try-server build
b. Install the latest langpack XPI from l10n-mozilla-central
c. Start the try-server build using the following parameters -
    firefox.exe -UILocale he -contentLocale he

Comment 84

[Avi L]

Attachment: ff 4.0b5pre (fixed)

seems like the bug is fixed in this tryserver build
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d8a2a1b2227a/tryserver-win32/

Comment 85

[Avi L]

Attachment: ff 4.0b5pre (fixed, w/ new ff button)

notice the title bar is changing direction when using the new ff button (on xp sp3 in hebrew at least)


tomer, try with classic theme as well, since it was reported that the bug affects win7 in classic theme.

Comment 86

[Avi L]

> (In reply to comment #68)
> Avi, the uidirection prefs are required for our RTL locales to work.  We
> default it to rtl for he, ar and fa.  Removing them would mean breaking RTL
> support in our UI on all platforms.
i understand that, but i tested thunderbird as well (other products like seamonkey and sunbird do not use this pref) and it seems the pref is mostly ignored, i.e changing it to ltr for hebrew keeps the ui in rtl display with only minor display issues (for example in ff: back/fwd buttons; and in tb: tab drawing) that can be fixed using css in the chrome code.

Comment 87

[Simon Montagu :smontagu]

(In reply to comment #81)
> hope I didn't wreck a try run with that.

Yeah, just a bit ;-) New builds with the corrected should be appearing soon at ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c4784d2567bf/

Comment 88

[Simon Montagu :smontagu]

(In reply to comment #86)
> > (In reply to comment #68)
> > Avi, the uidirection prefs are required for our RTL locales to work.  We
> > default it to rtl for he, ar and fa.  Removing them would mean breaking RTL
> > support in our UI on all platforms.
> i understand that, but i tested thunderbird as well (other products like
> seamonkey and sunbird do not use this pref) and it seems the pref is mostly
> ignored, i.e changing it to ltr for hebrew keeps the ui in rtl display with
> only minor display issues (for example in ff: back/fwd buttons; and in tb: tab
> drawing) that can be fixed using css in the chrome code.

Yes, but the uidirection pref is what we use to determine the value of the -moz-locale-dir selector which chrome css uses for directionally-determined formatting.

Comment 89

[(no longer active)]

(In reply to comment #88)
> (In reply to comment #86)
> > > (In reply to comment #68)
> > > Avi, the uidirection prefs are required for our RTL locales to work.  We
> > > default it to rtl for he, ar and fa.  Removing them would mean breaking RTL
> > > support in our UI on all platforms.
> > i understand that, but i tested thunderbird as well (other products like
> > seamonkey and sunbird do not use this pref) and it seems the pref is mostly
> > ignored, i.e changing it to ltr for hebrew keeps the ui in rtl display with
> > only minor display issues (for example in ff: back/fwd buttons; and in tb: tab
> > drawing) that can be fixed using css in the chrome code.
> 
> Yes, but the uidirection pref is what we use to determine the value of the
> -moz-locale-dir selector which chrome css uses for directionally-determined
> formatting.

In other words, if an application does not support RTL mode in its theme (which, AFAIK, Thunderbird doesn't), that's a separate issue.  The uidirection pref is tied to the platform level support for RTL theming of the application's chrome.

Comment 90

[(no longer active)]

(In reply to comment #83)
> Created attachment 471765 [details]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=471765
> Screenshot of Firefox 4b5pre using smontagu's try-server build
> 
> Seems that the try-server builds produced by smontagu gives better results than
> the nightly builds.
> 
> Steps to reproduce
> a. Download the en-us try-server build
> b. Install the latest langpack XPI from l10n-mozilla-central
> c. Start the try-server build using the following parameters -
>     firefox.exe -UILocale he -contentLocale he

Tomer, could you please test this build (and Jim's as well) in Aero Glass mode as well?

Comment 91

[Tomer Cohen :tomer]

(In reply to comment #90)
> Tomer, could you please test this build (and Jim's as well) in Aero Glass mode
> as well?

Sure. I'll get access to a physical Win7 machine with localized UI this evening to test it on, and will look if the same build looks any better. 

By the way, this bug doesn't reproduce on Wine. :)

Comment 92

[Tomer Cohen :tomer]

Attachment: Windows7 with Aero enabled. Please note it has a broken Firefox button!

On Windows 7 environment with localized right-to-left UI (Hebrew in this case), there is no mirrored UI, but the Firefox button is sitting just behind the window controls.

Comment 93

[Jim Mathies [:jimm]]

(In reply to comment #87)
> (In reply to comment #81)
> > hope I didn't wreck a try run with that.
> 
> Yeah, just a bit ;-) New builds with the corrected should be appearing soon at
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c4784d2567bf/

Try server barfed or something, we only got 2003 opt builds. Looks like these are here if the cset is right:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/hg-c4784d2567bf/

Comment 94

[Jim Mathies [:jimm]]

(In reply to comment #92)
> Created attachment 471984 [details]
> Windows7 with Aero enabled. Please note it has a broken Firefox button!
> 
> On Windows 7 environment with localized right-to-left UI (Hebrew in this case),
> there is no mirrored UI, but the Firefox button is sitting just behind the
> window controls.

Tomer, mind taking those for a spin on aero glass?

Comment 95

[Jim Mathies [:jimm]]

(In reply to comment #83)
> Created attachment 471765 [details]
> Screenshot of Firefox 4b5pre using smontagu's try-server build
> 
> Seems that the try-server builds produced by smontagu gives better results than
> the nightly builds.
> 
> Steps to reproduce
> a. Download the en-us try-server build
> b. Install the latest langpack XPI from l10n-mozilla-central
> c. Start the try-server build using the following parameters -
>     firefox.exe -UILocale he -contentLocale he

How do you avoid the "lang pack is not compatible with 4.0b6pre" error? I'd like to check a few things with this with personas but can't seem to get the lang pack to install on try builds.

Comment 96

[Simon Montagu :smontagu]

The langpacks in http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010-09-01-03-mozilla-central-l10n/ and later are 4.0b6-compatible. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/ seems to be out of date for some reason.

Comment 97

[Axel Hecht [:Pike]]

Be sure to scroll down to the b6 builds? The old ones are all b5 ones. The hebrew linux one is busted in upload today, though. Windows haven't yet started.

Comment 98

[(no longer active)]

(In reply to comment #92)
> Created attachment 471984 [details]
> Windows7 with Aero enabled. Please note it has a broken Firefox button!
> 
> On Windows 7 environment with localized right-to-left UI (Hebrew in this case),
> there is no mirrored UI, but the Firefox button is sitting just behind the
> window controls.

I think ths is because the try server build doesn't include attachment 471635 [details] [diff] [review].  Simon, can you confirm this please?

Comment 99

[Tomer Cohen :tomer]

Attachment: screenshot nightly-tryserver

I can confirm that the try-server build UI is better here with Windows 7, Localized version of Windows and 4b6pre Hebrew language pack. 

As for the Window caption, something here make the browser less responsive for few moments every now and than. When this happen, the window caption is reverted back to Windows default, making the orange Firefox button to disappear until the browser is responsive again.

Comment 100

[Simon Montagu :smontagu]

(In reply to comment #98)
> > On Windows 7 environment with localized right-to-left UI (Hebrew in this case),
> > there is no mirrored UI, but the Firefox button is sitting just behind the
> > window controls.
> 
> I think ths is because the try server build doesn't include attachment 471635 [details] [diff] [review]. 
> Simon, can you confirm this please?

Yes, that's right. The builds at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/hg-c4784d2567bf/ do include it

Comment 101

[Simon Montagu :smontagu]

Checked in attachment 471552 [details] [diff] [review] http://hg.mozilla.org/mozilla-central/rev/f692ed55fb8a

Jim, do you want to keep this open for attachment 471635 [details] [diff] [review] or to do that work in a follow-up bug?

Comment 102

[Jim Mathies [:jimm]]

(In reply to comment #101)
> Checked in attachment 471552 [details] [diff] [review]
> http://hg.mozilla.org/mozilla-central/rev/f692ed55fb8a
> 
> Jim, do you want to keep this open for attachment 471635 [details] [diff] [review] or to do that work in
> a follow-up bug?

lets just land it here. requesting review from roc. 

Roc, this is a smallish 4 line patch that flips the caption buttons on glass for rtl windows.

Comment 103

[Avi L]

Attachment: ff 4.0b6pre (fixed)

today's trunk in hebrew seems to be fixed

Comment 104

[Avi L]

Attachment: ff 4.0b6pre (fixed, w/ new ff button)

Comment 105

[Avi L]

added 2 new screenshots, the latest trunk 4.0b6pre from today seems to fix the problem.

note that the title bar behaviour is different between enabling the new ff button and disabling it (enabled behaviour is the right one, but minimize/maximize icons are reversed)


btw, is the "intl.uidirection.*" pref related to bug 224547 ? if so, it seems firefox ignores the pref when drawing the title bar...

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why bother checking nsUXThemeData::CheckForCompositor()? Is it harmful to set this attribute when DWM is not running? Setting it whether DWM is running or not would mean this works correctly if the DWM is started after the window has been created, I presume.

Comment 107

[Jim Mathies [:jimm]]

(In reply to comment #106)
> Why bother checking nsUXThemeData::CheckForCompositor()? Is it harmful to set
> this attribute when DWM is not running? Setting it whether DWM is running or
> not would mean this works correctly if the DWM is started after the window has
> been created, I presume.

Good point. There's no need for it.

Comment 108

[Jim Mathies [:jimm]]

(In reply to comment #106)
> Why bother checking nsUXThemeData::CheckForCompositor()? Is it harmful to set
> this attribute when DWM is not running? Setting it whether DWM is running or
> not would mean this works correctly if the DWM is started after the window has
> been created, I presume.

No it's not harmful, and yes, it caches the value while in aero basic. So if you flip to glass, the buttons are in the right place. Good call.

Comment 109

[Jim Mathies [:jimm]]

Attachment: dwm patch

Comment 110

[(no longer active)]

(In reply to comment #99)
> As for the Window caption, something here make the browser less responsive for
> few moments every now and than. When this happen, the window caption is
> reverted back to Windows default, making the orange Firefox button to disappear
> until the browser is responsive again.

That's windows stepping in and rendering the title bar itself.  I don't think there's much to be done about that on our side, I'm afraid.  And the same problem happens in LTR builds as well.

Comment 111

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/08155ca779b6

Comment 112

[Avi L]

Attachment: ff 4 beta 6 (affected again)

seems the bug is not fixed after all, this is the build1 of beta 6 (build 2 is currently not available for windows)

Comment 113

[Avi L]

Attachment: ff 4 beta 6 titlebar

notice in the previous attachment, the titlebar is mirrored as well.
when i changed "intl.uidirection.he" to ltr, firefox is ok, but sometimes (usually when playing with window size or min/maximizing the window) the original close/min/max buttons reappear like in this screenshot.

Comment 114

[Axel Hecht [:Pike]]

could you find a regression range?

Comment 115

[Tomer Cohen :tomer]

I suspect we've moved these checkins to beta7.

Comment 116

[Axel Hecht [:Pike]]

Right, please try a nightly? beta 6 is just a small tweak of beta 5, not a current snapshot.

Comment 117

[(no longer active)]

Beta 6 was released with only a couple of high priority fixes over Beta 5, and the Beta 6 build that we've been talking about has been renamed to Beta 7, which will ship with this bug fixed.

Comment 118

[Avi L]

sorry, didn't know that, though i didn't notice anything on the wiki...