Native Windows theme support needs to be able to handle mirrored RTL drawing

RESOLVED FIXED in mozilla1.9.1a1

Status

()

Core
Widget: Win32
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Kai Liu, Assigned: Kai Liu)

Tracking

(Blocks: 1 bug, {fixed1.9.0.2, rtl})

Trunk
mozilla1.9.1a1
x86
Windows Vista
fixed1.9.0.2, rtl
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.2 +
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.26 KB, patch
vlad
: review+
Samuel Sidler (old account; do not CC)
: approval1.9.0.2+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 326428 [details] [diff] [review]
RTL-aware DrawThemeBackground

We need a way to mirror DrawThemeBackground so that widgets that are not direction-neutral will look right in RTL.  SetLayout is troublesome because it reverses the coordinate system of the DC's entire drawing area, which means that in addition to mirroring the appearance, it also mirrors the position.

We could either try adjusting our rects to counter the position changes that SetLayout causes or we could create a separate DC with the same size as the widget rect, call SetLayout on that, and transfer the results over.

I think I finally figured out how to reliably and exactly counter the position mirroring caused by SetLayout, so this patch uses the first approach.  Since SetLayout is a black-box function, I can't be completely sure that this is correct, but this works with a number of different widget styles on both Vista and XP, and I have yet to find a problem with it in my testing.

(This patch only adds a RTL-aware wrapper for DrawThemeBackground; the actual use of this will be implemented by the various bugs that this bug blocks.)
Attachment #326428 - Flags: superreview?(roc)
Attachment #326428 - Flags: review?(vladimir)
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
(Assignee)

Updated

10 years ago
Summary: Native Windows theme support needs to be able to handled mirrored RTL drawing → Native Windows theme support needs to be able to handle mirrored RTL drawing
(Assignee)

Comment 1

10 years ago
Created attachment 326685 [details] [diff] [review]
patch v1.1

It's legal for the clip rect to be null.
Attachment #326428 - Attachment is obsolete: true
Attachment #326685 - Flags: superreview?(roc)
Attachment #326685 - Flags: review?(vladimir)
Attachment #326428 - Flags: superreview?(roc)
Attachment #326428 - Flags: review?(vladimir)
(Assignee)

Updated

10 years ago
Attachment #326685 - Flags: superreview?(roc) → superreview?(vladimir)
Attachment #326685 - Flags: superreview?(vladimir)
Attachment #326685 - Flags: superreview+
Attachment #326685 - Flags: review?(vladimir)
Attachment #326685 - Flags: review+
Any way to create some tests for this?
(Assignee)

Comment 3

10 years ago
(In reply to comment #2)
> Any way to create some tests for this?
> 

I manually tested this function by having every widget take the new path if the direction is RTL (instead of just having the direction-sensitive widgets take the path) and everything looked okay.  As for automated testing, I haven't been able to come up with a good idea on how to do that, except to hard-code the desired end result into a PNG for a reftest, but that seems like it'd be fragile.
Hmm.. you could at least check that the RTL result is different than the LTR result, no?
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> Hmm.. you could at least check that the RTL result is different than the LTR
> result, no?
> 

Sure, I could do that.  I've attached the test to bug 403458.
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/8d870284418a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

10 years ago
Thanks for the push, Rob.  With your theme reorg landed, do you think it would be better if this lived in nsUXThemeData instead?
(Assignee)

Updated

10 years ago
Attachment #326685 - Flags: approval1.9.0.2?
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.1a1
Comment on attachment 326685 [details] [diff] [review]
patch v1.1

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #326685 - Flags: approval1.9.0.2? → approval1.9.0.2+
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Product: Core → SeaMonkey
(Assignee)

Updated

10 years ago
Component: Themes → Widget: Win32
Product: SeaMonkey → Core
Target Milestone: mozilla1.9.1a1 → ---
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9.1a1
Checked into 1.9.0 branch.
Keywords: checkin-needed → fixed1.9.0.2
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.