Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1812303002:
Added Layer based clipping to MenuScrollViewContainer::MenuScrollView (Closed)
DescriptionAdded Layer based clipping to MenuScrollViewContainer::MenuScrollView
ToolbarActionViews now own their own Layers so that they can host the
material design ripple. The icons were previously clipped via the
clipping bounds set on the Canvas passed through the OnPaint() methods.
This clipping mechanism no longer works because the icons are painted
to their own Layers. Thus this CL updates the MenuScrollView to perform
Layer based clipping.
BUG=596073
TEST=manual
Committed: https://crrev.com/4aa8a6f3be2aafda269f33e9d88a44083210b71b
Cr-Commit-Position: refs/heads/master@{#386802}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Replaced SetFillsBoundsOpaquely(false) with a solid background. #Patch Set 3 : Added TODO with reference to bug. #
Messages
Total messages: 25 (9 generated)
bruthig@chromium.org changed reviewers: + sky@chromium.org
sky@, can you take a peek?
Description was changed from ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 ========== to ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 TEST=manual ==========
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:141: layer()->SetFillsBoundsOpaquely(false); is this correct/necessary?
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:141: layer()->SetFillsBoundsOpaquely(false); On 2016/03/18 19:07:02, Evan Stade wrote: > is this correct/necessary? Yeah it's necessary. Without it the child menu items paint but never 'erase' themselves when the View is scrolled. So you end up with severe ghosting. Alternatively I could omit this and explicitly set a solid background color.
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:141: layer()->SetFillsBoundsOpaquely(false); On 2016/03/18 19:26:43, bruthig wrote: > On 2016/03/18 19:07:02, Evan Stade wrote: > > is this correct/necessary? > > Yeah it's necessary. Without it the child menu items paint but never 'erase' > themselves when the View is scrolled. So you end up with severe ghosting. > Alternatively I could omit this and explicitly set a solid background color. that seems better as opaquely filling bounds allows for perf optimizations. Should be easy to get the right color from the native theme.
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:139: SetPaintToLayer(true); I wonder if this should be an optional behavior triggered by the caller. This way we don't have to worry about perf with an expense of having to worry about each individual scenario. This way app menu that knows that it has ink-rich items would trigger this mask layer creation but other MenuScrollViews would not need to. However what you have here is certainly safer.
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:139: SetPaintToLayer(true); On 2016/03/22 14:30:01, varkha wrote: > I wonder if this should be an optional behavior triggered by the caller. This > way we don't have to worry about perf with an expense of having to worry about > each individual scenario. This way app menu that knows that it has ink-rich > items would trigger this mask layer creation but other MenuScrollViews would not > need to. > However what you have here is certainly safer. I will consider this once I have the other issues sorted out. https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:141: layer()->SetFillsBoundsOpaquely(false); On 2016/03/18 19:34:40, Evan Stade wrote: > On 2016/03/18 19:26:43, bruthig wrote: > > On 2016/03/18 19:07:02, Evan Stade wrote: > > > is this correct/necessary? > > > > Yeah it's necessary. Without it the child menu items paint but never 'erase' > > themselves when the View is scrolled. So you end up with severe ghosting. > > Alternatively I could omit this and explicitly set a solid background color. > > that seems better as opaquely filling bounds allows for perf optimizations. > Should be easy to get the right color from the native theme. So I've played around with this for 2 days now and when I replace the SetFillsBoundsOpaquely(false) with a solid background (see patch set 2) there are issues with drawing menus on ChromeOS with a resolution of 683x384. See screenshot here: https://drive.google.com/a/google.com/file/d/0B9S3QZqpjnG7Sm1WN2hOUzVNMTg/vie... For some reason there is a very thin black line near the bottom and on the right side that should not be there. The one on the right side is very difficult to see. Has anyone ever seen this before? I wasn't sure if this was a manifestation of https://bugs.chromium.org/p/chromium/issues/detail?id=133477 and/or a limitation of Skia.
Adding pkasting@ as a shot in the dark. Peter have you ever seen anything like what I described here: https://codereview.chromium.org/1812303002/#msg10 ?
On 2016/03/22 21:48:36, bruthig wrote: > Adding pkasting@ as a shot in the dark. > Peter have you ever seen anything like what I described here: > https://codereview.chromium.org/1812303002/#msg10 ? Looks like a scaling artifact. Were you running at a non-1x scale? I'd check that everything is positioned and sized correctly at whatever scale factor this was.
LGTM
The CQ bit was checked by bruthig@chromium.org
https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:139: SetPaintToLayer(true); On 2016/03/22 21:45:29, bruthig wrote: > On 2016/03/22 14:30:01, varkha wrote: > > I wonder if this should be an optional behavior triggered by the caller. This > > way we don't have to worry about perf with an expense of having to worry about > > each individual scenario. This way app menu that knows that it has ink-rich > > items would trigger this mask layer creation but other MenuScrollViews would > not > > need to. > > However what you have here is certainly safer. > > I will consider this once I have the other issues sorted out. It is not straightforward for the owner of the menu to configure the MenuScrollView. However, I'm not sure making it configurable is a win, especially if we plan to add ripples to all menu items. https://codereview.chromium.org/1812303002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_scroll_view_container.cc:141: layer()->SetFillsBoundsOpaquely(false); On 2016/03/22 21:45:29, bruthig wrote: > On 2016/03/18 19:34:40, Evan Stade wrote: > > On 2016/03/18 19:26:43, bruthig wrote: > > > On 2016/03/18 19:07:02, Evan Stade wrote: > > > > is this correct/necessary? > > > > > > Yeah it's necessary. Without it the child menu items paint but never > 'erase' > > > themselves when the View is scrolled. So you end up with severe ghosting. > > > Alternatively I could omit this and explicitly set a solid background color. > > > > that seems better as opaquely filling bounds allows for perf optimizations. > > Should be easy to get the right color from the native theme. > > So I've played around with this for 2 days now and when I replace the > SetFillsBoundsOpaquely(false) with a solid background (see patch set 2) there > are issues with drawing menus on ChromeOS with a resolution of 683x384. See > screenshot here: > https://drive.google.com/a/google.com/file/d/0B9S3QZqpjnG7Sm1WN2hOUzVNMTg/vie... > > For some reason there is a very thin black line near the bottom and on the right > side that should not be there. The one on the right side is very difficult to > see. Has anyone ever seen this before? I wasn't sure if this was a > manifestation of https://bugs.chromium.org/p/chromium/issues/detail?id=133477 > and/or a limitation of Skia. It has turned in to a bit of a rabbit hole trying to fill the bounds opaquely. I'm going to get this in as-is for now and track that work with http://crbug.com/601135.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812303002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812303002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1812303002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1812303002/40001
Message was sent while issue was closed.
Description was changed from ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 TEST=manual ========== to ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 TEST=manual ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 TEST=manual ========== to ========== Added Layer based clipping to MenuScrollViewContainer::MenuScrollView ToolbarActionViews now own their own Layers so that they can host the material design ripple. The icons were previously clipped via the clipping bounds set on the Canvas passed through the OnPaint() methods. This clipping mechanism no longer works because the icons are painted to their own Layers. Thus this CL updates the MenuScrollView to perform Layer based clipping. BUG=596073 TEST=manual Committed: https://crrev.com/4aa8a6f3be2aafda269f33e9d88a44083210b71b Cr-Commit-Position: refs/heads/master@{#386802} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4aa8a6f3be2aafda269f33e9d88a44083210b71b Cr-Commit-Position: refs/heads/master@{#386802}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1882143004/ by bruthig@chromium.org. The reason for reverting is: Reverting due to http://crbug.com/602913. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
