Issue metadata
Sign in to add a comment
|
Regression:Unwanted white line is seen after scrolling the page at L.H.S of header section in "www.bcci.tv"
Reported by
adha...@etouch.net,
Nov 8 2016
|
||||||||||||||||||||||
Issue descriptionChrome Version: 56.0.2912.0 Revision 817f8b49ade74a199d0c83d11a7befb89e1a0ba5-refs/heads/master@{#430205} OS: Windows(7,8,10), Mac(10.10.5,10.11.4),Linux(14.04 LTS) TEST URL:http://www.bcci.tv/ What steps will reproduce the problem? (1)Launch chrome and navigate to the above url. (2)Scroll the page up and down a few times.(Kindly refer the video) (3)Observe the L.H.S of header section. Actual:Unwanted white line is seen after scrolling the page under 'BCCI' at L.H.S of header section. Expected:No such Unwanted white line should be seen after scrolling the page. This is a Regression issue broken in M-56,will soon update other info. Good build:56.0.2909.0 Bad build:56.0.2911.0 Kindly find below the attached screen shot for reference.
,
Nov 8 2016
Unable to reproduce this on Mac with Chrome 56.0.2913.0 Changing component to Blink>Paint. If this is wrong, please update the component.
,
Nov 8 2016
Can't repro on Windows 2913 either. https://bugs.chromium.org/p/chromium/issues/detail?id=663254 also reproduced on 2912 but not 2913. Maybe some change fixed both. Checking.
,
Nov 10 2016
I can in fact reproduce this by scrolling the main content up and down slowly, and noting the white line appear at random scroll positions. The header element is fixed position, and it appears to be moving up or down by 1 pixel depending on the scroll position of the thing underneath it. As far as I can tell there is no dynamic CSS changes when the white line appears or disappears. This leads me to conclude that there is some computation that uses the scroll position in computing the fixed position element's position and that the calculation is unstable.
,
Nov 10 2016
Still can't repro on linux. Comment #4 was Win.
,
Nov 11 2016
,
Nov 14 2016
Still able to reproduce the issue on windows 7 using chrome version 56.0.2918.0.Please find the attached screen shot for the same. flackr@ Could you please look into this issue.
,
Nov 14 2016
Looks like the left header is promoted as a result of my patch (it's opaque), and consistently at a vertical offset of 60px. The top header however is not promoted because it's partially translucent, depending on the scroll offset it sometimes is only painted with a height of 59px and sometimes a height of 60px. It is also occasionally promoted when some promoted content needs to scroll beneath it. I'll look into why it's occasionally only painted 59px tall, but it seems to be an existing bug which is now visible due to promoting the left header which previously was not promoted.
,
Nov 14 2016
As an example, I was able to reproduce on this test page I created mimicking the top and left headers on that site: http://output.jsbin.com/somobu/quiet
,
Nov 15 2016
On further investigation, we seem to be painting the layer as if we are scrolled to one position (in my example scrolled to the top - (0, 0)), and then positing the layer in the property tree update at a different position (as if we're scrolled to (1, 0)), leading to the one pixel gap.
,
Nov 16 2016
I found out what's causing this, in cc we scroll by fractional amounts, then when we sync that scroll offset back to main we "truncate" the scroll delta because blink requires integer scroll deltas in ScrollTree::PullDeltaForMainThread: https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?dr=C&q=PullDeltaForMainThread&sq=package:chromium&l=1230 The problem currently is that with a tiny negative scroll (i.e. (0, -0.0001)) we will send a scroll delta of (0, -1) to blink but as far as cc is concerned the scrolling layer still snaps as if there was no scroll. blink then repaints the non-promoted fixed position objects at the new scroll offset only to have them positioned at the old scroll offset. Unfortunately, we can't just floor towards zero because the opposite can happen as well - the scroll delta can put us past the snapping point causing a visible scroll on cc where flooring would send a zero scroll delta back to main. We need to send the snapped scroll delta.
,
Nov 16 2016
I put up https://codereview.chromium.org/2511473003 which rounds to the nearest integer scroll offset. This should be correct in the common case (I can't reproduce the bug with this patch) and in non-integer snapping cases we would have already been incorrect.
,
Nov 22 2016
,
Nov 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75a7e4cb1ec29ded2e52539259233356caa883c8 commit 75a7e4cb1ec29ded2e52539259233356caa883c8 Author: flackr <flackr@chromium.org> Date: Tue Nov 22 23:39:33 2016 Round the scroll offset synced back to main instead of flooring. When scrolling up, flooring the scroll delta sent back to main can result in main thinking that we have scrolled up by a pixel while cc still positions layers as if we are at the old scroll position. To be technically correct according to the frame that will be produced, we would need to round according to the current scroll snap size but this isn ot easily known at the time of rounding so we use the nearest integer since this is the common case. BUG= 663291 TEST=LayerTreeHostImplTest.SyncSubpixelScrollDelta CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2511473003 Cr-Commit-Position: refs/heads/master@{#434022} [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/cc/trees/property_tree.cc [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/third_party/WebKit/LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added-expected.txt [modify] https://crrev.com/75a7e4cb1ec29ded2e52539259233356caa883c8/third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html
,
Nov 23 2016
Thanks for fix. adharap@ Please verify in todays canary. flackr@ Please request a merge to M56 if all looks good. It would be great to have everything set before Dev RC build cut @3 PM Monday 11/28.
,
Nov 25 2016
It seems to be working correctly for me in the latest canary - 57.0.2931.0 (Official Build) canary (64-bit) on Windows 7.
,
Nov 25 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Nov 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec4538615b56748d5cf39ce63ddefe40937211f9 commit ec4538615b56748d5cf39ce63ddefe40937211f9 Author: Robert Flack <flackr@chromium.org> Date: Fri Nov 25 22:02:36 2016 Round the scroll offset synced back to main instead of flooring. When scrolling up, flooring the scroll delta sent back to main can result in main thinking that we have scrolled up by a pixel while cc still positions layers as if we are at the old scroll position. To be technically correct according to the frame that will be produced, we would need to round according to the current scroll snap size but this isn ot easily known at the time of rounding so we use the nearest integer since this is the common case. BUG= 663291 TEST=LayerTreeHostImplTest.SyncSubpixelScrollDelta CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2511473003 Cr-Commit-Position: refs/heads/master@{#434022} (cherry picked from commit 75a7e4cb1ec29ded2e52539259233356caa883c8) Review URL: https://codereview.chromium.org/2529013003 . Cr-Commit-Position: refs/branch-heads/2924@{#100} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/cc/trees/property_tree.cc [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/third_party/WebKit/LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added-expected.txt [modify] https://crrev.com/ec4538615b56748d5cf39ce63ddefe40937211f9/third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html
,
Nov 25 2016
,
Nov 29 2016
Tested in chrome dev #56.0.2924.10 on Windows 10 & Mac 10.11.6 and observed that the issue is still reproducible in Mac 10.11.6. 1.Unwanted white line is displaying after scrolling the page. Unable to capture the blink in screen cast.Please let me know if i have missed anything. Thank you!
,
Nov 29 2016
Attaching Screen recording for the same. Thank you!
,
Nov 29 2016
I think the issue you are reproducing is a different issue, can you reproduce the persistent single pixel white line as in the original bug screenshot? The flicker in your video seems to be either an issue when we promote / depromote the header or due to the main thread animation.
,
Nov 30 2016
,
Nov 30 2016
Tested in chrome dev #56.0.2924.10 on Windows 10 & Mac 10.11.6 as per comment#22 with following steps mentioned in the original comment.Unwanted white line(Single pixel)is not displaying after scrolling the page.Please find the screen shot for reference.Hence adding TE-Verified label. @flacker: Can we raise the separate issue for flicker. Thank You!
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96f33ef6f7c21303a68dc52c3e27a925fb200ce5 commit 96f33ef6f7c21303a68dc52c3e27a925fb200ce5 Author: flackr <flackr@chromium.org> Date: Fri Jan 13 22:05:44 2017 Revert "Round the scroll offset synced back to main instead of flooring." and disable CompositeOpaqueFixedPosition Reverts the commit 75a7e4cb1ec29ded2e52539259233356caa883c8 (https://codereview.chromium.org/2511473003) and additionally disables CompositeOpaquePosition by default as that commit fixed a compositing bug exposed by compositing more fixed position elements ( https://crbug.com/663291 ). Rounding the synced scroll offset introduced a regression which caused scroll position to become unstable when zoomed in on long sites. TBR=weiliangc,boliu BUG= 677686 , 663291 , 661754 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2629793003 Cr-Commit-Position: refs/heads/master@{#443691} [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/cc/trees/property_tree.cc [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/content/child/runtime_features.cc [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/content/public/common/content_features.cc [modify] https://crrev.com/96f33ef6f7c21303a68dc52c3e27a925fb200ce5/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
,
Jan 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0e7600a8072b74faaccbddd2d90fd8130c5fc67 commit b0e7600a8072b74faaccbddd2d90fd8130c5fc67 Author: Alex Mineer <amineer@chromium.org> Date: Wed Jan 18 02:11:36 2017 Revert "Round the scroll offset synced back to main instead of flooring." and disable CompositeOpaqueFixedPosition Reverts the commit 75a7e4cb1ec29ded2e52539259233356caa883c8 (https://codereview.chromium.org/2511473003) and additionally disables CompositeOpaquePosition by default as that commit fixed a compositing bug exposed by compositing more fixed position elements ( https://crbug.com/663291 ). Rounding the synced scroll offset introduced a regression which caused scroll position to become unstable when zoomed in on long sites. TBR=weiliangc,boliu BUG= 677686 , 663291 , 661754 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel (cherry picked from commit 96f33ef6f7c21303a68dc52c3e27a925fb200ce5) Review-Url: https://codereview.chromium.org/2629793003 Cr-Original-Commit-Position: refs/heads/master@{#443691} Cr-Commit-Position: refs/branch-heads/2924@{#791} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/cc/trees/property_tree.cc [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/content/child/runtime_features.cc [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/content/public/common/content_features.cc [modify] https://crrev.com/b0e7600a8072b74faaccbddd2d90fd8130c5fc67/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
,
Apr 21 2017
At the ends of the scroll a nearly-white line can appear. This happens when device pixels are not multiples of logical pixels, (e.g. scaling of 91% when emulating an ipad pro on some random (mine) linux desktop.) This seems to happen when the top layer owns the last device pixel of the top bar (size comes to x.54 to x.63 of the pixel) but that pixel is only partially covered by the colored bar. The layer below does not contribute the rest of the color, rather a background sample does. This causes a very similar visual effect - a thin light line between the sections.
,
Apr 27 2017
Not a P1 level problem because this does not affect functionality. Removing milestones.
,
Nov 23 2017
,
Dec 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3ce2d3110a3f9177a41935daadd43ede72db553 commit a3ce2d3110a3f9177a41935daadd43ede72db553 Author: Yi Gu <yigu@chromium.org> Date: Tue Dec 05 15:49:54 2017 Round the scroll offset synced back to main thread instead of flooring Enabling feature CompositeOpaqueFixedPosition revealed crbug.com/663291 that promoted fixed opaque element indirectly caused a one pixel gap due to the way cc synced offset back to main. flackr@ fixed it by rounding the scroll offset instead of flooring it but the fix later on caused a regression crbug.com/677686 . So he reverted the fix as well as the feature CompositeOpaqueFixedPosition in Jan 2017 (https://codereview.chromium.org/2629793003). This patch relands flackr's revert patch. Meanwhile, it fixes a rounding bug which causes fractional delta pushed to main thread by adjusting the delta with the diff between active_base_ and its rounded value. Bug: 663291 , 677686 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: I9b7454760dc976b438ecf76c92c846f7086b6284 Reviewed-on: https://chromium-review.googlesource.com/793973 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Yi Gu <yigu@chromium.org> Cr-Commit-Position: refs/heads/master@{#521704} [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/cc/trees/layer_tree_host_unittest_scroll.cc [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/cc/trees/property_tree.cc [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/content/child/runtime_features.cc [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/content/public/common/content_features.cc [modify] https://crrev.com/a3ce2d3110a3f9177a41935daadd43ede72db553/third_party/WebKit/Source/platform/runtime_enabled_features.json5
,
Dec 5 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by msrchandra@chromium.org
, Nov 8 2016Owner: flackr@chromium.org
Status: Assigned (was: Unconfirmed)