New issue
Advanced search Search tips

Issue 856515 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression

Blocking:
issue 851530



Sign in to add a comment

ToolbarButton inkdrops masks are out of sync after their parent view resizes

Reported by rp...@etouch.net, Jun 26 2018

Issue description

Chrome version: 69.0.3473.0 (Official Build)Revision cd23eb9e98a965ea157e23052bb62553e5d520d5-refs/branch-heads/3473@{#1}(32/64-bit)
OS: Windows (7,8,8.1,10)

What steps will reproduce the problem?
1. Launch chrome,navigate to www.google.com and hover mouse on Back navigation arrow near omnibox,observe grey focus highlight
 
Actual: Grey focus highlight appears chopped from RHS 
Expected: Grey focus highlight should be seen properly

This is regression issue, broken in ‘M 69’ and will soon update other info:
Good build: 69.0.3472.0  (Revision: 569949).
Bad build: 69.0.3473.0 (Revision: 570288).


 
Actual_screenshot.png
82.3 KB View Download
Expected_screenshot.png
44.8 KB View Download

Comment 1 by rp...@etouch.net, Jun 26 2018

Labels: hasbisect OS-Linux
Owner: kylixrd@chromium.org
Status: Assigned (was: Unconfirmed)
You are probably looking for a change made after 570216 (known good), but no later than 570235 (first known bad).

Narrow Bisect info : 
https://chromium.googlesource.com/chromium/src/+log/3c16d9d466102e0839cad4e70cea850107143ca4..64c540a750df6adb453c0f8e6e63800d8948d04c?pretty=fuller&n=50

Suspecting: r570226 from Narrow bisect

@kylixrd: Could you please help to reassign if your change is not the cause for this change.

Note:
1.Unable to provide bisect using per-revision script,Hence providing bisect with old script.
2.Issue is not seen on Mac(10.12.6,10.13.1,10.13.6)OS

Cc: bsep@chromium.org kylixrd@chromium.org
Labels: Proj-MdRefresh
Owner: pbos@chromium.org

Comment 3 by pbos@chromium.org, Jun 26 2018

Labels: Needs-TestConfirmation
Can't repro on English / Windows 10 / 69.0.3473.0 or tip of tree. I'm setting no chrome://flags and am not using RTL.

Are there any additional steps I'm missing? Any other ideas?
inkdrop-no-repro.png
10.8 KB View Download

Comment 4 by pbos@chromium.org, Jun 26 2018

Labels: Needs-Feedback

Comment 5 by rp...@etouch.net, Jun 27 2018

Labels: -Needs-TestConfirmation -Needs-Feedback
With response to comment #3 & 4:
Retested this issue on Windows (7,8,8.1,10) & Linux OS using Canary build # 69.0.3473.0 and the issue is still reproducible.

Note : If you are unable to reproduce the issue on first instance then try using freshly launch chrome and perform the steps mentioned in the description.

Attaching the screen cast for the same.  

Thank you..!
69.0.3473.0 Behavior.mp4
256 KB View Download

Comment 6 by bsep@chromium.org, Jun 27 2018

I managed to reproduce it:
1. Open chrome restored
2. Hover the back button (the ink drop will be correct)
3. Maximize chrome
4. Hover the back button (the ink drop will be cut off)

Unsure if this is the same root cause as  bug 856510  because in debug mode going back and forth between maxmized/restored hits the DCHECK in layer.cc:1239...

Comment 7 by pbos@chromium.org, Jun 28 2018

Status: Started (was: Assigned)
Summary: ToolbarButton inkdrops masks are out of sync after their parent view resizes (was: Regression : Grey focus highlight appears chopped from RHS on Back navigation arrow near omnibox.)
Thanks that's super helpful. Not same root cause as the other bug (which was just RTL flipping not working properly for inkdrops). The inkdrop layer is not resized with its mask:

[26416:28044:0627/171406.271:FATAL:layer.cc(1239)] Check failed: bounds().ToString() == mask_layer()->bounds().ToString() (36x28 vs. 28x28)

Relevant part of stack (though too generic to make sense of):

 	cc.dll!cc::Layer::PushPropertiesTo(cc::LayerImpl * layer) Line 1239	C++	Symbols loaded.
 	cc.dll!cc::PictureLayer::PushPropertiesTo(cc::LayerImpl * base_layer) Line 52	C++	Symbols loaded.
 	cc.dll!cc::PushLayerPropertiesInternal<cc::Layer>(std::unordered_set<cc::Layer *,std::hash<cc::Layer *>,std::equal_to<cc::Layer *>,std::allocator<cc::Layer *> > layers_that_should_push_properties, cc::LayerTreeImpl * impl_tree) Line 127	C++	Symbols loaded.
 	cc.dll!cc::TreeSynchronizer::PushLayerProperties(cc::LayerTreeHost * host_tree, cc::LayerTreeImpl * impl_tree) Line 144	C++	Symbols loaded.
 	cc.dll!cc::LayerTreeHost::FinishCommitOnImplThread(cc::LayerTreeHostImpl * host_impl) Line 328	C++	Symbols loaded.
 	cc.dll!cc::SingleThreadProxy::DoCommit() Line 199	C++	Symbols loaded.
 	cc.dll!cc::SingleThreadProxy::ScheduledActionCommit() Line 809	C++	Symbols loaded.
 	cc.dll!cc::Scheduler::ProcessScheduledActions() Line 742	C++	Symbols loaded.
 	cc.dll!cc::Scheduler::NotifyReadyToCommit() Line 168	C++	Symbols loaded.
 	cc.dll!cc::SingleThreadProxy::DoPainting() Line 780	C++	Symbols loaded.

This probably doesn't happen pre-Refresh as the default inkdrop doesn't have a mask. Hypothesizing that the inkdrop mask has to be recreated on inkdrop resize. More generally thing is Too Dang Hard to use.

Comment 8 by pbos@chromium.org, Jun 28 2018

Issue 857411 has been merged into this issue.

Comment 9 by pbos@chromium.org, Jun 28 2018

Blocking: 851530
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 28 2018

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

commit 49ac4812986f8d49ba458461610dd59ebe80b493
Author: Peter Boström <pbos@chromium.org>
Date: Thu Jun 28 20:01:03 2018

Inform ink-drop of ToolbarButton bounds changes

Fixes bug (and DCHECK) when the leading ToolbarButton (back) switches
between maximized and and unmaximized windows. Maximizing the window
adds leading margin to the view which changes its bounds and size.

ToolbarButton overrides OnBoundsChanged but did not call the parent
OnBoundsChanged. Therefore InkDropHostView did not resize the inkdrop
and mask. This caused clipping in release and DCHECKs in debug due to
layer/mask size mismatches.

Bug:  chromium:856515 , chromium:857411
Change-Id: I6348b2cdef11a95e5800d337d60f5060b93c4d99
Reviewed-on: https://chromium-review.googlesource.com/1118972
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571236}
[modify] https://crrev.com/49ac4812986f8d49ba458461610dd59ebe80b493/chrome/browser/ui/views/toolbar/toolbar_button.cc

Comment 11 by pbos@chromium.org, Jun 28 2018

Status: Fixed (was: Started)
Should be fixed in next Canary, please verify.

Comment 12 by rp...@etouch.net, Jun 29 2018

Labels: TE-Verified-69.0.3476.0 TE-Verified-M69
Update:-
Re-tested this issue on Windows (7,8,8.1,10) and Linux(14.04 LTS) machines using latest Chrome Canary build# 69.0.3476.0 and fix is working as expected.. Hence adding TE-Verified labels. 
Kindly refer attached screen cast for your reference.
Fixed_behavior.mp4
193 KB View Download
Labels: -M-69 Group-Toolbar

Sign in to add a comment