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

Issue 663291 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Mac
Pri: 2
Type: Bug-Regression



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 description

Chrome 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.

 
Actual.png
809 KB View Download
Actual result.mp4
1.7 MB View Download
Expected result.mp4
1.9 MB View Download
Labels: hasbisect-per-revision
Owner: flackr@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good build: 56.0.2909.0 (Revision: 429737).
Bad build: 56.0.2911.0 (Revision: 430171).

You are probably looking for a change made after 429803 (known good), but no later than 429804 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/65426eef350f58c53bf76d1fd347c419347b41fd..5516863851ccc8158564461bb1c15af7f16436ac

@flackr -- Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
Thank You.

Comment 2 by rtoy@chromium.org, Nov 8 2016

Components: -Blink Blink>Paint
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.
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.
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.
Labels: -OS-Linux
Still can't repro on linux. Comment #4 was Win.
Labels: ReleaseBlock-Beta
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.
663291.png
2.1 MB View Download

Comment 8 by flackr@chromium.org, 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.

Comment 9 by flackr@chromium.org, 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
Cc: chrishtr@chromium.org
Status: Started (was: Assigned)
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.
Cc: -chrishtr@chromium.org bokan@chromium.org ajuma@chromium.org
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.
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.
Labels: Hotlist-Threaded-Rendering
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Cc: ligim...@chromium.org
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.
Labels: Merge-Request-56
It seems to be working correctly for me in the latest canary - 57.0.2931.0 (Official Build) canary (64-bit) on Windows 7.

Comment 17 by dimu@chromium.org, Nov 25 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 25 2016

Labels: -merge-approved-56 merge-merged-2924
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

Status: Fixed (was: Started)
Cc: rbasuvula@chromium.org
Labels: Needs-Feedback
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! 
Attaching Screen recording for the same.

Thank you!
663291.mp4
1.8 MB View Download
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.
Labels: -Needs-Feedback
Labels: TE-Verified-M56 TE-Verified-56.0.2924.10
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!
663291(Mac).mp4
5.2 MB View Download
Project Member

Comment 25 by bugdroid1@chromium.org, 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

Project Member

Comment 26 by bugdroid1@chromium.org, 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

Owner: petermayo@chromium.org
Status: Assigned (was: Fixed)
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.
Labels: -Pri-1 -ReleaseBlock-Beta -M-56 PaintTeamTriaged-20170427 BugSource-Chromium Pri-2
Not a P1 level problem because this does not affect functionality. Removing milestones.

Comment 29 by yigu@chromium.org, Nov 23 2017

Cc: petermayo@chromium.org
Owner: yigu@chromium.org
Project Member

Comment 30 by bugdroid1@chromium.org, 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

Comment 31 by yigu@chromium.org, Dec 5 2017

Status: Fixed (was: Assigned)

Sign in to add a comment