Adjust RTL formatting for web notifications in cros system tray.

Adjust RTL formatting for web notifications in cros system tray.

BUG=none

Review-Url: https://codereview.chromium.org/2721213002
Cr-Commit-Position: refs/heads/master@{#454963}
Committed: https://chromium.googlesource.com/chromium/src/+/ccb33d40c407ae11f528895dc7ceeb0d558ad290

[Evan Stade, 2017-03-01 04:13:04]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2017-03-02 17:25:05]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 17:25:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 17:55:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 17:55:44]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-03-02 21:32:32]

tdanderson@chromium.org changed reviewers:
+ mohsen@chromium.org

[tdanderson, 2017-03-02 21:32:33]

It's not immediately clear to me that you're doing the right thing; Mohsen
brought up some comments in person and has agreed to review this, +mohsen@.

Also can you please file a bug for this, containing a description of why what
we're doing right now is wrong, and how it should be fixed. (Without a bug, this
will never get verified by TEs, and the history of the change will exist only in
git logs, essentially hiding it from all non-SWEs.)

[mohsen, 2017-03-02 21:45:21]

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
ash/common/system/web_notification/web_notification_tray.cc:272:
base::i18n::WrapStringWithRTLFormatting(&str);
If I understand correctly, when the context is RTL, we still want to show "+n"
text LTR (as mathematical expressions are written LTR in RTL languages). In this
case, shouldn't we use WrapStringWithLTRFormatting (note the LTR) to force "+n"
written LTR?

[Evan Stade, 2017-03-02 22:22:52]

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
ash/common/system/web_notification/web_notification_tray.cc:272:
base::i18n::WrapStringWithRTLFormatting(&str);
On 2017/03/02 21:45:21, mohsen wrote:
> If I understand correctly, when the context is RTL, we still want to show "+n"
> text LTR (as mathematical expressions are written LTR in RTL languages). In
this
> case, shouldn't we use WrapStringWithLTRFormatting (note the LTR) to force
"+n"
> written LTR?

We want to show "99+" in RTL. That is what Chrome currently does, this just
effects that more gracefully. You can go type plus nine nine in a hebrew web
page and see that on screen it looks like 99+.

This text is put in a label. Labels intuit their directionality from the text,
not the UI (i.e. they don't care about i18n::IsRTL()). "+99" on its own has no
strong directional characters. Thus we'd fall back to a default of LTR for
displaying it. Wrapping with the RTL indicating character simply makes us use
RTL to display "+99", which ends up looking like "99+" on screen.

The only thing effectively incorrect about the present code is what the output
of a screen reader would be.

[Evan Stade, 2017-03-03 00:10:53]

On 2017/03/02 21:32:33, tdanderson wrote:
> It's not immediately clear to me that you're doing the right thing; Mohsen
> brought up some comments in person and has agreed to review this, +mohsen@.
> 
> Also can you please file a bug for this, containing a description of why what
> we're doing right now is wrong, and how it should be fixed. (Without a bug,
this
> will never get verified by TEs, and the history of the change will exist only
in
> git logs, essentially hiding it from all non-SWEs.)

I don't think TEs will be able to verify this because (a) visually it looks the
same and (b) I don't actually have a test case for them. I had to add debug code
to make it show up.

[mohsen, 2017-03-03 04:25:35]

LGTM

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
ash/common/system/web_notification/web_notification_tray.cc:272:
base::i18n::WrapStringWithRTLFormatting(&str);
On 2017/03/02 at 22:22:52, Evan Stade wrote:
> On 2017/03/02 21:45:21, mohsen wrote:
> > If I understand correctly, when the context is RTL, we still want to show
"+n"
> > text LTR (as mathematical expressions are written LTR in RTL languages). In
this
> > case, shouldn't we use WrapStringWithLTRFormatting (note the LTR) to force
"+n"
> > written LTR?
> 
> We want to show "99+" in RTL. That is what Chrome currently does, this just
effects that more gracefully. You can go type plus nine nine in a hebrew web
page and see that on screen it looks like 99+.
> 
> This text is put in a label. Labels intuit their directionality from the text,
not the UI (i.e. they don't care about i18n::IsRTL()). "+99" on its own has no
strong directional characters. Thus we'd fall back to a default of LTR for
displaying it. Wrapping with the RTL indicating character simply makes us use
RTL to display "+99", which ends up looking like "99+" on screen.
> 
> The only thing effectively incorrect about the present code is what the output
of a screen reader would be.


I see. I agree that your change is a better way of implementing the current
behavior. However, that behavior seems wrong to me. At least in Persian (and I
believe in other RTL languages) "99+" and "+99" (with Persian digits) mean the
same as in English. But, I also realize that this design for notification tray,
which works perfectly in LTR (e.g. "😁 +3" means a notification (😁) plus three
more), does not work well for RTL languages (none of "+3 😁" and "3+ 😁" seem
good, though I prefer the former). Anyways, I'll file a bug about this and see
what people think.

[Evan Stade, 2017-03-03 22:33:22]

On 2017/03/03 04:25:35, mohsen wrote:
> LGTM
> 
>
https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
> File ash/common/system/web_notification/web_notification_tray.cc (right):
> 
>
https://codereview.chromium.org/2721213002/diff/1/ash/common/system/web_notif...
> ash/common/system/web_notification/web_notification_tray.cc:272:
> base::i18n::WrapStringWithRTLFormatting(&str);
> On 2017/03/02 at 22:22:52, Evan Stade wrote:
> > On 2017/03/02 21:45:21, mohsen wrote:
> > > If I understand correctly, when the context is RTL, we still want to show
> "+n"
> > > text LTR (as mathematical expressions are written LTR in RTL languages).
In
> this
> > > case, shouldn't we use WrapStringWithLTRFormatting (note the LTR) to force
> "+n"
> > > written LTR?
> > 
> > We want to show "99+" in RTL. That is what Chrome currently does, this just
> effects that more gracefully. You can go type plus nine nine in a hebrew web
> page and see that on screen it looks like 99+.
> > 
> > This text is put in a label. Labels intuit their directionality from the
text,
> not the UI (i.e. they don't care about i18n::IsRTL()). "+99" on its own has no
> strong directional characters. Thus we'd fall back to a default of LTR for
> displaying it. Wrapping with the RTL indicating character simply makes us use
> RTL to display "+99", which ends up looking like "99+" on screen.
> > 
> > The only thing effectively incorrect about the present code is what the
output
> of a screen reader would be.
> 
> 
> I see. I agree that your change is a better way of implementing the current
> behavior. However, that behavior seems wrong to me. At least in Persian (and I
> believe in other RTL languages) "99+" and "+99" (with Persian digits) mean the
> same as in English. But, I also realize that this design for notification
tray,
> which works perfectly in LTR (e.g. "😁 +3" means a notification (😁) plus three
> more), does not work well for RTL languages (none of "+3 😁" and "3+ 😁" seem
> good, though I prefer the former). Anyways, I'll file a bug about this and see
> what people think.

thanks. I had just assumed based on the code that we were doing was intentional
and "correct" (as it seems pretty unlikely we'd write the code this way without
specific intentionality), but I will defer to those who know better if the
outcome of that discussion warrants changing the behavior. Honestly I still
don't know how to make it come up IRL.

[Evan Stade, 2017-03-06 20:38:46]

estade@chromium.org changed reviewers:
+ derat@chromium.org
- tdanderson@chromium.org

[Evan Stade, 2017-03-06 20:38:47]

+derat for OWNERS

[Daniel Erat, 2017-03-06 20:45:31]

lgtm as owner

[Evan Stade, 2017-03-06 20:59:26]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:00:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:33:58]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488833966127100, "parent_rev":
"762276ecdfbff938d3b7431ad0ed00c3ab6a4136", "commit_rev":
"ccb33d40c407ae11f528895dc7ceeb0d558ad290"}

[commit-bot: I haz the power, 2017-03-06 21:34:40]

Description was changed from

==========
Adjust RTL formatting for web notifications in cros system tray.

BUG=none
==========

to

==========
Adjust RTL formatting for web notifications in cros system tray.

BUG=none

Review-Url: https://codereview.chromium.org/2721213002
Cr-Commit-Position: refs/heads/master@{#454963}
Committed:
https://chromium.googlesource.com/chromium/src/+/ccb33d40c407ae11f528895dc7ce...
==========

[commit-bot: I haz the power, 2017-03-06 21:34:41]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ccb33d40c407ae11f528895dc7ce...