New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740813 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 726241



Sign in to add a comment

Adjust CAPS LOCK is ON notification layout in new-style notification

Project Member Reported by tetsui@chromium.org, Jul 11 2017

Issue description

CAPS LOCK is ON notification layout needs adjustment in new-style notification.
* The notification is not using title.
  When it shows up, the second line is ellided, which is not convenient.
* "ChromeOS system" is not shown in the header.
  (To be precise, this is still not in new mock, but I think it's OK to implement it and change later if the mock is changed.)
* The icon is now shown in the header, but shown on the right side.
 
VLzh2mJz3qw.png
16.5 KB View Download
qku2V1khv1x.png
18.1 KB View Download

Comment 1 by tetsui@chromium.org, Jul 11 2017

Blockedon: 726241
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d38d8c91dae426f0bb1099233e21e798fd9909ab

commit d38d8c91dae426f0bb1099233e21e798fd9909ab
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Fri Jul 14 05:12:31 2017

Use the title for "CAPS LOCK is ON" notification.

Use the title text instead of multi line message for "CAPS LOCK is ON"
notification.
Primarily fix for MD style notification but also works properly in
the current style.

BUG= 740813 
TEST=manual

Change-Id: I9121ae49fcf77fa4e7092d4a6a354b1d1a21d04e
Reviewed-on: https://chromium-review.googlesource.com/569329
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Terry Anderson <tdanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486685}
[modify] https://crrev.com/d38d8c91dae426f0bb1099233e21e798fd9909ab/ash/ash_strings.grd
[modify] https://crrev.com/d38d8c91dae426f0bb1099233e21e798fd9909ab/ash/system/tray_caps_lock.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/763959aadf1e04d49c0b997cda3d31296f63f172

commit 763959aadf1e04d49c0b997cda3d31296f63f172
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Thu Jul 20 10:06:04 2017

Add header UI string that will be used in MD notification.

Add "Chrome OS system" string that will be used for system notification
when new-style notification is enabled.

BUG=738779, 726241 , 740807 , 740813 
TEST=manual

Change-Id: I2e4ce586385065752abcc4e38445b7e67c6bb2ef
Reviewed-on: https://chromium-review.googlesource.com/578696
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488182}
[modify] https://crrev.com/763959aadf1e04d49c0b997cda3d31296f63f172/ui/strings/ui_strings.grd

Comment 4 by tetsui@chromium.org, Jul 21 2017

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01f38b46d3446cc7ae3f482d59dbc1841f625479

commit 01f38b46d3446cc7ae3f482d59dbc1841f625479
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Thu Aug 03 01:16:26 2017

Add first-party notification styling in MD notification.

In new-style notification, first-party notification should have
different styling compared to general web notifications.
To do this, all system notifications should be created by
new CreateSystemNotification. (Previously, it was not a requirement.)

By using the function,
* the display source defaults to string "ChromeOS system".
* the notification has single accent color based on
  SystemNotificationWarningLevel.
* gfx::VectorIcon will be passed as small_image instead of gfx::Image
  to perform icon coloring on CreateSystemNotification side.

TEST=out/Debug/message_center_unittests
BUG=738779, 726241 , 740807 , 740813 

Change-Id: I9e85b3f1de33b6499297f060b5305d0b02157eef
Reviewed-on: https://chromium-review.googlesource.com/566765
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491592}
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/BUILD.gn
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/fake_message_center.cc
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/fake_message_center.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/message_center.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/message_center_impl.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/message_center_style.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/notification.cc
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/notification.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/views/notification_header_view.cc
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/views/notification_header_view.h
[modify] https://crrev.com/01f38b46d3446cc7ae3f482d59dbc1841f625479/ui/message_center/views/notification_view_md.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e3561e0926074e531406ce0fb035e1e3a0d9c39

commit 9e3561e0926074e531406ce0fb035e1e3a0d9c39
Author: John Mellor <johnme@chromium.org>
Date: Thu Aug 03 11:10:58 2017

Revert "Add first-party notification styling in MD notification."

This reverts commit 01f38b46d3446cc7ae3f482d59dbc1841f625479.

Reason for revert: Sorry, this broke compilation on 3 bots:
- https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builder%20%28dbg%29
- https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builder%20%28dbg%29
- https://build.chromium.org/p/chromium.android/builders/Android%20MIPS%20Builder%20%28dbg%29

With the following linker error:

obj/ui/message_center/message_center/notification.o: In function `message_center::Notification::CreateSystemNotification(...)':
../../ui/message_center/notification.cc:182: undefined reference to `gfx::kNoneIcon'
obj/ui/message_center/message_center/notification.o: In function `message_center::Notification::CreateSystemNotification(...)':
../../ui/message_center/notification.cc:232: undefined reference to `gfx::CreateVectorIcon(gfx::VectorIcon const&, unsigned int)'


Original change's description:
> Add first-party notification styling in MD notification.
> 
> In new-style notification, first-party notification should have
> different styling compared to general web notifications.
> To do this, all system notifications should be created by
> new CreateSystemNotification. (Previously, it was not a requirement.)
> 
> By using the function,
> * the display source defaults to string "ChromeOS system".
> * the notification has single accent color based on
>   SystemNotificationWarningLevel.
> * gfx::VectorIcon will be passed as small_image instead of gfx::Image
>   to perform icon coloring on CreateSystemNotification side.
> 
> TEST=out/Debug/message_center_unittests
> BUG=738779, 726241 , 740807 , 740813 
> 
> Change-Id: I9e85b3f1de33b6499297f060b5305d0b02157eef
> Reviewed-on: https://chromium-review.googlesource.com/566765
> Reviewed-by: James Cook <jamescook@chromium.org>
> Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491592}

TBR=jamescook@chromium.org,yoshiki@chromium.org,peter@chromium.org,fukino@chromium.org,tetsui@chromium.org

Change-Id: I10fe30dbdad6012fbafe32c17a439065f4d816d3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 738779,  726241 ,  740807 ,  740813 
Reviewed-on: https://chromium-review.googlesource.com/600067
Reviewed-by: John Mellor <johnme@chromium.org>
Commit-Queue: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491696}
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/BUILD.gn
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/fake_message_center.cc
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/fake_message_center.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/message_center.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/message_center_impl.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/message_center_style.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/notification.cc
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/notification.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/views/notification_header_view.cc
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/views/notification_header_view.h
[modify] https://crrev.com/9e3561e0926074e531406ce0fb035e1e3a0d9c39/ui/message_center/views/notification_view_md.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ead8b5b61dacc735f3840152ecc5a11c80c724da

commit ead8b5b61dacc735f3840152ecc5a11c80c724da
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Thu Aug 10 02:14:46 2017

Reland "Add first-party notification styling in MD notification."

This is a reland of 01f38b46d3446cc7ae3f482d59dbc1841f625479

Fix from the original CL:
* Link gfx::VectorIcon and gfx::PaintVectorIcon on Android build,
  as they are now used in message_center::Notification, which is
  common for all the platforms that uses ui/message_center.

Original change's description:
> Add first-party notification styling in MD notification.
> 
> In new-style notification, first-party notification should have
> different styling compared to general web notifications.
> To do this, all system notifications should be created by
> new CreateSystemNotification. (Previously, it was not a requirement.)
> 
> By using the function,
> * the display source defaults to string "ChromeOS system".
> * the notification has single accent color based on
>   SystemNotificationWarningLevel.
> * gfx::VectorIcon will be passed as small_image instead of gfx::Image
>   to perform icon coloring on CreateSystemNotification side.
> 
> TEST=out/Debug/message_center_unittests
> BUG=738779, 726241 , 740807 , 740813 
> 
> Change-Id: I9e85b3f1de33b6499297f060b5305d0b02157eef
> Reviewed-on: https://chromium-review.googlesource.com/566765
> Reviewed-by: James Cook <jamescook@chromium.org>
> Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491592}

Bug: 738779,  726241 ,  740807 ,  740813 
Change-Id: I62fa4644347ce62d629f3218d319b09ac74db097
Reviewed-on: https://chromium-review.googlesource.com/601611
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493228}
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/gfx/BUILD.gn
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/BUILD.gn
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/fake_message_center.cc
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/fake_message_center.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/message_center.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/message_center_impl.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/message_center_style.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/notification.cc
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/notification.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/views/notification_header_view.cc
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/views/notification_header_view.h
[modify] https://crrev.com/ead8b5b61dacc735f3840152ecc5a11c80c724da/ui/message_center/views/notification_view_md.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce10182a51b8d4c9d4b99be26a462ca2683081b0

commit ce10182a51b8d4c9d4b99be26a462ca2683081b0
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Tue Aug 22 07:16:56 2017

Apply system style to "CAPS LOCK is ON" notification.

In new-style notification, system notification has different style from
other notifications such as web notifications.
* The context header shows the title "Chrome OS system".
* The context header has different color.
* New MD icon is used.
This CL applies the system style to "CAPS LOCK is ON" notification.

BUG= 740813 , 751024 
TEST=manual

Change-Id: I61f6909224eb4a4f10830b636c1f5be27f4a0117
Reviewed-on: https://chromium-review.googlesource.com/598751
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496239}
[modify] https://crrev.com/ce10182a51b8d4c9d4b99be26a462ca2683081b0/ash/resources/vector_icons/BUILD.gn
[add] https://crrev.com/ce10182a51b8d4c9d4b99be26a462ca2683081b0/ash/resources/vector_icons/notification_capslock.1x.icon
[add] https://crrev.com/ce10182a51b8d4c9d4b99be26a462ca2683081b0/ash/resources/vector_icons/notification_capslock.icon
[modify] https://crrev.com/ce10182a51b8d4c9d4b99be26a462ca2683081b0/ash/system/tray_caps_lock.cc

Comment 9 by tetsui@chromium.org, Aug 22 2017

Status: Fixed (was: Started)

Comment 10 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment