1238564 - Active opacity that's currently offscreen gets culled during drawing even when it's inside the display port

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: failing reftest

This is a regression from bug 1147673.

Comment 1

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ce68a02566e

Bug 1147673 changed APZ scrolling of active opacities to work like main thread scrolling - the opacity ContainerLayer stays in place and the contents move. However, if the content is completely outside the current scroll port, this causes the bounds of the nsDisplayOpacity to be empty, so the opacity item is skipped in ProcessDisplayItems and its contents are never painted.
The bounds of the opacity item are calculated by calling GetScrollClippedBoundsUpTo on the wrapped display list. The opacity item's scroll clip is null (we clear all clips when creating opacity items), so the GetScrollClippedBoundsUpTo call applies all scroll clips and returns an empty rect.

There are two ways we could fix this, depending on what we want an item's bounds to mean. If we want the bounds to reflect whatever is currently visible, then the empty rect is actually the correct value here. We could make FrameLayerBuilder and ClientContainerLayer::RenderLayer process display items / container layers even if they're currently empty (on the assumption that there's async-scrollable content that can cause them to not be empty in the final composition), but this feels a little forced.
The other thing we can do is to define a container item's bounds to include all pixels that can potentially be touched by the item's contents under async transforms. I like this option way more and I even expect it to fix a test that I originally wrote for bug 1148855 (pull-background-animated-position-5.html).

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this since it's a recent regression from APZ work and we aim to enable apz be default on release in 46.

Comment 3

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r?roc

This makes sure that for example the bounds of an opacity item are not empty if
the opacity item contains a scroll frame whose contents are currently scrolled
offscreen but still inside that scroll frame's display port.

On its own, this changeset causes test failures due to missed optimizations
because the bounds of many opacity items are now too large. That's because of
the way we're setting scroll clips on opacity items at the moment: Even if the
opacity is inside a scroll frame, we're currently only setting that scroll
frame's scroll clip on the opacity item's contents, not on the opacity item
itself, because the opacity item might also contain other items that are not
scrolled by this scroll frame.
The next patch in this bug will make us only do that when necessary.

Review commit: https://reviewboard.mozilla.org/r/36821/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36821/

Comment 4

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Allow constructing nsDisplayWrapList with a given scroll clip. r?roc

Review commit: https://reviewboard.mozilla.org/r/36823/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36823/

Comment 5

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?mattwoodrow

Always use an ancestor scroll clip of all direct children, or the original
scroll clip if the children don't share the same scroll clip tree.
Unfortunately this requires another pass over the stacking context display list.

Also, fix clips, scroll clips and creation order of blend items:
If a clipped mix-blend-mode item contains absolute / fixed positioned items,
those items should not be clipped, same for blend container items.
When a transform item contains blend modes, create the blend container inside
the transform.

Don't do tree comparisons on scroll clips from different scroll clip trees.
If the inner scroll clip is nullptr, because it was cleared, it will look like
it's the ancestor of the outer non-nullptr scroll clip.

These changes don't look very related, but it was very hard to get tests passing
with only some of the changes and not the others, and after having spent two
weeks on this patch I'm not thrilled about going back and checking exactly which
change was necessary to fix which test failure.

Review commit: https://reviewboard.mozilla.org/r/36825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36825/

Comment 6

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - When building a fixed/sticky display item, don't restore the clip until we're ready to build that item so that inner items aren't unnecessarily clipped. r?roc

This isn't really about regular clips, it's about scroll clips. If the inner
item has an unnecessary scroll clip (one that's already on the parent), this can
cause the inner item's bounds to be larger than necessary because, after the
first patch in bug 1238564, it will include the whole bounds of async-scrollable
scroll frames.
Also, if e.g. the inner item is an opacity item, and it has a scroll clip that's
not just the innermost ancestor scroll clip of all of its children, then
FrameLayerBuilder's mContainerBounds == mAccumulatedChildBounds assertion can
fail if the opacity item gets flattened away, because the child bounds won't
have been enlarged for the scroll clip.

There must be a better way to do the clip resetting in
nsFrame::BuildDisplayListForStackingContext - the code is not pretty at all.
But I'd rather get the tests passing first before we figure out how to clean it
up.

Review commit: https://reviewboard.mozilla.org/r/36827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36827/

Comment 7

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Include mIsAsyncScrollable information in DisplayItemScrollClip::ToString. r?roc

Review commit: https://reviewboard.mozilla.org/r/36829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36829/

Comment 8

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1ddbe6525b4

Comment 9

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/36823/#review33385

::: layout/base/nsDisplayList.cpp:2110
(Diff revision 1)
> + : nsDisplayItem(aBuilder, aFrame, aBuilder->ClipState().GetCurrentInnermostScrollClip())

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code says we can use delegated constructors.

Comment 10

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/36825/#review33387

::: layout/reftests/async-scrolling/reftest.list:37
(Diff revision 1)
> +fuzzy-if(Android,6,4) skip == offscreen-clipped-blendmode-3.html offscreen-clipped-blendmode-ref.html # bug 8724034 - wrong AGR on mix-blend-mode item

Oops, this should be bug 1251588.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8724041 [details]
MozReview Request: Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r?roc

https://reviewboard.mozilla.org/r/36821/#review33567

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8724042 [details]
MozReview Request: Bug 1238564 - Allow constructing nsDisplayWrapList with a given scroll clip. r?roc

https://reviewboard.mozilla.org/r/36823/#review33569

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8724043 [details]
MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?mattwoodrow

https://reviewboard.mozilla.org/r/36825/#review33783

::: layout/generic/nsFrame.cpp:2249
(Diff revision 1)
> +      containerItemScrollClip);

I think this is complicated enough that it would help to give a concrete example of when this happens and explain why another pass over the display items is necessary.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8724044 [details]
MozReview Request: Bug 1238564 - When building a fixed/sticky display item, don't restore the clip until we're ready to build that item so that inner items aren't unnecessarily clipped. r?roc

https://reviewboard.mozilla.org/r/36827/#review33785

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8724045 [details]
MozReview Request: Bug 1238564 - Include mIsAsyncScrollable information in DisplayItemScrollClip::ToString. r?roc

https://reviewboard.mozilla.org/r/36829/#review33787

Comment 16

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> ::: layout/generic/nsFrame.cpp:2249
> (Diff revision 1)
> > +      containerItemScrollClip);
> 
> I think this is complicated enough that it would help to give a concrete
> example of when this happens and explain why another pass over the display
> items is necessary.

I found a way to avoid the extra pass over the display items and am currently trying to make that work.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8724041 [details]
MozReview Request: Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36821/diff/1-2/

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8724042 [details]
MozReview Request: Bug 1238564 - Allow constructing nsDisplayWrapList with a given scroll clip. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36823/diff/1-2/

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8724043 [details]
MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36825/diff/1-2/

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8724044 [details]
MozReview Request: Bug 1238564 - When building a fixed/sticky display item, don't restore the clip until we're ready to build that item so that inner items aren't unnecessarily clipped. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36827/diff/1-2/

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8724045 [details]
MozReview Request: Bug 1238564 - Include mIsAsyncScrollable information in DisplayItemScrollClip::ToString. r?roc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36829/diff/1-2/

Comment 22

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Don't do another pass over the display list to figure out ancestor scroll clips. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38293/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38293/

Comment 23

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1238564 - Get rid of cross stacking context parent scroll clip. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/38295/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38295/

Comment 24

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/36825/#review33783

> I think this is complicated enough that it would help to give a concrete example of when this happens and explain why another pass over the display items is necessary.

I added a better comment in the "Don't do another pass" patch.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8724043 [details]
MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?mattwoodrow

https://reviewboard.mozilla.org/r/36825/#review34815

::: layout/generic/nsFrame.cpp:2203
(Diff revision 2)
> +    if (didResetClip) {

Could this check be combined with the !isTransformed one above?

I don't think Restore() does anything if didResetClip was false, so there's no need for nested ifs.

Same goes for the equivalent checks below.

Comment 26

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/38293/#review34911

::: layout/base/DisplayListClipState.h:262
(Diff revision 1)
> +      // Forward along the ancestor scroll clip to the original clip state.
> +      mSavedState.mStackingContextAncestorSC =
> +        DisplayItemScrollClip::PickAncestor(mSavedState.mStackingContextAncestorSC,
> +                                            mState.mStackingContextAncestorSC);

This ancestor scroll clip propagation needs to move into Restore().
When we enter an out-of-flow element, we call SetScrollClipForContainingBlockDescendants on a new clip state, and this clip state needs to make sure it doesn't clobber the ancestor scroll clip when it goes out of scope.

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8726887 [details]
MozReview Request: Bug 1238564 - Don't do another pass over the display list to figure out ancestor scroll clips. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38293/diff/1-2/

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8726888 [details]
MozReview Request: Bug 1238564 - Get rid of cross stacking context parent scroll clip. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38295/diff/1-2/

Comment 29

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8726887 [details]
MozReview Request: Bug 1238564 - Don't do another pass over the display list to figure out ancestor scroll clips. r?mattwoodrow

https://reviewboard.mozilla.org/r/38293/#review35409

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8726888 [details]
MozReview Request: Bug 1238564 - Get rid of cross stacking context parent scroll clip. r?mattwoodrow

https://reviewboard.mozilla.org/r/38295/#review35413

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc7d346c373
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00ae57c9f79
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c5a85656b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/84a877cbf631
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfd284f8791
https://hg.mozilla.org/integration/mozilla-inbound/rev/b52f496a8544
https://hg.mozilla.org/integration/mozilla-inbound/rev/728694d29faa

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fc7d346c373
https://hg.mozilla.org/mozilla-central/rev/a00ae57c9f79
https://hg.mozilla.org/mozilla-central/rev/66c5a85656b6
https://hg.mozilla.org/mozilla-central/rev/84a877cbf631
https://hg.mozilla.org/mozilla-central/rev/cbfd284f8791
https://hg.mozilla.org/mozilla-central/rev/b52f496a8544
https://hg.mozilla.org/mozilla-central/rev/728694d29faa

Comment 36

[Markus Stange [:mstange]]

Comment on attachment 8724041 [details]
MozReview Request: Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r?roc

Requesting approval for 46 and 47 on all reviewed patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: APZ / bug 1147673
[User impact if declined]: missing pieces on the page. Fixes a whole lot of bugs (see the "blocks" list)
[Describe test coverage new/current, TreeHerder]: this patch adds many tests
[Risks and why]: moderate
[String/UUID change made/needed]: none

Comment 37

[Chris Peterson [:cpeterson]]

kats says we'll want this fix for the APZ experiment on Beta 46.

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724041 [details]
MozReview Request: Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r?roc

Some risk here of regressions, but this should help us fix many apz bugs for the 46 beta apz/e10s experiment. Taking this for beta 4 for early next week.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

NOte for sheriffs, please uplift all the r+ patches here to aurora and beta.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e101e6c74b7d
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dceb30bcfe32
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d7db5f19e4bb
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c18e583d488d
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8c7f3ce97077
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9b8ac76a72bd
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dab205ab83eb

Comment 41

[Carsten Book [:Tomcat]]

has problems uplifting to beta:

grafting 335036:e101e6c74b7d "Bug 1238564 - Anticipate async scrolling when computing the scroll clipped bounds of a display list. r=roc a=lizzard"
merging layout/base/FrameLayerBuilder.cpp
merging layout/base/nsDisplayList.cpp
merging layout/base/nsDisplayList.h
merging layout/generic/nsPluginFrame.cpp
merging layout/reftests/async-scrolling/reftest.list
warning: conflicts while merging layout/reftests/async-scrolling/reftest.list! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

can you take a look ?

Comment 42

[Markus Stange [:mstange]]

The patch is hard to rebase on Beta because bug 1209278 hasn't landed on Beta. I think it should, it will make this uplift much less risky (and it's only removing unused code). I'll request Beta approval there.

Comment 43

[Markus Stange [:mstange]]

Oh, and the other problem is that bug 1231538 hasn't landed on Beta, and it has known regressions. I'll try rebasing around that one.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/38b65261b8a3
https://hg.mozilla.org/releases/mozilla-beta/rev/b6ba21bf67fc
https://hg.mozilla.org/releases/mozilla-beta/rev/28505a320d5e
https://hg.mozilla.org/releases/mozilla-beta/rev/a61aaa2d0ac7
https://hg.mozilla.org/releases/mozilla-beta/rev/9a8955f2e94f
https://hg.mozilla.org/releases/mozilla-beta/rev/f0a8c4bbf096
https://hg.mozilla.org/releases/mozilla-beta/rev/8ec449c9ddb1