Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398203002:
Linux Aura: Fix overlay shadows on notifications (Closed)
|
Created:
4 years, 2 months ago by Tom (Use chromium acct) Modified:
4 years ago CC:
chromium-reviews, Peter Beverloo, tfarina, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, Julien Isorce Samsung Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux Aura: Fix overlay shadows on notifications
Linux Aura got support for translucent windows in CL:
https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed640724e640fcf22fbbf87
Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in
or out, but that don't actually have an alpha mask. This was to support a
limitation on MS Windows where windows must be translucent to fade.
However, most Linux window managers only draw shadows on opaque windows: that is,
windows that do not have an alpha channel. Windows that fade in or out may still
have shadows since opacity is set as a property of the toplevel window.
Therefore, the solution is to use INFER_OPACITY for fading widgets so it will
work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that
have alpha masks.
BUG=640170
R=sky@chromium.org
Committed: https://crrev.com/e35682c33f0e363b498b04d816c86bb16d1f5014
Cr-Commit-Position: refs/heads/master@{#434745}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Only apply change on desktop Linux #
Messages
Total messages: 38 (20 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Description was changed from ========== Linux Aura: Fix overlay shadows on notifications BUG=640170 ========== to ========== Linux Aura: Fix overlay shadows on notifications BUG=640170 R=sky@chromium.org ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Linux Aura: Fix overlay shadows on notifications BUG=640170 R=sky@chromium.org ========== to ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ==========
thomasanderson@google.com changed reviewers: + sky@chromium.org
Description was changed from ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ========== to ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:226: GetWidget()->SetOpacity( I'm confused, this is changing the opacity. Doesn't that mean we want translucent?
On 2016/10/06 21:44:56, sky wrote: > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > File ui/message_center/views/toast_contents_view.cc (right): > > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > ui/message_center/views/toast_contents_view.cc:226: GetWidget()->SetOpacity( > I'm confused, this is changing the opacity. Doesn't that mean we want > translucent? My understanding is that translucent windows have non-uniform alpha masks, like a .png with transparency. SetOpacity() is a multiplier for the alpha channel, so you can make the entire window 50% translucent for example.
https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:62: // Sets the transparent background. Then, when the message view is slid out, I'm not familiar enough with this code to know if it's also using a transparent background. Based on this comment it looks to me as though it is. Could you ask a more local owner?
thomasanderson@google.com changed reviewers: + dewittj@chromium.org
On 2016/10/07 15:21:09, sky wrote: > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > File ui/message_center/views/toast_contents_view.cc (right): > > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > ui/message_center/views/toast_contents_view.cc:62: // Sets the transparent > background. Then, when the message view is slid out, > I'm not familiar enough with this code to know if it's also using a transparent > background. Based on this comment it looks to me as though it is. Could you ask > a more local owner? +dewittj@chromium.org
dewittj@chromium.org changed reviewers: + mukai@chromium.org
https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:62: // Sets the transparent background. Then, when the message view is slid out, On 2016/10/07 15:21:08, sky wrote: > I'm not familiar enough with this code to know if it's also using a transparent > background. Based on this comment it looks to me as though it is. Could you ask > a more local owner? +mukai@, the original author Seems like we aren't actually using an alpha channel per se, but we are sliding out the content and relying on the fact that the background is transparent. Unfortunately I know too little about the mechanics to really know if that's what we want here. Can I recommend a little test using https://tests.peter.sh/notification-generator/ and sliding out the notification using your finger on a touch device? If this is about shadows on notifications, it appears that shadows are working on ToT.
On 2016/10/07 18:59:26, dewittj wrote: > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > File ui/message_center/views/toast_contents_view.cc (right): > > https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... > ui/message_center/views/toast_contents_view.cc:62: // Sets the transparent > background. Then, when the message view is slid out, > On 2016/10/07 15:21:08, sky wrote: > > I'm not familiar enough with this code to know if it's also using a > transparent > > background. Based on this comment it looks to me as though it is. Could you > ask > > a more local owner? > > +mukai@, the original author > > Seems like we aren't actually using an alpha channel per se, but we are sliding > out the content and relying on the fact that the background is transparent. > Unfortunately I know too little about the mechanics to really know if that's > what we want here. Can I recommend a little test using > https://tests.peter.sh/notification-generator/ and sliding out the notification > using your finger on a touch device? > > If this is about shadows on notifications, it appears that shadows are working > on ToT. It doesn't work for me using Compiz. Some folks on bug 640170 are also able to repro.
https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:62: // Sets the transparent background. Then, when the message view is slid out, On 2016/10/07 18:59:26, dewittj wrote: > On 2016/10/07 15:21:08, sky wrote: > > I'm not familiar enough with this code to know if it's also using a > transparent > > background. Based on this comment it looks to me as though it is. Could you > ask > > a more local owner? > > +mukai@, the original author > > Seems like we aren't actually using an alpha channel per se, but we are sliding > out the content and relying on the fact that the background is transparent. > Unfortunately I know too little about the mechanics to really know if that's > what we want here. Can I recommend a little test using > https://tests.peter.sh/notification-generator/ and sliding out the notification > using your finger on a touch device? > > If this is about shadows on notifications, it appears that shadows are working > on ToT. It's hard for me to remember everything... Sorry. Probably, the motivation is that the notification card (the content view) is a slide view (https://chromium.googlesource.com/chromium/src/+/master/ui/message_center/vie...) because we want the slide-out behavior even inside of the notification center. So the slide-out and touch event handling is within the content view, not here. In that case, when the user swipes finger on the notification popup, only the content view moves horizontally and fades out -- thus the popup widget itself has a transparent background to make the popup itself looks like slid out. https://codereview.chromium.org/2398203002/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:226: GetWidget()->SetOpacity( On 2016/10/06 21:44:56, sky wrote: > I'm confused, this is changing the opacity. Doesn't that mean we want > translucent? This is for normal show/disappear animation. The hack of translucent background above is for user's swipe handling. It uses normal animation and uses translucency as usual (possibly this should use normal corewm window animation though. When I wrote this, Windows was not yet Aura and couldn't rely on corewm).
I think this is ok to lgtm, but please do a sanity check on a touch device. Thanks!
On 2016/10/17 16:01:44, dewittj wrote: > I think this is ok to lgtm, but please do a sanity check on a touch device. > Thanks! I tested the slide-out animation and indeed it has a white background instead of a transparent one. However, maybe fixing shadows is more important than having transparency on slide-out for Linux? (Because a white notification on a white bg looks really bad) FYI transparency was missing before https://codereview.chromium.org/2347383002/ which landed a little over a month ago and enabled transparency on all HW, so this CL would revert to the old behavior (unless I'm missing something on CrOS?)
On 2016/10/26 18:41:45, Tom Anderson wrote: > On 2016/10/17 16:01:44, dewittj wrote: > > I think this is ok to lgtm, but please do a sanity check on a touch device. > > Thanks! > > I tested the slide-out animation and indeed it has a white background instead of > a transparent one. > > However, maybe fixing shadows is more important than having transparency on > slide-out for Linux? (Because a white notification on a white bg looks really > bad) > > FYI transparency was missing before https://codereview.chromium.org/2347383002/ > which landed a little over a month ago and enabled transparency on all HW, so > this CL would revert to the old behavior (unless I'm missing something on CrOS?) cros supports window level transparency.
On 2016/10/26 19:57:27, sky wrote: > On 2016/10/26 18:41:45, Tom Anderson wrote: > > On 2016/10/17 16:01:44, dewittj wrote: > > > I think this is ok to lgtm, but please do a sanity check on a touch device. > > > Thanks! > > > > I tested the slide-out animation and indeed it has a white background instead > of > > a transparent one. > > > > However, maybe fixing shadows is more important than having transparency on > > slide-out for Linux? (Because a white notification on a white bg looks really > > bad) > > > > FYI transparency was missing before > https://codereview.chromium.org/2347383002/ > > which landed a little over a month ago and enabled transparency on all HW, so > > this CL would revert to the old behavior (unless I'm missing something on > CrOS?) > > cros supports window level transparency. Maybe we can just add "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" and add a comment referencing this bug for the change in toast_contents_view.cc?
On 2016/10/26 23:27:40, Tom Anderson wrote: > On 2016/10/26 19:57:27, sky wrote: > > On 2016/10/26 18:41:45, Tom Anderson wrote: > > > On 2016/10/17 16:01:44, dewittj wrote: > > > > I think this is ok to lgtm, but please do a sanity check on a touch > device. > > > > Thanks! > > > > > > I tested the slide-out animation and indeed it has a white background > instead > > of > > > a transparent one. > > > > > > However, maybe fixing shadows is more important than having transparency on > > > slide-out for Linux? (Because a white notification on a white bg looks > really > > > bad) > > > > > > FYI transparency was missing before > > https://codereview.chromium.org/2347383002/ > > > which landed a little over a month ago and enabled transparency on all HW, > so > > > this CL would revert to the old behavior (unless I'm missing something on > > CrOS?) > > > > cros supports window level transparency. > > Maybe we can just add "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" and add a > comment referencing this bug for the change in toast_contents_view.cc? I don't know which is more annoying, the lack of a shadow of the lack of translucency. Can you create screenshots and ask UX?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/26 23:50:07, sky wrote: > On 2016/10/26 23:27:40, Tom Anderson wrote: > > On 2016/10/26 19:57:27, sky wrote: > > > On 2016/10/26 18:41:45, Tom Anderson wrote: > > > > On 2016/10/17 16:01:44, dewittj wrote: > > > > > I think this is ok to lgtm, but please do a sanity check on a touch > > device. > > > > > Thanks! > > > > > > > > I tested the slide-out animation and indeed it has a white background > > instead > > > of > > > > a transparent one. > > > > > > > > However, maybe fixing shadows is more important than having transparency > on > > > > slide-out for Linux? (Because a white notification on a white bg looks > > really > > > > bad) > > > > > > > > FYI transparency was missing before > > > https://codereview.chromium.org/2347383002/ > > > > which landed a little over a month ago and enabled transparency on all HW, > > so > > > > this CL would revert to the old behavior (unless I'm missing something on > > > CrOS?) > > > > > > cros supports window level transparency. > > > > Maybe we can just add "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" and add > a > > comment referencing this bug for the change in toast_contents_view.cc? > > I don't know which is more annoying, the lack of a shadow of the lack of > translucency. Can you create screenshots and ask UX? sky@ I have approval from rolfe@ from UX. Can you approve ui/views/widget/widget.h? We'll also need a merge to M56 and M55.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2398203002/#ps40001 (title: "Only apply change on desktop Linux")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480368230104690,
"parent_rev": "d92d59c7b298cd0739573142be3d667f5f069103", "commit_rev":
"61b437dd6e15c66b0171287ce3a9cfd92f73d31c"}
Message was sent while issue was closed.
Description was changed from ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ========== to ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org ========== to ========== Linux Aura: Fix overlay shadows on notifications Linux Aura got support for translucent windows in CL: https://chromium.googlesource.com/chromium/src.git/+/62ba78ffcdf525eb9ed64072... Some Chrome widgets used a TRANSLUCENT_WINDOW opacity for widgets that fade in or out, but that don't actually have an alpha mask. This was to support a limitation on MS Windows where windows must be translucent to fade. However, most Linux window managers only draw shadows on opaque windows: that is, windows that do not have an alpha channel. Windows that fade in or out may still have shadows since opacity is set as a property of the toplevel window. Therefore, the solution is to use INFER_OPACITY for fading widgets so it will work across platforms. TRANSLUCENT_WINDOW should only be used on widgets that have alpha masks. BUG=640170 R=sky@chromium.org Committed: https://crrev.com/e35682c33f0e363b498b04d816c86bb16d1f5014 Cr-Commit-Position: refs/heads/master@{#434745} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e35682c33f0e363b498b04d816c86bb16d1f5014 Cr-Commit-Position: refs/heads/master@{#434745} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
