Skip to content

GitLab

T
tangled
Commit cd28c42d authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

Fix position:sticky when inside fixed subtree 

In https://crrev.com/4d25b125dae5 I changed the scroll tree parenting
logic so that elements in position:fixed subtrees have the LayoutView's
ScrollNode as the scroll parent. This made sense since scrolling over a
fixed element should cause the document to scroll. However, this is
slightly different from how the transform tree looks. Because scrolling
the document doesn't cause position:fixed eement so translate, these
nodes don't have the LayoutView's ScrollTranslation transform node as an
ancestor.

As a simple example, a scrolling document with a position:fixed <div>
scroller will generate the following scroll and transform trees
(approximately):

      *ScrollTree*                          *TransformTree*

         Root                                    Root
          |                                       |
VisualViewport Translation                 VisualViewport
          |                                  /         \
  LayoutView Translation                    /           \
          |                             Fixed      LayoutView
  Fixed Scroller Translation


The situation above makes sense for what parent-child relationships mean
in each tree: the scroll tree encodes how scrolls chain; scrolling on a
child should bubble up to its parent in this tree. The transform tree
encodes the physical effect of scrolling a node. In the above example,
scrolling from the fixed scroller should bubble up to the LayoutView
(when the scroller is fully scrolled) but scrolling the LayoutView will
not cause movement of the fixed scroller.

The above makes sense but caused sticky code to get confused. A sticky
constraint is attached to the scroll translation node. With the above
situation, this meant that inside a fixed subtree, we'd attach it to the
VisualViewport's scroll translation node. This was unexpected; the
constraints are in "document coordinates", meaning that to translate
them into the viewport space we must apply the scroll offset [1]. The
compositor would use the visual viewport's (typically 0) scroll offset
to adjust these values, leading to incorrect calculations.

This previously worked because the scroll node used in a fixed subtree
would be the visual viewport (before the CL mentioned at the top). In
[2] we check whether the current overflow clip is also our scroller,
prior to the CL this check have failed because "our scroller" was the
visual viewport but our clip was the LayoutView. Now they are both the
LayoutView.

The fix in this CL is to make the check in [2] more stringent; we also
want to make sure that our scroller is the nearest scroller in the
transform tree. That is, if we scroll it, will we cause the current node
to move? If not, we don't need a sticky constraint on the compositor
because user scrolling can't change the sticky's offset relative to its
clip.

[1] https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?l=321&rcl=628f869d1fda631a85f051ad13b5d278319298fc
[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc?l=553&rcl=99a5a1266f303ba6ae46174a2b4cbd165ea7e934

Bug: 1019142
Change-Id: I781943ff43514905d399803c780c6081d7d47e8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2097542Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750264}
parent e72859e7
Hide whitespace changes
Inline Side-by-side
Showing with 371 additions and 2 deletions
third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
View file @ cd28c42d
......@@ -549,8 +549,20 @@ void FragmentPaintPropertyTreeBuilder::UpdateStickyTranslation() {
// DCHECK_EQ(scroller_properties->Scroll(), context_.current.scroll);
// However there is a bug that AncestorOverflowLayer() may be computed
// incorrectly with clip escaping involved.
if (scroller_properties &&
scroller_properties->Scroll() == context_.current.scroll) {
bool nearest_scroller_is_clip =
scroller_properties &&
scroller_properties->Scroll() == context_.current.scroll;
// Additionally, we also want to make sure that the nearest scroller
// actually translates this node. If it doesn't (e.g. a position fixed
// node in a scrolling document), there's no point in adding a constraint
// since scrolling won't affect it. Indeed, if we do add it, the
// compositor assumes scrolling does affect it and produces incorrect
// results.
bool translates_with_nearest_scroller =
context_.current.transform->NearestScrollTranslationNode()
.ScrollNode() == context_.current.scroll;
if (nearest_scroller_is_clip && translates_with_nearest_scroller) {
const StickyPositionScrollingConstraints& layout_constraint =
layer->AncestorOverflowLayer()
->GetScrollableArea()
......
third_party/blink/web_tests/external/wpt/css/css-position/position-sticky-fixed-ancestor-iframe-ref.html 0 → 100644
View file @ cd28c42d
<!DOCTYPE html>
<title>Sticky elements inside fixed ancestors and iframe shouldn't account for scroll</title>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
iframe {
margin: 10px;
width: 90%;
height: 90%;
border: 1px solid black;
}
.spacer {
height: 120vh;
}
</style>
<div class="spacer"></div>
<iframe srcdoc="
<!DOCTYPE html>
<title>Reference for sticky elements inside fixed ancestors shouldn't account for scroll</title>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
.sticky {
background: green;
width: 100px;
height: 10%;
}
.spacer {
height: calc(25vh - 10%);
}
.long {
height: 600vh;
}
.position-parent {
position: absolute;
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
top: 100vh;
background-color: lightgrey;
}
.container {
width: 100px;
height: 100%;
background-color: grey;
}
button {
position: fixed;
left: 20px;
top: 20px;
}
.fixed {
position: fixed;
top: 25vh;
}
</style>
<div class='position-parent fixed'>
<div class='container'>
<div class='spacer'></div>
<div class='sticky'></div>
</div>
</div>
<div class='long'></div>
<button id='button'>Toggle Fixed</button>
<script>
window.scrollTo(0, document.querySelector('.long').clientHeight);
</script>
"></iframe>
<div class="spacer"></div>
<script>
const child = document.querySelector('iframe');
child.scrollIntoView();
</script>
third_party/blink/web_tests/external/wpt/css/css-position/position-sticky-fixed-ancestor-iframe.html 0 → 100644
View file @ cd28c42d
<!DOCTYPE html>
<html class='reftest-wait'>
<title>Sticky elements inside fixed ancestors and iframe shouldn't account for scroll</title>
<link rel="match" href="position-sticky-fixed-ancestor-iframe-ref.html" />
<link rel="help" href="https://www.w3.org/TR/css-position-3/#sticky-pos" />
<link rel="help" href="https://crbug.com/1019142">
<meta name="assert" content="This test checks that a sticky element inside a fixed subtree and iframe doesn't scroll with the viewport "/>
<script src="/common/reftest-wait.js"></script>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
iframe {
margin: 10px;
width: 90%;
height: 90%;
border: 1px solid black;
}
.spacer {
height: 120vh;
}
</style>
<div class="spacer"></div>
<iframe src="resources/position-sticky-fixed-ancestor-iframe-child.html"></iframe>
<div class="spacer"></div>
<script>
const child = document.querySelector('iframe');
child.scrollIntoView();
window.onload = () => {
const childDoc = child.contentDocument;
function toggleFixed() {
childDoc.querySelector('.position-parent').classList.toggle('fixed');
}
childDoc.getElementById('button').addEventListener('click', toggleFixed);
requestAnimationFrame(() => {
childDoc.scrollingElement.scrollTop = childDoc.querySelector('.long').clientHeight;
requestAnimationFrame(() => {
toggleFixed();
takeScreenshot();
});
});
};
</script>
</html>
third_party/blink/web_tests/external/wpt/css/css-position/position-sticky-fixed-ancestor-ref.html 0 → 100644
View file @ cd28c42d
<!DOCTYPE html>
<title>Reference for sticky elements inside fixed ancestors shouldn't account for scroll</title>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
.sticky {
background: green;
width: 100px;
height: 10%;
}
.spacer {
height: calc(25vh - 10%);
}
.long {
height: 600vh;
}
.position-parent {
position: absolute;
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
top: 100vh;
background-color: lightgrey;
}
.container {
width: 100px;
height: 100%;
background-color: grey;
}
button {
position: fixed;
left: 20px;
top: 20px;
}
.fixed {
position: fixed;
top: 25vh;
}
</style>
<div class="position-parent fixed">
<div class="container">
<div class="spacer"></div>
<div class="sticky"></div>
</div>
</div>
<div class="long"></div>
<button id="button">Toggle Fixed</button>
<script>
window.scrollTo(0, document.querySelector('.long').clientHeight);
</script>
third_party/blink/web_tests/external/wpt/css/css-position/position-sticky-fixed-ancestor.html 0 → 100644
View file @ cd28c42d
<!DOCTYPE html>
<html class='reftest-wait'>
<title>Sticky elements inside fixed ancestors shouldn't account for scroll</title>
<link rel="match" href="position-sticky-fixed-ancestor-ref.html" />
<link rel="help" href="https://www.w3.org/TR/css-position-3/#sticky-pos" />
<link rel="help" href="https://crbug.com/1019142">
<meta name="assert" content="This test checks that a sticky element inside a fixed subtree doesn't scroll with the viewport "/>
<script src="/common/reftest-wait.js"></script>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
.sticky {
background: green;
position: sticky;
bottom: 50vh;
width: 100px;
height: 10%;
}
.spacer {
height: 90%;
}
.long {
height: 600vh;
}
.position-parent {
position: absolute;
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
top: 100vh;
background-color: lightgrey;
}
.container {
width: 100px;
height: 100%;
background-color: grey;
}
button {
position: fixed;
left: 20px;
top: 20px;
}
.fixed {
position: fixed;
top: 25vh;
}
</style>
<div class="position-parent">
<div class="container">
<div class="spacer"></div>
<div class="sticky"></div>
</div>
</div>
<div class="long"></div>
<button id="button">Toggle Fixed</button>
<script>
function toggleFixed() {
document.querySelector('.position-parent').classList.toggle('fixed');
}
document.getElementById('button').addEventListener('click', toggleFixed);
requestAnimationFrame(() => {
window.scrollTo(0, document.querySelector('.long').clientHeight);
requestAnimationFrame(() => {
toggleFixed();
takeScreenshot();
});
});
</script>
</html>
third_party/blink/web_tests/external/wpt/css/css-position/resources/position-sticky-fixed-ancestor-iframe-child.html 0 → 100644
View file @ cd28c42d
<!DOCTYPE html>
<style>
body,html {
margin: 0;
width: 100%;
height: 100%;
}
.sticky {
background: green;
position: sticky;
bottom: 50vh;
width: 100px;
height: 10%;
}
.spacer {
height: 90%;
}
.long {
height: 600vh;
}
.position-parent {
position: absolute;
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
top: 100vh;
background-color: lightgrey;
}
.container {
width: 100px;
height: 100%;
background-color: grey;
}
button {
position: fixed;
left: 20px;
top: 20px;
}
.fixed {
position: fixed;
top: 25vh;
}
</style>
<div class="position-parent">
<div class="container">
<div class="spacer"></div>
<div class="sticky"></div>
</div>
</div>
<div class="long"></div>
<button id="button">Toggle Fixed</button>
</html>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment