Notification balloons don't want the WS_EX_LAYOUTRTL style flag, since the di...

Notification balloons don't want the WS_EX_LAYOUTRTL style flag, since the directionality of the content is specified already inside the HTML.

BUG=46170
TEST=create a notification while using an RTL language for Chrome UI.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49723

[John Gregg, 2010-06-10 00:14:25]

Patch is adding a new flag to Widget::CreatePopupWidget to control whether the
RTL window style flags should be used, and updating all call sites to maintain
the previous behavior except for notifications.

[jeremy, 2010-06-10 11:17:04]

Idan, xji: Can you comment whether this is the right fix?

http://codereview.chromium.org/2794001/diff/1/8
File views/widget/widget.h (right):

http://codereview.chromium.org/2794001/diff/1/8#newcode69
views/widget/widget.h:69: NotAllowRTLLayout
I don't think "RTL Allowed" reflects what this does since IIUC, under the covers
this toggles a flag that controls whether the window origin is mirrored by the
OS.

Suggestions for alternate names
MirrorWindowOrigin
WindowOriginLTR

Also, a code comment explaining when you need to use either one would be helpful
since most developers aren't familiar with BiDi issues.

[John Gregg, 2010-06-10 16:23:18]

I will certainly add a comment.  The reason I called it "Allow" is because
passing the parameter does not cause RTL layout on its own, it only "allows" it
to happen in the native layer *if* the current locale is RTL.  

Perhaps MirrorOriginWhenRTL would be more accurate?

On 2010/06/10 11:17:04, jeremy wrote:
> Idan, xji: Can you comment whether this is the right fix?
> 
> http://codereview.chromium.org/2794001/diff/1/8
> File views/widget/widget.h (right):
> 
> http://codereview.chromium.org/2794001/diff/1/8#newcode69
> views/widget/widget.h:69: NotAllowRTLLayout
> I don't think "RTL Allowed" reflects what this does since IIUC, under the
covers
> this toggles a flag that controls whether the window origin is mirrored by the
> OS.
> 
> Suggestions for alternate names
> MirrorWindowOrigin
> WindowOriginLTR
> 
> Also, a code comment explaining when you need to use either one would be
helpful
> since most developers aren't familiar with BiDi issues.

[xji, 2010-06-10 18:14:21]

I tried a notification. Besides the incorrectly mirrored HTML contents, the
Options/Dismiss buttons do not looks correct.
I will attach screenshots in the bug.

http://codereview.chromium.org/2794001/diff/1/2
File chrome/browser/chromeos/text_input/candidate_window.cc (right):

http://codereview.chromium.org/2794001/diff/1/2#newcode834
chrome/browser/chromeos/text_input/candidate_window.cc:834:
views::Widget::AllowRTLLayout));
Agree, MirrorOriginInRTL might be a better choice.

http://codereview.chromium.org/2794001/diff/1/4
File chrome/browser/views/notifications/balloon_view.cc (right):

http://codereview.chromium.org/2794001/diff/1/4#newcode301
chrome/browser/views/notifications/balloon_view.cc:301: // this code is already
taking care of correctly reversing the layout.
Should it be "because the HTML layout is controlled by directionality specified
in HTML itself"?

http://codereview.chromium.org/2794001/diff/1/4#newcode308
chrome/browser/views/notifications/balloon_view.cc:308:
Widget::NotAllowRTLLayout);
Maybe DisableMirrorInRTL ?

http://codereview.chromium.org/2794001/diff/1/9
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/2794001/diff/1/9#newcode1436
views/widget/widget_gtk.cc:1436: RTLAllowedParam allow_rtl_layout) {
enable_mirror_in_rtl ?

[John Gregg, 2010-06-10 18:23:18]

What I saw is that the options/dismiss buttons are in the correct place, but the
hover behavior is wrong.  If that's what you saw, this patch also fixes that
problem.

On 2010/06/10 18:14:21, xji wrote:
> I tried a notification. Besides the incorrectly mirrored HTML contents, the
> Options/Dismiss buttons do not looks correct.
> I will attach screenshots in the bug.
> 
> http://codereview.chromium.org/2794001/diff/1/2
> File chrome/browser/chromeos/text_input/candidate_window.cc (right):
> 
> http://codereview.chromium.org/2794001/diff/1/2#newcode834
> chrome/browser/chromeos/text_input/candidate_window.cc:834:
> views::Widget::AllowRTLLayout));
> Agree, MirrorOriginInRTL might be a better choice.
> 
> http://codereview.chromium.org/2794001/diff/1/4
> File chrome/browser/views/notifications/balloon_view.cc (right):
> 
> http://codereview.chromium.org/2794001/diff/1/4#newcode301
> chrome/browser/views/notifications/balloon_view.cc:301: // this code is
already
> taking care of correctly reversing the layout.
> Should it be "because the HTML layout is controlled by directionality
specified
> in HTML itself"?
> 
> http://codereview.chromium.org/2794001/diff/1/4#newcode308
> chrome/browser/views/notifications/balloon_view.cc:308:
> Widget::NotAllowRTLLayout);
> Maybe DisableMirrorInRTL ?
> 
> http://codereview.chromium.org/2794001/diff/1/9
> File views/widget/widget_gtk.cc (right):
> 
> http://codereview.chromium.org/2794001/diff/1/9#newcode1436
> views/widget/widget_gtk.cc:1436: RTLAllowedParam allow_rtl_layout) {
> enable_mirror_in_rtl ?

[John Gregg, 2010-06-10 19:04:22]

New patch poster.  I changed the naming for the enums to be

enum MirroringParam {
  MirrorOriginInRTL
  DontMirrorOriginInRTL
}

[xji, 2010-06-10 19:04:37]

lgtm for html content part, after address the comments.

[xji, 2010-06-10 19:14:19]

lgtm.
But beng might be a better person to consult.

http://codereview.chromium.org/2794001/diff/15001/16003
File chrome/browser/views/notifications/balloon_view.cc (right):

http://codereview.chromium.org/2794001/diff/15001/16003#newcode301
chrome/browser/views/notifications/balloon_view.cc:301: // this code is already
taking care of correctly reversing the layout.
Is the reason "because HTML content layout is determined by directionality in
HTML itself"? I am afraid that "this code is already taking care of correctly
reversing the layout" mislead people thinking the HTML content is mirrored in
someway. Or I mis-understood?

[jeremy, 2010-06-10 20:58:58]

I'd really like to hear Idan's opinion on this patch.

I agree with xji's comments on the screenshot in the bug, could you post
screenshots of the rtl & ltr notification with this patch?

http://codereview.chromium.org/2794001/diff/15001/16007
File views/widget/widget.h (right):

http://codereview.chromium.org/2794001/diff/15001/16007#newcode74
views/widget/widget.h:74: // popup will be mirrored if the current locale is
RTL.  You should use
"the contents of the popup will be mirrored" -> The view origin will be
mirrored.

[John Gregg, 2010-06-10 21:09:54]

On 2010/06/10 20:58:58, jeremy wrote:
> I'd really like to hear Idan's opinion on this patch.
> 
> I agree with xji's comments on the screenshot in the bug, could you post
> screenshots of the rtl & ltr notification with this patch?

xji's comments are comparing two *different versions* of the notifications UI,
both of which are out of date relative to trunk.  Some of the problems he
identifies are RTL problems, some of them (like removing "from") were
intentional UI changes.

After the patch, the notifications look like this:
english chrome: http://www/~johnnyg/wrench_notifications_win.png
arabic chrome: http://www/~johnnyg/wrench_notifications_win_rtl.png
(with correct hover behavior shown on the options menu).

> 
> http://codereview.chromium.org/2794001/diff/15001/16007
> File views/widget/widget.h (right):
> 
> http://codereview.chromium.org/2794001/diff/15001/16007#newcode74
> views/widget/widget.h:74: // popup will be mirrored if the current locale is
> RTL.  You should use
> "the contents of the popup will be mirrored" -> The view origin will be
> mirrored.

[John Gregg, 2010-06-10 21:11:08]

> After the patch, the notifications look like this:
> english chrome: http://www/%7Ejohnnyg/wrench_notifications_win.png
> arabic chrome: http://www/%7Ejohnnyg/wrench_notifications_win_rtl.png
> (with correct hover behavior shown on the options menu).

Note that in both cases, the notification content itself is correctly LTR, as
provided by the web page that created it (like the extension in the original
bug)

[jeremy, 2010-06-10 21:20:34]

Screenshots LGTM, thanks.

On Fri, Jun 11, 2010 at 12:11 AM, <johnnyg@chromium.org> wrote:

>  After the patch, the notifications look like this:
>> english chrome: http://www/%7Ejohnnyg/wrench_notifications_win.png
>> arabic chrome: http://www/%7Ejohnnyg/wrench_notifications_win_rtl.png
>>
>> (with correct hover behavior shown on the options menu).
>>
>
> Note that in both cases, the notification content itself is correctly LTR,
> as
> provided by the web page that created it (like the extension in the
> original
> bug)
>
>
> http://codereview.chromium.org/2794001/show
>

[John Gregg, 2010-06-11 21:13:03]

So are we good on the code, or still want to hear from Idan/Ben?

On 2010/06/10 21:20:34, jeremy wrote:
> Screenshots LGTM, thanks.
> 
> On Fri, Jun 11, 2010 at 12:11 AM, <mailto:johnnyg@chromium.org> wrote:
> 
> >  After the patch, the notifications look like this:
> >> english chrome: http://www/%257Ejohnnyg/wrench_notifications_win.png
> >> arabic chrome: http://www/%257Ejohnnyg/wrench_notifications_win_rtl.png
> >>
> >> (with correct hover behavior shown on the options menu).
> >>
> >
> > Note that in both cases, the notification content itself is correctly LTR,
> > as
> > provided by the web page that created it (like the extension in the
> > original
> > bug)
> >
> >
> > http://codereview.chromium.org/2794001/show
> >
>

[jeremy, 2010-06-11 22:24:38]

I'm ok landing this now, we can revisit if Idan has an issue.  Can you try
to catch Ben and see if he can review this?

On Sat, Jun 12, 2010 at 12:13 AM, <johnnyg@chromium.org> wrote:

> So are we good on the code, or still want to hear from Idan/Ben?
>
>
> On 2010/06/10 21:20:34, jeremy wrote:
>
>> Screenshots LGTM, thanks.
>>
>
>  On Fri, Jun 11, 2010 at 12:11 AM, <mailto:johnnyg@chromium.org> wrote:
>>
>
>  >  After the patch, the notifications look like this:
>> >> english chrome: http://www/%257Ejohnnyg/wrench_notifications_win.png
>> >> arabic chrome:
>> http://www/%257Ejohnnyg/wrench_notifications_win_rtl.png
>>
>> >>
>> >> (with correct hover behavior shown on the options menu).
>> >>
>> >
>> > Note that in both cases, the notification content itself is correctly
>> LTR,
>> > as
>> > provided by the web page that created it (like the extension in the
>> > original
>> > bug)
>> >
>> >
>> > http://codereview.chromium.org/2794001/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2794001/show
>

[idana, 2010-06-16 13:46:16]

John,

Nice working with you again ;-)

Apologies for not reviewing this earlier. The patch looks good overall. The only
thing I am worried about is the position of the notification window when the OS
UI is RTL. In the Hebrew/Arabic version of Windows (at least Windows XP IIRC),
the start menu is on the bottom right and therefore the notification window's
position should be mirrored too. I am not sure whether or not the code currently
handles this correctly.

http://codereview.chromium.org/2794001/diff/15001/16007
File views/widget/widget.h (right):

http://codereview.chromium.org/2794001/diff/15001/16007#newcode80
views/widget/widget.h:80: MirroringParam mirror_in_rtl);
Since almost all the call sites use MirrorOriginInRTL (the default behavior), it
makes more sense to overload CreatePopupWidget such that only BalloonView uses
the version which takes the mirroring config.