Closed Bug 1238564 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 + fixed
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: regression)

Attachments

(8 files)

3.84 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Attached patch failing reftestSplinter Review
This is a regression from bug 1147673.
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).
Blocks: 1246469
Blocks: 1246848
Tracking this since it's a recent regression from APZ work and we aim to enable apz be default on release in 46.
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/
Attachment #8724041 - Flags: review?(roc)
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/
Attachment #8724043 - Flags: review?(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/
Attachment #8724044 - Flags: review?(roc)
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.
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.
Blocks: 1244873
Blocks: 1238534
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
Attachment #8724041 - Flags: review?(roc) → review+
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 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.
Attachment #8724043 - Flags: review?(roc)
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
Attachment #8724044 - Flags: review?(roc) → review+
Comment on attachment 8724045 [details]
MozReview Request: Bug 1238564 - Include mIsAsyncScrollable information in DisplayItemScrollClip::ToString. r?roc

https://reviewboard.mozilla.org/r/36829/#review33787
(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 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 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/
Attachment #8724043 - Attachment description: MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?roc → MozReview Request: Bug 1238564 - Set the innermost possible scroll clip on opacity items during creation. r?mattwoodrow
Attachment #8724043 - Flags: review?(matt.woodrow)
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 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 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/
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 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.
Attachment #8724043 - Flags: review?(matt.woodrow) → review+
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 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 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 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
Attachment #8726887 - Flags: review?(matt.woodrow) → review+
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
Attachment #8726888 - Flags: review?(matt.woodrow) → review+
Depends on: 1255068
No longer depends on: 1255068
Duplicate of this bug: 1238730
Duplicate of this bug: 1246469
No longer blocks: 1246848
Duplicate of this bug: 1246848
Blocks: 1247854
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
Attachment #8724041 - Flags: approval-mozilla-beta?
Attachment #8724041 - Flags: approval-mozilla-aurora?
Attachment #8724042 - Flags: approval-mozilla-beta?
Attachment #8724042 - Flags: approval-mozilla-aurora?
Attachment #8724042 - Flags: approval-mozilla-beta?
Attachment #8724042 - Flags: approval-mozilla-aurora?
kats says we'll want this fix for the APZ experiment on Beta 46.
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.
Attachment #8724041 - Flags: approval-mozilla-beta?
Attachment #8724041 - Flags: approval-mozilla-beta+
Attachment #8724041 - Flags: approval-mozilla-aurora?
Attachment #8724041 - Flags: approval-mozilla-aurora+
NOte for sheriffs, please uplift all the r+ patches here to aurora and beta.
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 ?
Flags: needinfo?(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.
Flags: needinfo?(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.
Depends on: 1264297
You need to log in before you can comment on or make changes to this bug.