layout: Do not include position:fixed children when calculating scrollable overflow for root element
#38618
Conversation
|
🔨 Triggering try run (#16902599839) for Linux (WPT) |
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#54256) with upstreamable changes. |
e1a6ef2 to
48447fb
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
|
Can you check whether this fixes #38182 too? |
|
Test results for linux-wpt from try job (#16902599839): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (22)
Stable unexpected results (1)
|
|
|
| // We need to calculate the overflow for each fragments within the tree | ||
| // to be processed in the further stages of reflow. | ||
| let calculated_overflow = fragment.calculate_scrollable_overflow_for_parent(); | ||
|
|
||
| // Uncontained fixed positioned element while placed as a child of the | ||
| // fragment tree, is not contained by the initial containing block, | ||
| // cannot be scrolled, and should not included in the calculation. | ||
| // <https://drafts.csswg.org/css-position/#fixed-cb> | ||
| let should_include_overflow = | ||
| fragment | ||
| .retrieve_box_fragment() | ||
| .is_some_and(|box_fragment| { | ||
| box_fragment.borrow().style.get_box().position != Position::Fixed | ||
| }); | ||
|
|
||
| match should_include_overflow { | ||
| true => overflow.union(&calculated_overflow), | ||
| false => overflow, | ||
| } |
There was a problem hiding this comment.
I think it would make more sense to move this to BoxFragment::scrollable_overflow_for_parent. You can add an early return of Zero::zero() if the box uses fixed positioning.
What does "uncontained" mean in this case?
There was a problem hiding this comment.
I think it would make more sense to move this to
BoxFragment::scrollable_overflow_for_parent. You can add an early return ofZero::zero()if the box uses fixed positioning.
AFAIU, the fragment tree is arranged so that a fragment's parent is its containing block, so we doesn't to care about position there. But for fragment tree it would be different, since it contains all of the fragments, and this would the corner cases of the scrollable overflow.
What does "uncontained" mean in this case?
Just to define fixed element without ancestor that would "contain it". Adding more context..
There was a problem hiding this comment.
scrollable_overflow_for_parent is only called one place, this place, so it is safe to move it there.
There was a problem hiding this comment.
scrollable_overflow_for_parent is only called one place, this place, so it is safe to move it there.
Since it is used to calculate overflow of all Fragments, we will need to checks whether the fixed positioned fragment is contained by its parent or not. And, I am afraid that it would introduce unnecessary checks. We could introduce scrollable_overflow_for_fragment_tree (or something similar) though. 🤔
Please correct me if I am missing something though.
Missed that issue, seems to be working fine after this fix. 👍 |
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#54256) title and body. |
48447fb to
b56f3aa
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
| promise_test(async (t) => { | ||
| await new Promise(resolve => target.addEventListener('load', resolve)); | ||
|
|
||
| assert_less_than(target.contentDocument.documentElement.scrollHeight, 1000); | ||
| assert_less_than(target.contentDocument.documentElement.scrollWidth, 1000); | ||
|
|
||
| }, "Uncontained fixed positioned shouldn't affect scrollable overflow of a document element."); |
There was a problem hiding this comment.
Why use an iframe here? Couldn't you just check the scrollHeight and scrollWidth of the current document? I'm also a bit confused because in this case, the fixed position child does not have a parent in the fragment tree...so should be taken into account in FragmentTree::calculate_scrollable_overflow. You can see this by running servo with -Z dump-flow-tree.
There was a problem hiding this comment.
Why use an iframe here? Couldn't you just check the scrollHeight and scrollWidth of the current document?
Little bit afraid of about messing with the Document itself, where the ICB size, window size matters. Iframe it not perfect, by the means that it add another dependency, but it acts like a sandboxes here. Do you have a recommendation for adding a test with the document?
There was a problem hiding this comment.
I think you could avoid the <iframe> and make the test a bit stronger by:
- Measuring the
scrollHeightandscrollWidthof the main document. - Adding the big
position: fixedelement to the page contents. You can make it programmatically twice as large as the dimensions you measured before. - Measure the
scrollHeightandscrollWidthof the page after and assert that they are the same.
<iframe> loading and such add a lot of complications to the test.
There was a problem hiding this comment.
Done, please check!
| // Fixed positioned element that is not contained by any box is placed | ||
| // as a child of the fragment tree. But, it is not contained by the | ||
| // initial containing block, cannot be scrolled, and should not included | ||
| // in the calculation. |
There was a problem hiding this comment.
"not contained by any box" is still a very weird thing in terms of CSS terminology. All boxes have a containing block and we don't commonly say that an element that establishes another element's containing block "contains that element." The problem is that it's very ambiguous. Better to say that outright that an "element establishes the containing block for a fixed position descendant."
Wording aside, I think this change isn't quite right. If the child is fixed position, then the parent here is the fixed positioned element's containing block, ie it's an element that establishes a containing block for all descendants. For instance, an element with a transform establishes a containing block for fixed ancestors and indeed affects its scroll overflow. If it doesn't have a parent (is a child of the root), then it's containing block is the initial containing block.
In this test case:
<!DOCTYPE html>
<div id="foo" style="overflow: scroll; transform: translateX(100px); width: 200px; height: 200px; outline: solid; overflow: auto;">
<div style="position: fixed; height: 300px; width: 100px; background: pink;">hello</div>
</div>The overflow: scroll parent establishes a containing block for the fixed position child. This doesn't happen if you remove the transform on the parent.
Indeed the specification that really matters here says nothing about fixed position boxes, but it does say this:
The border boxes of all boxes for which it is the containing block and whose border boxes are positioned not wholly in the negative scrollable overflow region, accounting for transforms by projecting each box onto the plane of the element that establishes its 3D rendering context. [CSS3-TRANSFORMS]
I think this means that fixed positioned boxes that are children of the root (no parent) should not be taken into account for scrollable overflow.
There was a problem hiding this comment.
"not contained by any box" is still a very weird thing in terms of CSS terminology. All boxes have a containing block and we don't commonly say that an element that establishes another element's containing block "contains that element." The problem is that it's very ambiguous. Better to say that outright that an "element establishes the containing block for a fixed position descendant."
We also have specific cases when there are no ancestor (element) established one such containing block, but, I agree that "box" could be changed to "ancestor". Maybe like "Fixed positioned element without any ancestor that establishes fixed containing block...".
Nevertheless, the important things here is about the understanding of the spec.
I think this means that fixed positioned boxes that are children of the root (no parent) should not be taken into account for scrollable overflow.
This is not correct, the spec says (albeit in quite convoluted way) that "For a box A, any boxes that the box A is the containing block, we should include their border boxes if the border boxes are positioned not wholly....".
This is shown by this case:
<style>
.target-1 {
width: 100%;
height: 200px;
background-color: green;
margin-top: 200px;
position: fixed;
}
.target-2 {
width: 100%;
height: 200px;
background-color: red;
position: fixed;
}
.container {
width: 100px;
height: 100px;
will-change: transform;
overflow: scroll;
}
</style>
<div class="container">
<div class="target-1"></div>
<div class="target-2"></div>
</div>Where we could scroll the container that established a fixed containing block.
I'm also a bit confused because in this case, the fixed position child does not have a parent in the fragment tree...so should be taken into account in FragmentTree::calculate_scrollable_overflow. You can see this by running servo with -Z dump-flow-tree
Looking at the implementation, for Servo, the "parent" of fixed positioned child is the FragmentTree itself. We calculated the scrollable_overflow of a FragmentTree as the union of all it's children. This is correct for a Fragment but for a FragmentTree we also have the fixed positioned child without any ancestor that establishes a fixed containing block.
And, we used these for the root scroll node's content_rect, viewport ScrollHeight, and viewport ScrollWidth. For root scroll node, it does not make sense to include them since the "containing block" for them is the reference frame. See
servo/components/layout/display_list/stacking_context.rs
Lines 166 to 178 in 6029976
Thus, they are not scrollable by it in the first hand.
There was a problem hiding this comment.
We also have specific cases when there are no ancestor (element) established one such containing block, but, I agree that "box" could be changed to "ancestor". Maybe like "Fixed positioned element without any ancestor that establishes fixed containing block...".
Nope, this is still wrong. A fixed position element can have the initial containing block as its containing block, but still have an ancestor in the flat tree. At the very least, unless the fixed position element is the root element, then it always has an ancestor. I think the thing here is that we are getting confused talking about the ancestor of the element and the ancestor of the fragment in the fragment tree. The fixed positioned element has an ancestor, but the fixed positioned fragment is much more likely to not have an ancestor in the fragment tree.
I think this means that fixed positioned boxes that are children of the root (no parent) should not be taken into account for scrollable overflow.
This is not correct, the spec says (albeit in quite convoluted way) that "For a box
A, any boxes that the boxAis the containing block, we should include their border boxes if the border boxes are positioned not wholly....".This is shown by this case:
<style> .target-1 { width: 100%; height: 200px; background-color: green; margin-top: 200px; position: fixed; } .target-2 { width: 100%; height: 200px; background-color: red; position: fixed; } .container { width: 100px; height: 100px; will-change: transform; overflow: scroll; } </style> <div class="container"> <div class="target-1"></div> <div class="target-2"></div> </div>Where we could scroll the container that established a fixed containing block.
This isn't the case that I was trying to refer to though. This is the case where a fixed position child has a containing block which isn't the initial containing block. What I'm suggesting is that when the containing block of a fixed positioned element is the initial containing block, perhaps it should not be considered for scroll overflow calculation. The specification says that "For a box A, any boxes that the box A is the containing block, we should include their border boxes if the border boxes are positioned not wholly....". When the containing block is the initial containing block, I think that means that there is no box A that is the containing block for the fixed positioned element.
I'm also a bit confused because in this case, the fixed position child does not have a parent in the fragment tree...so should be taken into account in FragmentTree::calculate_scrollable_overflow. You can see this by running servo with -Z dump-flow-tree
Looking at the implementation, for Servo, the "parent" of fixed positioned child is the
FragmentTreeitself. We calculated thescrollable_overflowof aFragmentTreeas the union of all it's children. This is correct for aFragmentbut for aFragmentTreewe also have the fixed positioned child without any ancestor that establishes a fixed containing block.
In this case, the fixed positioned Fragment is right under the root of the FragmentTree, but your change doesn't address that case, so I'm confused how it affects the outcome.
There was a problem hiding this comment.
Okay, ah sorry, I get the point now.
In this case, the fixed positioned Fragment is right under the root of the Fra 3933 gmentTree, but your change doesn't address that case, so I'm confused how it affects the outcome.
Not sure are we referring to the same cases or not, but for cases like data:text/html, <div style="position: fixed;"></div>, the fragment(s) is placed inside the FragmentTree.root_fragments alongside root fragment which is the first in the list.
AFAIU, fixed positioned fragment will reside under root fragment when root fragment establishes containing block for fixed positioned fragment, which seems conformant to the spec. They are scrollable, and could extend the scrollable area.
https://drafts.csswg.org/css-position/#fixed-cb
If no ancestor establishes one, the fixed positioning containing block is:
- in continuous media, the layout viewport (whose size matches the dynamic viewport size); as a result, fixed boxes do not move when the document is scrolled.
Print Flow Tree Result
┌ Fragment Tree
│ ├─ Box
│ │ base=BaseFragment { tag: Some(Tag { node: OpaqueNode(43961552470312), pseudo: None }), flags: FragmentFlags(IS_ROOT_ELEMENT) }
│ │ content=Rect(1024pxx8px at (0px, 0px))
│ │ padding rect=Rect(1024pxx8px at (0px, 0px))
│ │ border rect=Rect(1024pxx8px at (0px, 0px))
│ │ margin=(0px,0px,0px,0px)
│ │ scrollable_overflow=Rect(1024pxx8px at (0px, 0px))
│ │ baselines=Baselines { first: None, last: None }
│ │ overflow=AxesOverflow { x: Visible, y: Visible }
│ │ ├─ Box
│ │ │ base=BaseFragment { tag: Some(Tag { node: OpaqueNode(43961552470392), pseudo: None }), flags: FragmentFlags(IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT) }
│ │ │ content=Rect(1008pxx0px at (8px, 8px))
│ │ │ padding rect=Rect(1008pxx0px at (8px, 8px))
│ │ │ border rect=Rect(1008pxx0px at (8px, 8px))
│ │ │ margin=(8px,8px,8px,8px)
│ │ │ scrollable_overflow=Rect(1008pxx0px at (8px, 8px))
│ │ │ baselines=Baselines { first: None, last: None }
│ │ │ overflow=AxesOverflow { x: Visible, y: Visible }
│ │ │ └─ AbsoluteOrFixedPositioned
│ ├─ Box
│ │ base=BaseFragment { tag: Some(Tag { node: OpaqueNode(43961552470432), pseudo: None }), flags: FragmentFlags(0x0) }
│ │ content=Rect(0pxx0px at (8px, 8px))
│ │ padding rect=Rect(0pxx0px at (8px, 8px))
│ │ border rect=Rect(0pxx0px at (8px, 8px))
│ │ margin=(0px,0px,0px,0px)
│ │ scrollable_overflow=Rect(0pxx0px at (8px, 8px))
│ │ baselines=Baselines { first: None, last: None }
│ │ overflow=AxesOverflow { x: Visible, y: Visible }
There was a problem hiding this comment.
Not sure are we referring to the same cases or not, but for cases like
data:text/html, <div style="position: fixed;"></div>, the fragment(s) is placed inside theFragmentTree.root_fragmentsalongside root fragment which is the first in the list.
Perhaps the fix is to ignore just the fixed position fragments that are directly in the FragmentTree::root_fragments vector as these are the fragments that have the initial containing block as their containing blocks?
There was a problem hiding this comment.
Perhaps the fix is to ignore just the fixed position fragments that are directly in the FragmentTree::root_fragments vector as these are the fragments that have the initial containing block as their containing blocks?
Yes, I believe that is what is PR planning to do. What do you think about the current changes?
Signed-off-by: Jo Steven Novaryo <jo.steven.novaryo@huawei.com>
b56f3aa to
b394565
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
| promise_test(async (t) => { | ||
| await new Promise(resolve => target.addEventListener('load', resolve)); | ||
|
|
||
| assert_less_than(target.contentDocument.documentElement.scrollHeight, 1000); | ||
| assert_less_than(target.contentDocument.documentElement.scrollWidth, 1000); | ||
|
|
||
| }, "Uncontained fixed positioned shouldn't affect scrollable overflow of a document element."); |
There was a problem hiding this comment.
I think you could avoid the <iframe> and make the test a bit stronger by:
- Measuring the
scrollHeightandscrollWidthof the main document. - Adding the big
position: fixedelement to the page contents. You can make it programmatically twice as large as the dimensions you measured before. - Measure the
scrollHeightandscrollWidthof the page after and assert that they are the same.
<iframe> loading and such add a lot of complications to the test.
| let should_include_overflow = | ||
| fragment | ||
| .retrieve_box_fragment() | ||
| .is_some_and(|box_fragment| { | ||
| box_fragment.borrow().style.get_box().position != Position::Fixed | ||
| }); | ||
|
|
||
| match should_include_overflow { | ||
| true => overflow.union(&calculated_overflow), | ||
| false => overflow, | ||
| } |
There was a problem hiding this comment.
Nit: I think an early return is a little bit nicer here rather than a match on a boolean which always feels a bit odd:
if fragment
.retrieve_box_fragment()
.is_some_and(|box_fragment| {
box_fragment.borrow().style.get_box().position != Position::Fixed
})
{
return overflow;
}
fragment
.calculate_scrollable_overflow_for_parent()
.union(&overflow),
There was a problem hiding this comment.
Nice, thanks! It does looks weird to have boolean matches.
| // We need to calculate the overflow for each fragments within the tree | ||
| // to be processed in the further stages of reflow. | ||
| let calculated_overflow = fragment.calculate_scrollable_overflow_for_parent(); |
There was a problem hiding this comment.
I wouldn't do this is the fragment is position fixed.
There was a problem hiding this comment.
The calculation is still required by stacking context tree when we are defining the scroll frame. I have modified it to be more appropriate. Please let me know if you have more concern.
| // Fixed positioned fragment without any ancestor that establishes | ||
| // its containing block is placed as a child of the fragment tree. | ||
| // But, it is not contained by the initial containing block, cannot | ||
| // be scrolled, and should not included in the calculation. These | ||
| // fragments should be ignored. | ||
| // <https://drafts.csswg.org/css-position/#fixed-cb> |
There was a problem hiding this comment.
| // Fixed positioned fragment without any ancestor that establishes | |
| // its containing block is placed as a child of the fragment tree. | |
| // But, it is not contained by the initial containing block, cannot | |
| // be scrolled, and should not included in the calculation. These | |
| // fragments should be ignored. | |
| // <https://drafts.csswg.org/css-position/#fixed-cb> | |
| // Scrollable overflow should be accumulated in the block that | |
| // establishes the containing block for the element. Thus, fixed | |
| // positioned fragments whose containing block is the initial | |
| // containing block should not be included in overflow calculation. | |
| // See <https://www.w3.org/TR/css-overflow-3/#scrollable>. |
There was a problem hiding this comment.
Done, thanks.
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
329a3bf to
185776c
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
aac042c to
bee8813
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
bee8813 to
6047724
Compare
|
Thanks! |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
Signed-off-by: Jo Steven Novaryo <jo.steven.novaryo@huawei.com>
6047724 to
f4cfcaa
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#54256). |
position:fixed children when calculating scrollable overflow for root element
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#54256) title and body. |
…ollable overflow for root element (servo#38618) Reimplementation of: servo#35931 For a `FragmentTree` we define a scrollable overflow calculation that includes the overflow all of it's children `Fragments`. In practice we are using this calculation for scrolling area of the viewport and defining the root scroll frames. However, since uncontained fixed positioned element is located outside of the document and should not be scrolled, and therefore it would make no sense to include them in the calculation of its scrollable overflow as well. Testing: New and existing WPT tests Fixes: servo#38617 Fixes: servo#38182 --------- Signed-off-by: Jo Steven Novaryo <jo.steven.novaryo@huawei.com>
Reimplementation of: #35931
For a
FragmentTreewe define a scrollable overflow calculation that includes the overflow all of it's childrenFragments. In practice we are using this calculation for scrolling area of the viewport and defining the root scroll frames. However, since uncontained fixed positioned element is located outside of the document and should not be scrolled, and therefore it would make no sense to include them in the calculation of its scrollable overflow as well.Testing: New and existing WPT tests
Fixes: #38617
Fixes: #38182