New issue
Advanced search Search tips

Issue 833837 link

Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug



Sign in to add a comment

[css-grid] Scroll reset position when updating inner html on content

Reported by fi...@piktiv.se, Apr 17 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce the problem:
JS fiddle that reproduces the problem:
https://jsfiddle.net/8ca1us4u/84/

What is the expected behavior?
Scroll as usual.
Works fine in Firefox/Edge

What went wrong?
Scroll position gets reset/jumps when updating inner html

Did this work before? N/A 

Chrome version: 65.0.3325.181  Channel: stable
OS Version: 10.0
Flash Version:
 
Labels: Needs-Triage-M65
I cannot reproduce this on Chrome 66.0.3359.117 on a Mac. I don't immediately have access to a windows machine right now to test.
Cc: vamshi.kommuri@chromium.org
Components: -Blink Blink>Scroll
Labels: -Type-Bug -Pri-2 Target-67 Triaged-ET Target-66 M-66 FoundIn-66 FoundIn-67 hasbisect OS-Linux Pri-1 Type-Bug-Regression
Status: Untriaged (was: Unconfirmed)
Able to reproduce the issue on reported chrome version 65.0.3325.181 using Windows 10 and Ubuntu 14.04. The issue is seen from M60(60.0.3112.0).
Note: The issue is not seen on Mac 10.13.1
As the issue seems to be fixed on latest canary 68.0.3399.0, providing Reverse-Bisect info.

Bisect Information:
===================
Last Bad Build   : 67.0.3377.0 
First Good Build : 67.0.3378.0

While running tool bisect we have got all the bad builds, even after changing the range it didn't help. Hence providing with manual change log from omahaproxy.

https://chromium.googlesource.com/chromium/src/+log/67.0.3377.0..67.0.3378.0?pretty=fuller&n=10000

As we are not very sure about the suspect from the above change log, requesting someone to help in assigning it to the right owner.

Tentatively changing the status to Untriaged and adding component Blink>scroll, please change if this isn't apt.

Thanks!


Cc: sunyunjia@chromium.org bokan@chromium.org
sunyunjia@/bokan@: Could you please help us in identifying the CL from the above range which would have fixed this issue in M-67 and M-68.

Thanks!

Comment 5 by bokan@chromium.org, Apr 23 2018

Labels: -Pri-1 Pri-2
Owner: chaopeng@chromium.org
Status: Assigned (was: Untriaged)
I still see this in 68.0.3404.0. It only occurs for me on a low-DPI configuration (i.e. setting screen scaling to 100%).

Given it's not a regression it's not Pri-1. Chao, please investigate when you can.
Labels: -Pri-2 -M-66 M-68 ReleaseBlock-Stable Pri-1
Since this is a recent regression, please have a fix during M68 time frame.
Cc: skobes@chromium.org
A cleaner example: https://jsfiddle.net/c0ddwymh/

This issue seems related to Scroll Anchoring.

1. For grid,  we use a Layout to calc MinSizeForChild and this Layout calls ClampScrollOffsetAfterOverflowChange that reseted the scroll offset to 0,0. 

Example:
1) in chrome://flags, turn off #enable-scroll-anchoring
2) open https://jsfiddle.net/2b4ufc66/ scroll in red and click 

Stack trace see attachment.

2. Scroll Anchoring seems aid to fix the issue below. It find the anchoring by element. It does not update when user scroll less than current element height. 

When js change html, it first reset scroll to 0,0 then restore to previous anchor. These erase the user scroll. But if you scroll a huge amount it can stop at that point. I am not sure this is a bug.

skobes WDYT?
Labels: -Pri-1 -ReleaseBlock-Stable -Type-Bug-Regression -Target-66 -Target-67 Pri-2 Type-Bug
As #5, this is not a regression.
This is still an issue in 69.0.3497.23
Cc: chaopeng@chromium.org
Components: -Blink>Scroll Blink>Layout>Grid
Owner: e...@chromium.org
The underlying bug here is that grid layout clamps prematurely.  This happens even without scroll anchoring.

Scroll anchoring makes some adjustments that alter the symptoms of this bug.  But scroll anchoring isn't designed to compensate for incorrect clamping in other parts of layout.

The correct fix is for LayoutGrid to avoid clamping.  This can probably be done with DelayScrollOffsetClampScope, which is used by the flexbox code for a similar purpose.

For the web developer, a workaround is to use out-of-flow positioning or "contain: strict" on the element being updated:

  https://jsfiddle.net/duak49rx/2/

This prevents the update from triggering relayout of the grid items.

@eae, could you route this to someone on layout?
Cc: e...@chromium.org
Owner: r...@igalia.com
I'm on holidays next week but I can try to fix it this one.

It seems the diff is as simple as just adding a line like skobes@ suggested:
http://ix.io/1jjE/diff

However I don't know how to write a test for this, any idea about that?
Re #11: Here is a test you can use.
grid-preserves-scroll.html
663 bytes View Download
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 8

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

commit c4b63701a1580e28962aa8693e767d74163b17aa
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Wed Aug 08 00:10:38 2018

[css-grid] Fix scroll reset when updating content of grid item

This patch simply uses DelayScrollOffsetClampScope to avoid
resetting scroll position when the contents of a grid item are modified.

BUG= 833837 
TEST=fast/css-grid-layout/grid-item-scroll-position.html

Change-Id: I74dd5e67d432b7b86b756ed741c7ad24243e6b8e
Reviewed-on: https://chromium-review.googlesource.com/1165983
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581404}
[add] https://crrev.com/c4b63701a1580e28962aa8693e767d74163b17aa/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-item-scroll-position.html
[modify] https://crrev.com/c4b63701a1580e28962aa8693e767d74163b17aa/third_party/blink/renderer/core/layout/layout_grid.cc

Status: Fixed (was: Assigned)
Summary: [css-grid] Scroll reset position when updating inner html on content (was: CSS GRID scroll reset position when updating inner html on content)
Thank you very much skobes@!

This should be fixed now.

Sign in to add a comment