If we have an anchor that is transformed, then if we assign the ASR of the anchor to the anchored content, that means the anchored content will get the same spatial node in web render as the anchor, that means web render will draw it as if it is also transformed. This is not correct. In order to not render incorrectly in this case we disable async scrolling with the anchor in this case. It will still scroll with the anchor, just without the benefit of async scrolling (ie the main thread updates it).
In order to avoid this we keep track on the nsDisplayListBuilder whether a frame async scrolls with its anchor or not. We don't try to do any tracking outside of painting. This seems okay because outside of painting we only use this information for activating display ports, and async scrolling with the anchor only inserts more frames in the ASR/async scrollable frame ancestor chain. We clear the information on the nsDisplayListBuilder after every frame. RDL is already disabled when anchor pos async scroll is happening so we don't have to worry about any invalidations.
This then requires an extra walk of the same frames that ActivateDisplayportOnASRAncestors will walk over later. And further, we might have to recurse if we have chained anchors and walk them again. Minimizing this walk seems too complicated. Chained anchors shouldn't be common, if we are walking these frames they should be in the cpu's cache.
We have to create yet another function that walks over ASRs (ShouldAsyncScrollWithAnchorNotCached), because we need to check each individual frame that is walked for being transformed (the existing users don't need this level of granularity).
Since it's only a bit more code this patch also fixes bug 2003845, which exists because ASRs can only scroll in both axis, but anchor pos sometimes only scrolls in one axis with its anchor. If we encounter this case, and a scroll frame for which this would actually make a difference we disable async scroll as well.
Differential Revision: https://phabricator.services.mozilla.com/D275453
This reverts commit 21344205d1.
Revert "Bug 2003843. Handle the transform case and one-axis case of async scrolling with CSS anchor pos. r=hiro,layout-reviewers"
This reverts commit c1b9aa572b.
If we have an anchor that is transformed, then if we assign the ASR of the anchor to the anchored content, that means the anchored content will get the same spatial node in web render as the anchor, that means web render will draw it as if it is also transformed. This is not correct. In order to not render incorrectly in this case we disable async scrolling with the anchor in this case. It will still scroll with the anchor, just without the benefit of async scrolling (ie the main thread updates it).
In order to avoid this we keep track on the nsDisplayListBuilder whether a frame async scrolls with its anchor or not. We don't try to do any tracking outside of painting. This seems okay because outside of painting we only use this information for activating display ports, and async scrolling with the anchor only inserts more frames in the ASR/async scrollable frame ancestor chain. We clear the information on the nsDisplayListBuilder after every frame. RDL is already disabled when anchor pos async scroll is happening so we don't have to worry about any invalidations.
This then requires an extra walk of the same frames that ActivateDisplayportOnASRAncestors will walk over later. And further, we might have to recurse if we have chained anchors and walk them again. Minimizing this walk seems too complicated. Chained anchors shouldn't be common, if we are walking these frames they should be in the cpu's cache.
We have to create yet another function that walks over ASRs (ShouldAsyncScrollWithAnchorNotCached), because we need to check each individual frame that is walked for being transformed (the existing users don't need this level of granularity).
Since it's only a bit more code this patch also fixes bug 2003845, which exists because ASRs can only scroll in both axis, but anchor pos sometimes only scrolls in one axis with its anchor. If we encounter this case, and a scroll frame for which this would actually make a difference we disable async scroll as well.
Differential Revision: https://phabricator.services.mozilla.com/D275453
The bug we are trying to fix is that OneStepInASRChain will always walk to the parent after it's while loop walking the anchor chain, potentially missing a sticky asr at the end of the anchor chain. But we can't just return the end of the anchor chain because we want to only consider it for sticky ASRs, not scroll ASRs. So we need to change these functions so that we can express that with the return value. So we change OneStepInASRChain and GetASRAncestorFrame to operate on FrameAndASRKind.
Differential Revision: https://phabricator.services.mozilla.com/D275376
I want to make a bug fix that changes the return type of GetASRAncestorFrame and OneStepInASRChain to (ASRKind, frame) so that the relation of scroll and sticky ASRs can better be represented in these functions. Trying to shoe horn all these different behaviour into one function is getting kind of complicated.
I also took the opportunity to move OneStepInAsyncScrollableAncestorChain, OneStepInASRChain, and GetASRAncestorFrame into the same place in DisplayPortUtils by ActivateDisplayportOnASRAncestors so that all this similar code is in the same place.
Differential Revision: https://phabricator.services.mozilla.com/D275373
This is the most complicated and intricate patch in this series.
We want to assign the ASR of the anchor to the anchored content. But there is no painting relationship between them. Meaning that BuildDisplayList for the anchor might be called before or after BuildDisplayList for the anchored content.
If BuildDisplayList is called for the anchor before the anchored content we want to make sure that any scroll frames containing the anchor are activated. This is easy on desktop as all scroll frames that want it are activated. But on android we don't do that yet. That's where bug 1995759 comes in to activate all scroll frames when anchor pos is used.
If BuildDisplayList is called for the anchor after the anchored content then we want to activate any scroll frames containing the anchor during the BuildDisplayList call for the anchored content. That's one of the main things that this patch does. We create a new function DecideScrollableLayerEnsureDisplayport that always set a minimal display port to do that for us (it's a simplified version of DecideScrollableLayer).
Then we use our knowledge that the anchor is in the containing block of the anchored content to walk the scroll frame ancestors of the anchor until we reach the anchored content's containing block. Once we've collected all the scroll frames we can create the ASR structs.
Differential Revision: https://phabricator.services.mozilla.com/D271806
With CSS anchor pos we know that the anchor is in the containing block of the anchored content. We use that to limit a walk up the frame tree in subsequent patches and make the computation simpler. So we need OneStepInASRChain to obey that limit too.
Differential Revision: https://phabricator.services.mozilla.com/D271804
In subsequent patches we need to match the ASR tree which is different from the "async scrollable ancestor chain", so we need these functions.
WantAsyncScroll and mWillBuildScrollableLayer are completely independent of each other, ie it's possible to have all four combinations of them being true/false.
ASR's don't walked from fixed pos to root scroll frame, that behaviour is for other reasons that don't apply to ASRs.
Differential Revision: https://phabricator.services.mozilla.com/D271805
We want to assign the ASR of the anchor to the anchored content. That means we are changing the ASR chain so we need to adjust the functions that walk the async scrollable ancestors chain. In this patch we adjust DisplayPortUtils::OneStepInAsyncScrollableAncestorChain.
Differential Revision: https://phabricator.services.mozilla.com/D271802
This reverts commit 8244ecad8b.
Revert "Bug 1988032. Disable RDL if are there active CSS anchor pos. r=layout-reviewers,emilio"
This reverts commit 9063b3c208.
Revert "Bug 1988032. Don't async scroll with anchors if there is an active view transition. r=layout-reviewers,emilio"
This reverts commit 472901d513.
Revert "Bug 1988030. Detect and reject an anchor that is outside of the positioned frame's containing block. r=layout-reviewers,emilio"
This reverts commit 237e8563f6.
Revert "Bug 1988032. Add a pref to control async scroll of CSS anchor positioned content. r=layout-reviewers,hiro"
This reverts commit 98d14e2dc8.
Revert "Bug 1988032. Add some basic anchor pos plus async scroll reftests. r=hiro,layout-reviewers"
This reverts commit 7165d46e3b.
Revert "Bug 1988032. Assign the ASR of the anchor to the anchored content. r=hiro,layout-reviewers,layout-anchor-positioning-reviewers,emilio"
This reverts commit cb45424f42.
Revert "Bug 1988030. Create a function that activates and creates ASRs for scrollframes of the anchor of anchored content. r=hiro,layout-reviewers,layout-anchor-positioning-reviewers,botond,emilio"
This reverts commit 1816a3d49c.
Revert "Bug 1988030. Allow passing a limit ancestor to DisplayPortUtils::OneStepInASRChain that we don't walk past. r=hiro,layout-reviewers"
This reverts commit 7cd59a61c6.
Revert "Bug 1988030. Create nsLayoutUtils::GetASRAncestorFrame and OneStepInASRChain that walks only scroll frames that are currently async scrollable in the ASR order. r=botond,hiro,layout-reviewers,layout-anchor-positioning-reviewers,emilio"
This reverts commit e53645f574.
Revert "Bug 1988030. Adjust DisplayPortUtils::OneStepInAsyncScrollableAncestorChain to be CSS anchor pos aware. r=hiro,layout-reviewers,layout-anchor-positioning-reviewers,emilio"
This reverts commit e7619a7efc.
Revert "Bug 1988030. Adjust nsLayoutUtils::GetAsyncScrollableAncestorFrame to be CSS anchor pos aware. r=layout-reviewers,layout-anchor-positioning-reviewers,hiro,emilio"
This reverts commit 537725a5c4.
This is the most complicated and intricate patch in this series.
We want to assign the ASR of the anchor to the anchored content. But there is no painting relationship between them. Meaning that BuildDisplayList for the anchor might be called before or after BuildDisplayList for the anchored content.
If BuildDisplayList is called for the anchor before the anchored content we want to make sure that any scroll frames containing the anchor are activated. This is easy on desktop as all scroll frames that want it are activated. But on android we don't do that yet. That's where bug 1995759 comes in to activate all scroll frames when anchor pos is used.
If BuildDisplayList is called for the anchor after the anchored content then we want to activate any scroll frames containing the anchor during the BuildDisplayList call for the anchored content. That's one of the main things that this patch does. We create a new function DecideScrollableLayerEnsureDisplayport that always set a minimal display port to do that for us (it's a simplified version of DecideScrollableLayer).
Then we use our knowledge that the anchor is in the containing block of the anchored content to walk the scroll frame ancestors of the anchor until we reach the anchored content's containing block. Once we've collected all the scroll frames we can create the ASR structs.
Differential Revision: https://phabricator.services.mozilla.com/D271806
With CSS anchor pos we know that the anchor is in the containing block of the anchored content. We use that to limit a walk up the frame tree in subsequent patches and make the computation simpler. So we need OneStepInASRChain to obey that limit too.
Differential Revision: https://phabricator.services.mozilla.com/D271804
In subsequent patches we need to match the ASR tree which is different from the "async scrollable ancestor chain", so we need these functions.
WantAsyncScroll and mWillBuildScrollableLayer are completely independent of each other, ie it's possible to have all four combinations of them being true/false.
ASR's don't walked from fixed pos to root scroll frame, that behaviour is for other reasons that don't apply to ASRs.
Differential Revision: https://phabricator.services.mozilla.com/D271805
We want to assign the ASR of the anchor to the anchored content. That means we are changing the ASR chain so we need to adjust the functions that walk the async scrollable ancestors chain. In this patch we adjust DisplayPortUtils::OneStepInAsyncScrollableAncestorChain.
Differential Revision: https://phabricator.services.mozilla.com/D271802
Calling SetProperty is the property exists actually destroys the existing property value and allocates a new one, so we should avoid it. I saw this show up in a profile (althought as a small percent).
Differential Revision: https://phabricator.services.mozilla.com/D253322
In https://hg-edge.mozilla.org/integration/autoland/rev/4681951d59b7 from bug 1379458 we made GetAsyncScrollableAncestorFrame no longer follow from oof to placeholder as part of a larger change moving away from following placeholders. However SetZeroMarginDisplayPortOnAsyncScrollableAncestors uses GetAsyncScrollableAncestorFrame but still has a GetParentOrPlaceholderForCrossDoc call in it. Looking at all the callers of SetZeroMarginDisplayPortOnAsyncScrollableAncestors they all set a display port on the passed in frame and then call SetZeroMarginDisplayPortOnAsyncScrollableAncestors so it does not seem like they would want some kind of special behaviour here, ie following the normal paint chain looks the what we want here like everywhere else.
I also checked the other GetParentOrPlaceholder calls, they don't seem to suffer from this problem (ie they seem fine).
Differential Revision: https://phabricator.services.mozilla.com/D246168
In https://hg-edge.mozilla.org/integration/autoland/rev/4681951d59b7 from bug 1379458 we made GetAsyncScrollableAncestorFrame no longer follow from oof to placeholder as part of a larger change moving away from following placeholders. However SetZeroMarginDisplayPortOnAsyncScrollableAncestors uses GetAsyncScrollableAncestorFrame but still has a GetParentOrPlaceholderForCrossDoc call in it. Looking at all the callers of SetZeroMarginDisplayPortOnAsyncScrollableAncestors they all set a display port on the passed in frame and then call SetZeroMarginDisplayPortOnAsyncScrollableAncestors so it does not seem like they would want some kind of special behaviour here, ie following the normal paint chain looks the what we want here like everywhere else.
I also checked the other GetParentOrPlaceholder calls, they don't seem to suffer from this problem (ie they seem fine).
Differential Revision: https://phabricator.services.mozilla.com/D246168
This is a chrome only css property (so it could still be used in a UA sheet that is applied to a content document) that was created solely to tabs/xul decks in a fashion that is more like the rest of css/html. And it still only has that single use in our tree. A UA sheet applying this property to content it doesn't control doesn't seem like it could be very useful.
Even those these checks are simple they are relatively costly because we check them on every frame and when we are traversing the frame tree (for display list building or setting a displayport) we are often memory bound, so pulling in another bit of otherwise cold memory for a rare style struct makes us wait for loading more memory and pushes more useful stuff out of caches. The parent process check remains very quick as verified by profiling.
Differential Revision: https://phabricator.services.mozilla.com/D231648
This is a chrome only css property (so it could still be used in a UA sheet that is applied to a content document) that was created solely to tabs/xul decks in a fashion that is more like the rest of css/html. And it still only has that single use in our tree. A UA sheet applying this property to content it doesn't control doesn't seem like it could be very useful.
Even those these checks are simple they are relatively costly because we check them on every frame and when we are traversing the frame tree (for display list building or setting a displayport) we are often memory bound, so pulling in another bit of otherwise cold memory for a rare style struct makes us wait for loading more memory and pushes more useful stuff out of caches. The parent process check remains very quick as verified by profiling.
Differential Revision: https://phabricator.services.mozilla.com/D231648
Convert `nsIScrollableFrame` to `ScrollContainerFrame` for all the APIs in
nsLayoutUtils, and then adapt other callers until everything compiles.
In `nsLayoutUtils::CalculateBasicFrameMetrics()`'s documentation,
s/ComputeFrameMetrics/ComputeScrollMetadata/ because the method was renamed in
https://hg.mozilla.org/mozilla-central/rev/cb2023f50288
Differential Revision: https://phabricator.services.mozilla.com/D211493
Also, simplify some callers of `GetScrollTargetFrame()` to drop
`nsIScrollableFrame*` and unnecessary `do_QueryFrame`. We'll continue removing
more `nsIScrollableFrame*` in later parts.
Differential Revision: https://phabricator.services.mozilla.com/D211490
In theory, changing the return type from `nsIFrame*` to `ScrollContainerFrame*`
exposes `ScrollContainerFrame` to the callers who might not needed, but almost
all of the callers in cpp files are already exposed to `nsIScrollableFrame`, as
demonstrated in this patch via replacing the #include from
"nsIScrollableFrame.h" to "ScrollContainerFrame.h", so this is OK.
Some callers can be simplified since we no longer need `do_QueryFrame` to
`nsIScrollableFrame`.
Differential Revision: https://phabricator.services.mozilla.com/D211488
Convert `nsIScrollableFrame` to `ScrollContainerFrame` for all the APIs in
nsLayoutUtils, and then adapt other callers until everything compiles.
In `nsLayoutUtils::CalculateBasicFrameMetrics()`'s documentation,
s/ComputeFrameMetrics/ComputeScrollMetadata/ because the method was renamed in
https://hg.mozilla.org/mozilla-central/rev/cb2023f50288
Differential Revision: https://phabricator.services.mozilla.com/D211493
Also, simplify some callers of `GetScrollTargetFrame()` to drop
`nsIScrollableFrame*` and unnecessary `do_QueryFrame`. We'll continue removing
more `nsIScrollableFrame*` in later parts.
Differential Revision: https://phabricator.services.mozilla.com/D211490
In theory, changing the return type from `nsIFrame*` to `ScrollContainerFrame*`
exposes `ScrollContainerFrame` to the callers who might not needed, but almost
all of the callers in cpp files are already exposed to `nsIScrollableFrame`, as
demonstrated in this patch via replacing the #include from
"nsIScrollableFrame.h" to "ScrollContainerFrame.h", so this is OK.
Some callers can be simplified since we no longer need `do_QueryFrame` to
`nsIScrollableFrame`.
Differential Revision: https://phabricator.services.mozilla.com/D211488
This is to avoid confusion with `IsScrollContainerFrame()` introduced in Part 3.
A note to performance concern:
We've used `IsScrollContainerOrSubclass()` in bug 1873414 to avoid expensive
`nsIScrollableFrame* sf = do_QueryFrame()`. Per [1], casting from `nsIFrame*` to
`nsIScrollableFrame*` has no fast path.
Ideally, `ScrollContainerFrame* sf = do_QueryFrame()` should be fast, and we can
use it to implement `IsScrollContainerOrSubclass()` like
`IsImageFrameOrSubclass()`. However, in this patch, I intend to keep the
performance metrics unchanged.
Remove `GetAsScrollContainer()` since it's only used in `MOZ_ASSERT`, and I'm
planning to remove `nsIScrollableFrame` in bug 1896516.
[1] https://searchfox.org/mozilla-central/rev/dd6e430c1bc2db90d9b3b1dd7e5215b4edc4d51a/layout/generic/nsQueryFrame.h#117-121
Differential Revision: https://phabricator.services.mozilla.com/D210395
Sylvestre Ledru