651905 - Caption buttons show on top of the Firefox button with a RTL language pack applied to a LTR Firefox when system menu is displayed

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Eric Sh.]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0

When I right click the menu bar of a normal window(not full screen one) with firefox button enabled, or with a full screen with the firefox button enabled and with the tabs not ontop, the normal close, maximize and minimize buttons show up ontop of the firefox button and they are on both sides of the menubar.

when I click on a place on the screen in firefox or outside it the buttons dissapear.

Note: This also happens sometimes when I right click the firefox task on the taskbar.

Reproducible: Always

Steps to Reproduce:
1. Open a firefox window set up as said in the deatils
2. Right click the menubar
Actual Results:  
Buttons show on top of the firefox button.

Expected Results:  
The firefox button will stay as it is.

Comment 1

[Eric Sh.]

Attachment: Firefox window menubar with buttons on top of the firefox button

Comment 2

[aravindm]

Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110421 Firefox/6.0a1

Works fine for me. 
Does the issue still occur if you start Firefox in Safe Mode?
http://support.mozilla.com/en-US/kb/Safe+Mode

How about with a new, empty profile?
http://support.mozilla.com/en-US/kb/Basic+Troubleshooting#Make_a_new_profile

Comment 3

[Eric Sh.]

Have the same bug in Safe Mode and in a new profile I just created in the profile manager.

I also downloaded the portable version of firefox and looks like this bug is specific for the Right-To-Left version of firefox because I downloaded the Left-To-RIght portable version and it works fine.

Comment 4

[Eric Sh.]

And why are you testing with firefox 6.0? This is a 4.0 bug.

Comment 5

[Eric Sh.]

Just tested and found out that if you use an english version of firefox and install a right-to-left(for example hebrew or arabic) language pack you can replicate this bug

See:
http://kb.mozillazine.org/Language_packs#Downloading_a_language_pack

You'd better download a portable version or use a clean profile if you don't know any RTL language.

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

Moving to Core for further Investigation.

Comment 7

[Kai Liu]

I don't think this is a widget issue.

I can't reproduce this using the steps in comment 0 with a full RTL version, nor with a Force RTL'ed LTR version.  So if the problem is present only on a LTR version with a RTL language pack, then it's probably an issue with how the language pack is being applied...

Comment 8

[Eric Sh.]

(In reply to comment #7)
> I don't think this is a widget issue.
> 
> I can't reproduce this using the steps in comment 0 with a full RTL version,
> nor with a Force RTL'ed LTR version.  So if the problem is present only on a
> LTR version with a RTL language pack, then it's probably an issue with how the
> language pack is being applied...

That is very strange, what language is your RTL version?
And are you sing a RTL windows?

I will check on other computers with RTL firefox but not RTL windows and post soon...

Comment 9

[Jim Mathies [:jimm]]

Rather odd because the caption buttons on the right look like they are being drawn by Windows, and the buttons on the left are drawn by us when the fx button is enabled on XP. We remove the entire non-client caption area in this case so Windows should not be drawing there at all.

Comment 10

[(no longer active)]

Tomer, can you reproduce this?

Comment 11

[dror3go]

I can reproduce this bug, XP & Firefox 4.0.1.

Comment 12

[Tomer Cohen :tomer]

Is it a theme issue? Becuase from the screenshot above I understand it only happen on Windows Classic theme.

(In reply to comment #11)
> I can reproduce this bug, XP & Firefox 4.0.1.

Thanks Dror! ☺

Comment 13

[dror3go]

Attachment: bug showen when using different theme (FXChrome)

(In reply to comment #12)
> Is it a theme issue? Becuase from the screenshot above I understand it only
> happen on Windows Classic theme.

This bug is *not* a theme issue. The browser paints the 3 buttons for some reason.

To reproduce it, all that's needed is:
- Uncheck the "menu bar" under view -> toolbars
- Change intl.uidirection.en to "rtl" in about:config
- Make the window not maximized
- Open context menu for the window.

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> Created attachment 529978 [details]
> bug showen when using different theme (FXChrome)
> 
> (In reply to comment #12)
> > Is it a theme issue? Becuase from the screenshot above I understand it only
> > happen on Windows Classic theme.
> 
> This bug is *not* a theme issue. The browser paints the 3 buttons for some
> reason.
> 
> To reproduce it, all that's needed is:
> - Uncheck the "menu bar" under view -> toolbars
> - Change intl.uidirection.en to "rtl" in about:config
> - Make the window not maximized
> - Open context menu for the window.

Can you do all that, but after changing the pref, restart first and see if the problem still exists?

Comment 15

[dror3go]

> Can you do all that, but after changing the pref, restart first and see if the
> problem still exists?

Yes, it does.

Comment 16

[Simon Montagu :smontagu]

Apparently this bug is only in Windows XP. However, we used to have a very similar bug in Windows 7, but it was fixed in attachment 472642 [details] [diff] [review] in bug 588735. See the screenshot (before the fix) in attachment 471984 [details].

Comment 17

[Simon Montagu :smontagu]

(In reply to comment #16)
> Apparently this bug is only in Windows XP.

Correction: I can reproduce in Windows 7 with Windows Classic theme.

Comment 18

[Simon Montagu :smontagu]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bb4f093a50b&tochange=b36eeab1df8b

Comment 19

[Simon Montagu :smontagu]

Jimm, could this have been caused by bug 602532 or bug 602450?

Comment 20

[Simon Montagu :smontagu]

(In reply to comment #17)
> Correction: I can reproduce in Windows 7 with Windows Classic theme.

... or any non-Aero theme.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #19)
> Jimm, could this have been caused by bug 602532 or bug 602450?

Those are both specific to the system menu. If we have a case where the window is correct but the system menu isn't aligned right, then maybe, but since the window's command buttons are also aligned incorrectly, I'd say not likely.

Comment 22

[Simon Montagu :smontagu]

Yes, but the misaligned command buttons only appear when the system menu is displayed.

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #20)
> (In reply to comment #17)
> > Correction: I can reproduce in Windows 7 with Windows Classic theme.
> 
> ... or any non-Aero theme.

Ok, I can reproduce this now. I was under the impression the buttons were always being drawn in the wrong place, but I see now it's only when you bring up the system menu, and then they disappear after the menu is dismissed. This is definitely a widget drawing bug.

Comment 24

[Jim Mathies [:jimm]]

On english systems with the rtl setting reversed, I see that the click alignment of the context menu is correct, but the text is not. I'm assuming there's nothing we can do about that since the context menu is created by the system.

Comment 25

[Simon Montagu :smontagu]

Can we pass TPM_LAYOUTRTL to TrackPopupMenu when the window is RTL?

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #25)
> Can we pass TPM_LAYOUTRTL to TrackPopupMenu when the window is RTL?

Ah, yes. I looked at the TPM params on msdn, they didn't list it except in a little blurb at the bottom :) That looks like it would fix the context menu.

Comment 27

[Jim Mathies [:jimm]]

Attachment: menu patch

Unfortunately it doesn't fix the buttons. Windows still thinks it's an ltr window.

Comment 28

[Simon Montagu :smontagu]

Right, I see the reverse on Hebrew Windows - the system menu is always Hebrew, and always RTL, whatever the language and orientation of the app. It's the same in other apps, e.g. OpenOffice.

Comment 29

[Jim Mathies [:jimm]]

On the caption button drawing, it looks like Windows is doing some spurious nc client area painting on either the context menu event or the mouse down event. We need to track down which and figure out some way to prevent it. WM_SETTEXT handling may hold clues on how to do this. If someone wants to pick this up please do, otherwise I'll try to get back to it at some point.

Comment 30

[Simon Montagu :smontagu]

Attachment: Use SetMenuItemInfo instead of EnableMenuItem

Apparently the spurious button painting is a side-effect of calling EnableMenuItem, and SetMenuItemInfo doesn't have the side-effect.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 530287 [details] [diff] [review]
Use SetMenuItemInfo instead of EnableMenuItem

They must repaint the buttons when you change the menu state.. tricky tricky.

Comment 32

[Simon Montagu :smontagu]

Attachment: LTR version of the bug

By the way, the bug is reproducible without forcing RTL UI, as long as one uses a Firefox theme which changes the appearance of the control buttons.

Comment 33

[(no longer active)]

Comment on attachment 530264 [details] [diff] [review]
menu patch

Do we still want this?

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 530264 [details] [diff] [review]
menu patch

I believe we still want this but I'll defer to Simon on it..

Comment 35

[Simon Montagu :smontagu]

There are tryserver builds with attachment 530287 [details] [diff] [review] at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e9b2acd4baff/try-win32/

If someone can confirm that this fixes the bug in WinXP also, that would be great.

Comment 36

[Simon Montagu :smontagu]

Comment on attachment 530264 [details] [diff] [review]
menu patch

What effect does this have exactly on an LTR Windows system?

Comment 37

[Jim Mathies [:jimm]]

(In reply to comment #36)
> Comment on attachment 530264 [details] [diff] [review] [review]
> menu patch
> 
> What effect does this have exactly on an LTR Windows system?

It flips the alignment of the text in the system context menu. I thought that was part of this bug.

Comment 38

[Simon Montagu :smontagu]

Comment on attachment 530264 [details] [diff] [review]
menu patch

Review of attachment 530264 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, sorry, I misunderstood comment 27.

Unfortunately, when I tested the patch on Hebrew Windows 7, it makes the system menu LTR when Firefox UI is RTL, which we don't want. I'd like to check out what it does on Hebrew XP, but I won't have access to an XP system until sometime next week.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> Comment on attachment 530264 [details] [diff] [review] [review]
> menu patch
> 
> Review of attachment 530264 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Oh, sorry, I misunderstood comment 27.
> 
> Unfortunately, when I tested the patch on Hebrew Windows 7, it makes the
> system menu LTR when Firefox UI is RTL, which we don't want. I'd like to
> check out what it does on Hebrew XP, but I won't have access to an XP system
> until sometime next week.

I was worried about that. I think we should only apply the flag when fx is ltr, and the os is rtl. (Although I guess it'll still be in english I think, regardless of the language firefox is using.)

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #39)
> (In reply to comment #38)
> > Comment on attachment 530264 [details] [diff] [review] [review] [review]
> > menu patch
> > 
> > Review of attachment 530264 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Oh, sorry, I misunderstood comment 27.
> > 
> > Unfortunately, when I tested the patch on Hebrew Windows 7, it makes the
> > system menu LTR when Firefox UI is RTL, which we don't want. I'd like to
> > check out what it does on Hebrew XP, but I won't have access to an XP system
> > until sometime next week.
> 
> I was worried about that. I think we should only apply the flag when fx is
> ltr, and the os is rtl. (Although I guess it'll still be in english I think,
> regardless of the language firefox is using.)

eh, when fx is rtl, and the os is ltr!

Comment 41

[Simon Montagu :smontagu]

I'm in two minds about this. Maybe we should always use the OS direction since the menu will always be in the OS language?

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> I'm in two minds about this. Maybe we should always use the OS direction
> since the menu will always be in the OS language?

Fine with me, but I don't use rtl versions so I don't know how seriously rtl users take something like that. I'd suggest we file a follow up if we consider it serious enough and land your fix here for the bigger problem.

Comment 43

[(no longer active)]

(In reply to comment #41)
> I'm in two minds about this. Maybe we should always use the OS direction since
> the menu will always be in the OS language?

Yes, I think that is what we should do.

Comment 44

[Simon Montagu :smontagu]

Pushed http://hg.mozilla.org/mozilla-central/rev/1f5db39fbb1a after Dror confirmed on IRC that the patch fixes XP also.