Closed Bug 588735 Opened 10 years ago Closed 10 years ago

rtl text in UI displayed as mirror-image

Categories

(Core :: Internationalization, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: avi3k3, Assigned: smontagu)

References

Details

(Keywords: regression, rtl, Whiteboard: [4b4])

Attachments

(22 files, 4 obsolete files)

134.19 KB, image/png
Details
150.99 KB, image/jpeg
Details
76.80 KB, image/png
Details
121.39 KB, image/png
Details
133.87 KB, image/png
Details
134.91 KB, image/png
Details
165.23 KB, image/png
Details
1.02 KB, patch
jimm
: feedback-
Details | Diff | Splinter Review
991 bytes, patch
Details | Diff | Splinter Review
9.03 KB, patch
ehsan
: feedback+
Details | Diff | Splinter Review
1.56 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.97 KB, patch
jimm
: review+
ehsan
: feedback+
Details | Diff | Splinter Review
156.96 KB, image/png
Details
132.83 KB, image/png
Details
102.60 KB, image/png
Details
461.56 KB, image/png
Details
381.99 KB, image/png
Details
105.38 KB, image/png
Details
92.08 KB, image/png
Details
1.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
119.59 KB, image/png
Details
16.83 KB, image/png
Details
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.
bug 587247 is one of them
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.
blocking2.0: --- → ?
Keywords: rtl
Summary: rtl interface displays backwards → rtl text in UI displayed as mirror-image
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
blocking2.0: ? → betaN+
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image 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.
Blocks: 587247
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.
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.
Arabic is not opting in beta4 too. I guess RTL-locale versions can't be released until this is fixed.
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.
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
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...
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...
Ehsan, if you have time to bisect within  the range in comment 11 to find which changeset is responsible, that would be great.
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
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.
(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.
Whiteboard: [4b4]
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)
(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?
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.
Keywords: regression
(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.
(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 <robert@ocallahan.org>
date:        Fri Jul 16 09:07:51 2010 +1200
summary:     Bug 564991. Part 11: Start retaining layer trees. r=mats
(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?
(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.
When did the UI become black?
(In reply to comment #29)
> When did the UI become black?

That needs its own bisect analysis...
(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...
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.
(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...
(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 <jmathies@mozilla.com>
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.  :-)
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?
Attached patch Part 1: Don't use a mirrored DC (obsolete) — Splinter Review
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.
Attachment #468761 - Flags: review?
Attachment #468761 - Flags: feedback?(smontagu)
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.
(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.
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)
(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.
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.
Attached patch Part 2: Hit testing WIP (obsolete) — Splinter Review
This patch basically replaces all of our ScreenToClient uses with MapWindowPoints.  But unfortunately it doesn't seem to have any kind of effect...
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.
Thoughts?
Attachment #470323 - Flags: feedback?(jmathies)
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.
Attachment #470323 - Flags: feedback?(jmathies) → feedback-
* SetProcessDefaultLayout -> SetLayout
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?
Here's a patch that removes the remaining sub windows in popups and dialogs.
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.
Flags: in-testsuite?
(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?
(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?
(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...
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.
Attachment #470787 - Flags: feedback?(ehsan)
(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>.
(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 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).
Attachment #470787 - Flags: feedback?(ehsan) → feedback+
(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?
(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.
(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 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.
(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.
(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?
(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!).
(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;
>+  }
>+
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.
Attachment #471552 - Flags: review?(jmathies)
Attachment #471552 - Flags: feedback?(ehsan)
Attachment #471545 - Flags: review?(jmathies) → review+
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
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?
Attachment #471577 - Attachment is patch: true
(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.
Attachment #471552 - Flags: review?(jmathies) → review+
Simon, can we put all these patches together and fire them off to try for some test builds?
(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 on attachment 471545 [details] [diff] [review]
Use member instead of window style [landed in comment 73]

http://hg.mozilla.org/mozilla-central/rev/1cf41b2181da
Attachment #471545 - Attachment description: Use member instead of window style → Use member instead of window style [landed in comment 73]
I pushed my last two patches to try already, and Tomer said he could create a Hebrew language pack which people could test with.
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/smontagu@mozilla.com-aa37c042f1d3/ in due course.
(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).
(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 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.
Attachment #471552 - Flags: feedback?(ehsan) → feedback+
Comment on attachment 471577 [details] [diff] [review]
mirror caption buttons on glass desktops for mIsRTL windows

This is amazing, Jim!
(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.
(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.
updated
Attachment #471577 - Attachment is obsolete: true
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
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.
> (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.
(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/smontagu@mozilla.com-c4784d2567bf/
(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 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.
(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?
(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. :)
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.
(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/smontagu@mozilla.com-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/
(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?
(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.
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.
(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?
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.
(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
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?
Attachment #471635 - Flags: review?(roc)
(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.
Attached image ff 4.0b6pre (fixed)
today's trunk in hebrew seems to be fixed
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...
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.
Attachment #471635 - Flags: review?(roc) → review-
(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.
(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.
Attached patch dwm patchSplinter Review
Attachment #471635 - Attachment is obsolete: true
Attachment #472642 - Flags: review?(roc)
(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.
http://hg.mozilla.org/mozilla-central/rev/08155ca779b6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 594278
seems the bug is not fixed after all, this is the build1 of beta 6 (build 2 is currently not available for windows)
Attached image 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
could you find a regression range?
I suspect we've moved these checkins to beta7.
Right, please try a nightly? beta 6 is just a small tweak of beta 5, not a current snapshot.
Blocks: 596206
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
sorry, didn't know that, though i didn't notice anything on the wiki...
No longer blocks: 587247
Duplicate of this bug: 587247
Attachment #468761 - Attachment is obsolete: true
Attachment #468761 - Flags: review?
Attachment #468761 - Flags: feedback?(smontagu)
Attachment #468825 - Attachment is obsolete: true
Duplicate of this bug: 587244
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.