Incorrect display of right-to-left menus in Vista

VERIFIED FIXED in mozilla1.9.1a1

Status

()

defect
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: tomer, Assigned: kliu)

Tracking

({rtl, verified1.9.0.2, verified1.9.1})

Trunk
mozilla1.9.1a1
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9 -
blocking1.9.0.1 -
blocking1.9.0.2 +
wanted1.9.0.x +
in-testsuite +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MU+][needs 441452])

Attachments

(6 attachments, 5 obsolete attachments)

Mozilla/5.0 (Windows; U; Windows NT 6.0; he; rv:1.9b2pre) Gecko/2007111004 Minefield/3.0b2pre

A. There is a bar at the left, which should be moved to the right.
B. Subfolder arrows should be flipped.
Over to xptoolkit/menus.

Tomer, if you want these bugs to get fixed on the platform side, you have to file them there. As long as you file them in Localizations-Hebrew, developers will just not look at it.
Component: he-IL / Hebrew → XP Toolkit/Widgets: Menus
Product: Mozilla Localizations → Core
QA Contact: hebrew.he → xptoolkit.menus
Version: unspecified → Trunk
Ok. Thanks.
Blocks: fx3-l10n-he
Blocks: fx35-l10n-fa
No longer blocks: Persian-Fx3.5
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Both of the issues I've described in the are still visible with beta5. Please help.

Mozilla/5.0 (Windows; U; Windows NT 6.0; he; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Giving this one a shot.  The summary is edited in order to reflect the nature of the problem.
Assignee: nobody → ehsan.akhgari
Summary: [RTL] Firefox 3 theme issues in Windows Vista → Incorrect display of right-to-left menus in Vista
Status: NEW → ASSIGNED
Posted patch WIP Patch (obsolete) — Splinter Review
This is a WIP patch which solves a number of issues: incorrect display of the menu gutter and menu drop down arrows, and incorrect placement of checkboxes and radio buttons within the menus.  This patch is a work in progress because of two issues:

1. When moving the mouse cursor on the menu, things seem to be shifted 1 pixel to the right.

2. The code for the placement of the arrow on the menu uses the width of a menu separator.  This seems to be incorrect for very wide menus (with wide accelltexts).

I'll provide screenshots of both problems.  I may need some help to figure out what's causing these two problems, and how to solve them.
Please note that I have moved the mouse from the top of the menu on the first 5 menu items, and the background of these items has been shifted 1 pixel to the right.
Screenshot of the problem 2 with wide menu items both with and without wide acceltexts.
The first issue could either be a painting problem when we repaint only part of the menu, or a problem where parts of the menu have actually moved but we haven't repainted them all. One way to narrow that down would be to see if there's a difference between the first time you show the menu and the next time. If that doesn't help, I'd add logging code where we paint the gutter bar and try to identify the difference in parameters that is causing the difference in painting, and then work my way up the call stack with logging code to try to show where that difference came from. Another option would be to set breakpoints and try to catch the two different painting situations in the debugger, although that's tricky. You might find that setting an "ignore count" for the breakpoint in the debugger would let you open the menu and then select several items before the breakpoint triggers, which would let you catch the "bad paint when selecting an item" case.

Not really sure about the second one. I'd check if it's a problem with LTR (presuambly it isn't) and if it isn't, I'd try to figure out how it works for LTR.
I was testing my patch in bug 435825 on RTL and when I noticed just how broken the menu was on Vista, I hacked up a similar patch before I thought to search to see if someone had already filed a bug about this, and that's when I found this bug.  I'm surprised nobody's marked this as blocking...

(In reply to comment #6)
> 1. When moving the mouse cursor on the menu, things seem to be shifted 1 pixel
> to the right.
> 

When I tested my version of the RTL fix patch, the first menu two menus that I tried didn't exhibit this behavior, so it took me a while to finally run into this problem.  That this problem only shows up with some menus made me suspect rounding errors, so I played around with a context menu that I'd try popping up at various points on the screen, and as I suspected, whether or not the problem manifested itself was dependent on the location of the menu, which I think strongly suggests a rounding error.  Having the gutter positioned on the right instead of the left exposes us to width calculations and the potential round-off errors associated with them.

> 2. The code for the placement of the arrow on the menu uses the width of a menu
> separator.  This seems to be incorrect for very wide menus (with wide
> accelltexts).
>

That's because GetThemePartSize retrieves a generic size from the theme (IIRC, it corresponds to the size of the bitmap image used to draw these things) and is not necessarily the size of the element as presented on the screen.  GetThemePartSize on the menu separator part will always return 144x6, and the only part of that result that could be used is the height.
(In reply to comment #10)
> I was testing my patch in bug 435825 on RTL and when I noticed just how broken
> the menu was on Vista, I hacked up a similar patch before I thought to search
> to see if someone had already filed a bug about this, and that's when I found
> this bug.  I'm surprised nobody's marked this as blocking...

I would be very glad to see your patch.  Can you attach it to this bug, please?

> (In reply to comment #6)
> > 1. When moving the mouse cursor on the menu, things seem to be shifted 1 pixel
> > to the right.
> > 
> 
> When I tested my version of the RTL fix patch, the first menu two menus that I
> tried didn't exhibit this behavior, so it took me a while to finally run into
> this problem.  That this problem only shows up with some menus made me suspect
> rounding errors, so I played around with a context menu that I'd try popping up
> at various points on the screen, and as I suspected, whether or not the problem
> manifested itself was dependent on the location of the menu, which I think
> strongly suggests a rounding error.  Having the gutter positioned on the right
> instead of the left exposes us to width calculations and the potential
> round-off errors associated with them.

Yeah, this looks like it can be the culprit.  We need to find out why the rounding error only occurs when menu items are hovered, though.

> > 2. The code for the placement of the arrow on the menu uses the width of a menu
> > separator.  This seems to be incorrect for very wide menus (with wide
> > accelltexts).
> >
> 
> That's because GetThemePartSize retrieves a generic size from the theme (IIRC,
> it corresponds to the size of the bitmap image used to draw these things) and
> is not necessarily the size of the element as presented on the screen. 
> GetThemePartSize on the menu separator part will always return 144x6, and the
> only part of that result that could be used is the height.

Hmmm.  Is there any way to find the size of the actual element drawn in Win32/Gecko?

I may find some time to play with this again later today.  It would be very helpful if you can attach your patch as well.

Thanks!
Posted patch my wip (obsolete) — Splinter Review
(In reply to comment #11)
> I would be very glad to see your patch.  Can you attach it to this bug, please?
> 

There really isn't much to see; what I did was similar to what you did.  Anyway, I'm attaching what I have.  To make things less noisy, I also included a diff against the bug 435825 patch, which is probably more useful.

> We need to find out why the
> rounding error only occurs when menu items are hovered, though.
> 

Yes, that's the part that confuses me.  Unfortunately, I'm still very new to all this, so I don't know where such a rounding error would be such that it only takes effect during the hover.  It would've made more sense if it showed the same incorrect offset during hover and non-hover.

> Hmmm.  Is there any way to find the size of the actual element drawn in
> Win32/Gecko?
> 

The arrow container frame should still be laid out on the left, and it's only the painting that's happening on the right.  In order for SetLayout to flip the painting position to the right, it would need to know horizontal size of the popup or menu item, and I'm guessing that it's getting that from the device context.  If there was a way to extract that from the hdc (an initial attempt last night with GetViewportExtEx didn't work), then we could exactly reverse the position mirroring that SetLayout does (argh, why doesn't Microsoft provide an image-mirroring-only option?), but that's only a guess; I haven't had enough time to play with this a whole lot.  Alternatively, we could get the dimensions of the parent frame and use that to undo the position mirroring, but I think it's best to undo the SetLayout positional mirroring using the same methods used by SetLayout to achieve it, if possible.

Or we could just draw the arrow using the classic menu code if it's RTL and just avoid the SetLayout re-positioning mess all together.
Hmm.  Bugzilla is treating my attachment as a diff of one file instead of two separate diffs.  The stuff starting at "Lines 454-469" is the start of the second diff.


Ideally, It'd be nice to avoid using SetLayout all together; the fewer weird blackbox Microsoft functions we call, the better.  Unfortunately, it's unavoidable for the gutter line and for the the arrow (unless we ditch drawThemeBG for the arrow in RTL).  In the case of the gutter line, what I had planned on doing at first was something along the lines of
gutterRect.right = bgRect.left - gutterSize.cx + widthOfGutterLine;
But it turns out that the theme reports the gutter line size as being 3 pixels wide, even though the line itself is only 2 pixels wide (I guess there's a transparent pixel in there), and I didn't feel comfortable with hard-coding a 1px correction to the reported line width.
Here's a log (with comments) of the the gutter and clip rects being passed to DrawThemeBackground each time the background is painted.

// Opening the menu for the first time (it's painting twice for some reason)
gutter: pos(3,3) dim(28,176)
g-clip: pos(0,0) dim(255,182)
gutter: pos(3,3) dim(28,176)
g-clip: pos(0,0) dim(255,182)

// Paint events as I move my mouse down the menu
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,3) dim(250,45)
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,3) dim(250,45)
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,25) dim(250,44)
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,47) dim(250,45)
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,69) dim(250,45)
// This item had a flyout submenu that opened on the right
gutter: pos(3,3) dim(28,176)
g-clip: pos(3,113) dim(246,22)

// Paint event when I move my mouse out and the entire
// menu repaints and fixes itself
gutter: pos(3,3) dim(28,176)
g-clip: pos(0,0) dim(255,182)

The gutter rect's position and dimensions are consistent (and correct) for every one of the paint events.  What does worry me, though, is the clipping rect's size.  The menu's width is 255px, and with 3px margins on each edge, the clipping rect for the hovers should be coming out to 249px, not 250px.  Of course, the size of the clipping rect isn't going to affect or cause this mis-rendering, but I guess it is indicative of the problem.  I should also point at the final hover, the one where the clipping rect came out to 246px.  That was from a submenu that opened to the right (there was no space on the left).  In that case, the line (and the menu arrow) shifted to the *left* by several pixels (not just 1px).

So of the three things being passed to DrawThemeBackground, the gutter rect is correct, the clipping rect only affects which parts should be repainted, leaving us with only the device context--the dimensions of which appear to be used by SetLayout to do the positional mirroring.  But the dimensions of the device context are not consistent; they are subject to what looks like a rounding error on hover, and when there's a popup menu that obstructs part of the menu that we are trying to draw, that appears to affect the dimensions of the device context as well.  I don't think the device context dimensions as provided by Gecko were ever intended to be used for positioning, but by using SetLayout, that's what we're doing.  Also, when testing your patch, I had problems with the menu popup's outer borders shifting when hovering over a menu flyout that opened on the right; I didn't see that with my patch because I only did SetLayout on the gutter and not the entire menu popup, so that also points a finger at the way SetLayout is doing its positional mirroring.

So the problem, I think, is with using SetLayout--we really shouldn't be using it because even if the rounding error (well, at least what I think is a rounding error) is fixed, there is still the problem of the positioning being wrong when a menu flyout is positioned over the right edge, thus pushing things inward, and all this seems to be a larger problem of the way SetLayout does the position mirroring being incompatible with Gecko (and I'm guessing that it's a problem with the device context dimensions)

Which, unfortunately, means that we have to do two hackish things for the Vista menu in RTL:
1) Fall back to the classic menu arrow rendering if we're using RTL.
2) gutterRect.right = bgRect.left - gutterSize.cx + 2;  // hard-code 2px
I don't like either hack, but given that the alternative is to use SetLayout, I think I can live with this.
This works.  I'll post a patch against trunk shortly.

(BTW, looking back over my comments, I made a typo in comment 13 that I copied into comment 14; I meant to say bgRect.right, not bgRect.left)
Attachment #322761 - Attachment is obsolete: true
Not sure who to target for a review...

In my testing of this patch, this does not cause any problems with LTR menus or classic RTL menus, and it fixes the RTL Vista menus without running into the problems in comment 6.

This should be a very safe patch because everything that this patch touches is if'ed by IsFrameRTL.  Because of how obviously wrong the current menu looks under Vista RTL and because this is a safe patch, I'd like to nominate this for RC2.
Attachment #322797 - Attachment is obsolete: true
Attachment #322806 - Flags: superreview?(vladimir)
Attachment #322806 - Flags: review?(vladimir)
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Whiteboard: [RC2?]
Wow, thanks Kai for picking up my slack!  :-)

In order to provide the endgame drivers with a sense of the degree of broken-ness of RTL builds in Vista, please check out this screen shot.  You can see problems in menu backgrounds, the positioning of the checkbox and radio icons, and the incorrect direction for the drop-down arrow.  Attachment 288288 [details] provides another screenshot of the problem.
Whiteboard: [RC2?] → [has patch][needs review vlad][RC2?]
We'll get this in the first security and stability release.
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1+
Flags: blocking1.9-
Whiteboard: [has patch][needs review vlad][RC2?] → [has patch][needs review vlad][RC2-]
Posted patch patch v1.1 (obsolete) — Splinter Review
Same patch as comment 16, except I moved the fallback check off to a separate inlined function to reduce code duplication (this also makes adding similar forced classic fallbacks--one may be needed for the RTL resizer grip--easier in the future).
Assignee: ehsan.akhgari → kliu
Attachment #317917 - Attachment is obsolete: true
Attachment #322806 - Attachment is obsolete: true
Attachment #322938 - Flags: superreview?(vladimir)
Attachment #322938 - Flags: review?(vladimir)
Attachment #322806 - Flags: superreview?(vladimir)
Attachment #322806 - Flags: review?(vladimir)
Blocks: 436779
No longer blocks: fx35-l10n-fa
Is there someone other than Vlad who can review this? He's not available for the next two weeks.
Oh, okay.  I picked him because he reviewed the patch that originally implemented uxthemed menus for Vista, but I guess I could ask someone else.
Attachment #322938 - Flags: superreview?(vladimir)
Attachment #322938 - Flags: superreview?(roc)
Attachment #322938 - Flags: review?(vladimir)
Attachment #322938 - Flags: review?(roc)
Attachment #322938 - Flags: superreview?(roc)
Attachment #322938 - Flags: superreview+
Attachment #322938 - Flags: review?(roc)
Attachment #322938 - Flags: review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review vlad][RC2-] → [has patch,review][RC2-]
Attachment #322938 - Flags: approval1.9.0.1?
This won't block the release of 3.0.1, but we'd take a reviewed patch that was tested on RTL and LTR builds. Has it been? :)
Flags: wanted1.9.0.x+
Flags: blocking1.9.1+
Flags: blocking1.9.0.1-
Flags: blocking1.9.0.1+
Whiteboard: [has patch,review][RC2-] → [has patch,review][MU+]
(In reply to comment #22)
> tested on RTL and LTR builds. Has it been? :)
> 

Yes.
Depends on: 441452
The patch in comment 19 works and has been tested on both RTL and LTR.  It's a safe patch, and it does fix this bug.  But it's also hackish in its approach, and I never really liked it.

This patch, which depends on bug 441452, is, I think, much better.  Like the comment 19 patch, it doesn't affect LTR at all, and it fixes this bug without running into any of the problems described earlier in this bug.
Attachment #326429 - Flags: superreview?(roc)
Attachment #326429 - Flags: review?(vladimir)
Flags: in-litmus?
Keywords: checkin-needed
Attachment #322938 - Flags: approval1.9.0.1?
I think Vlad's still away from reviews, but maybe Roc can look at it before we freeze up for 3.0.1.
Vlad's back tomorrow, is that soon enough?
Whiteboard: [has patch,review][MU+] → [has patch][MU+]
Attachment #326429 - Flags: superreview?(roc) → superreview?(vladimir)
Attachment #326429 - Flags: superreview?(vladimir)
Attachment #326429 - Flags: superreview+
Attachment #326429 - Flags: review?(vladimir)
Attachment #326429 - Flags: review+
Posted patch testSplinter Review
Per bug 441452 comment 4, here is a test that fails on Vista without the patch but should pass with the patch.
Attachment #322938 - Attachment is obsolete: true
Attachment #328048 - Flags: review?(vladimir)
Vlad, ping?
Whiteboard: [has patch][MU+] → [has patch][MU+][test needs review then landing]
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/0836a89c5b59
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][MU+][test needs review then landing] → [MU+]
Attachment #326429 - Flags: approval1.9.0.2?
Component: XP Toolkit/Widgets: Menus → Widget: Win32
QA Contact: xptoolkit.menus → win32
Target Milestone: --- → mozilla1.9.1a1
Flags: blocking1.9.0.2+
Whiteboard: [MU+] → [MU+][needs baking]
Comment on attachment 326429 [details] [diff] [review]
patch v2: alternate, less hackish, approach

Approved for 1.9.0.2. Please land in CVS. a=ss

Also, be sure to land the test when this patch lands.
Attachment #326429 - Flags: approval1.9.0.2? → approval1.9.0.2+
Whiteboard: [MU+][needs baking] → [MU+][needs 441452]
Keywords: checkin-needed
Patch checked into 1.9.0 branch with test.
Flags: in-testsuite+
Verified fix on Mozilla/5.0 (Windows; U; Windows NT 6.0; he; rv:1.9.0.2pre) Gecko/2008081906 GranParadiso/3.0.2pre.   
Status: RESOLVED → VERIFIED
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.