1193557 - Using mix-blend-mode flattens classic scrollbars with APZ

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: minimal testcase

Using mix-blend-mode causes classic scrollbars to be flattened into the PaintedLayer of the scrollframe, even when the scrollframe has a displayport. The result is that the thumb and track of a classic scrollbar move asynchronously with the scrolled content. (See testcase for an example.)

We should either disable APZ for these frames or make sure the scrollbars are layerized, ideally the latter.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip

This patch hoists classic scrollbars outside the blend container, but it doesn't fix the problem yet. We're not getting metrics on the scrollframe or the scroll thumb.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

So this approach turns out not to work, since APZ won't be able to deliver events to subframes. This is complicated by the fact that blur modes are only known after vising all children in a stacking context. Our options are:

(1) Support blend modes in the compositor.
(2) Make blend modes trigger reflow, and track which stacking frames would create a blend container.
(3) Handle events synchronously, and maintain/add synchronous event paths where missing (Desktop touch events, for example).
(4) Create scroll info items unconditionally, and hoist them into the outermost stacking context.

Option #1 is ideal, but it's a lot of work. #2 seems complicated. #3 seems unideal since we want to get rid of synchronous event paths and not add more. So we're left with #4, which has a problem: hit testing will break when scroll info items are hoisted above a rotated transform. For now, we're deciding that's okay since blur modes are obscure enough to begin with, and it's unlikely we'll care about hit testing rotated scrollable frames that are blurred.

This patch implements #4, and supersedes the existing scroll info item hoisting used for SVG effects.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix hoisting, part 1

This patch adds two scroll info item hoisting lists: a "committed" list of confirmed items that must be hoisted, and a "pending" list of items that may need to be hoisted later.

Pending lists are temporary and created for each stacking context. If at the end of a stacking context we see that an SVG or blur effect would flatten layers, we take the current pending list and append it to the committed list, which is owned by nsLayoutUtils::PaintFrame().

If layers will not be flattened, the pending list for the stacking context is merged into the parent stacking context's pending list.

At the root stacking context, both lists are resolved. The committed list will be non-empty if any blur modes or SVG effects occurred. The pending list will be empty if all stacking contexts were flattened or no scrollframes were active. The committed list is merged into the final display list, and the pending list is thrown away (since those scrollframes will have active layers).

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8653157 [details] [diff] [review]
fix hoisting, part 1

I think technically we may only need scroll info items for the rootmost flattened scrollframes, since nested ones will not propagate their event regions up and will never activate. But in case we ever want to fix that, this patch will support it.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, fix event regions

Scroll info items get hoisted, but event regions for inactive frames don't. This means nested, flattened subframes cannot be scrolled when APZ is enabled.

This patch simply makes scroll info layers into one big dispatch-to-content region. Alternately, we could add event region items to the hoisted list, or we could aggregate their regions like how the scroll info items get aggregated. But this is a quicker fix for an obscure case and the performance shouldn't matter too much.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, test case

Tests both issues in this bug.

Comment 7

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

Comment on attachment 8654433 [details] [diff] [review]
part 2, fix event regions

Review of attachment 8654433 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 8

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

Comment on attachment 8654434 [details] [diff] [review]
part 3, test case

Review of attachment 8654434 [details] [diff] [review]:
-----------------------------------------------------------------

This test I'm a little concerned about. A few things:
- I'd like the test moved into gfx/layers/apz/test
- Please make sure the test is passing everywhere; in particular B2G doesn't really support wheel events and so this test might fail there.
- I applied the test locally (without the fixes) and it passed without APZ but failed with APZ enabled. That's as expected, I think. However the APZ-enabled case also triggered an assertion in my debug build. With the fixes the test itself passed but again there were assertions, and the assertions caused the mochitest harness to fail the test. I'm not sure if the assertions are a pre-existing issue or not - at least one of them is but the others may not be and it's probably worth looking into at least. If they are pre-existing then this is fine and we can file a follow-up for the assertions. If they are not pre-existing then we'll have to fix them.

Comment 9

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

Also for the record I ran test on Linux debug desktop build like so:

xvfb-run ./mach mochitest -f plain --e10s --setpref layers.async-pan-zoom.enabled=true layout/generic/test/test_scroll_inactive_flattened_frame.html

For non-APZ I omitted the --e10s and --setpref bits.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8653157 [details] [diff] [review]
fix hoisting, part 1

Review of attachment 8653157 [details] [diff] [review]:
-----------------------------------------------------------------

So this is mostly fine but I don't like the part that gets the layer manager, and makes the display list look different based on the layer manager type. Is there a way to avoid that? Where do we currently decide to flatten blend modes?

::: layout/generic/nsFrame.cpp
@@ +2256,5 @@
> +      hoistedScrollInfoItems.Commit();
> +    }
> +
> +    if (hoistedScrollInfoItems.IsFinished()) {
> +      // If we're the parent stacking context, no more blur modes can be

s/parent/root/? (And s/blur/blend/)

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Markus Stange [:mstange] from comment #10)
> Comment on attachment 8653157 [details] [diff] [review]
> fix hoisting, part 1
> 
> Review of attachment 8653157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is mostly fine but I don't like the part that gets the layer
> manager, and makes the display list look different based on the layer
> manager type. Is there a way to avoid that? Where do we currently decide to
> flatten blend modes?

We decide here in nsDisplayMixBlendMode::GetLayerState: https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4025 

So it looks hard to determine without the LayerManager. Though maybe we could store the blend mode in the scroll info item and then ignore it during display list processing if it won't flatten.

Comment 12

[Markus Stange [:mstange]]

(In reply to David Anderson [:dvander] from comment #11)
> Though maybe we
> could store the blend mode in the scroll info item and then ignore it during
> display list processing if it won't flatten.

That sounds better to me.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, fix hoisting v2

This version only commits scroll info layer items within SVG effects. Items within blend modes stay in the pending list, and then are only committed at the very end.

ScrollInfoItems created within blend modes, but not SVG effects, will skip creating a container layer if their LayerManager supports its blend modes.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8655093 [details] [diff] [review]
part 1, fix hoisting v2

>+  void SetUsingWidgetLayers(bool aValue) { mIsUsingWidgetLayers = aValue; }
>+  bool IsUsingWidgetLayers() const { return mIsUsingWidgetLayers; }

Doesn't IsPaintingToWindow do what you want already?

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8655093 [details] [diff] [review]
part 1, fix hoisting v2

Review of attachment 8655093 [details] [diff] [review]:
-----------------------------------------------------------------

It's not pretty, but I can't come up with anything better.

::: layout/base/nsDisplayList.h
@@ +317,5 @@
>    /**
> +   * Call this if we're painting to widget layers.
> +   */
> +  void SetUsingWidgetLayers(bool aValue) { mIsUsingWidgetLayers = aValue; }
> +  bool IsUsingWidgetLayers() const { return mIsUsingWidgetLayers; }

I agree with Timothy - let's always do it when we're painting, not just for widget layers.

@@ +976,5 @@
>    nsDisplayItem*                 mGlassDisplayItem;
> +  // When encountering inactive layers, we need to hoist scroll info items
> +  // above these layers so APZ can deliver events to content. Such scroll
> +  // info items are considered "committed" to the final hoisting list. If
> +  // no hoisting is needed immediately, it may be needed later if a blur

s/blur/blend/

::: layout/generic/nsFrame.cpp
@@ +2292,5 @@
> +      hoistedScrollInfoItems.AnnotateForBlendModes(aBuilder->ContainedBlendModes());
> +    }
> +
> +    if (hoistedScrollInfoItems.IsRootStackingContext()) {
> +      // If we're the parent stacking context, no more mix-blend modes can be

The comment still needs to be fixed (parent -> root).

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8655093 [details] [diff] [review]
part 1, fix hoisting v2

Review of attachment 8655093 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +1944,5 @@
> +  void Commit() {
> +    nsDisplayItem* iter = nullptr;
> +    while ((iter = mPendingList.RemoveBottom()) != nullptr) {
> +      MOZ_ASSERT(iter->GetType() == nsDisplayItem::TYPE_SCROLL_INFO_LAYER);
> +      nsDisplayScrollInfoLayer *item = static_cast<decltype(item)>(iter);

I just did an informal poll in the office, and this form was preferred:
auto item = static_cast<nsDisplayScrollInfoLayer*>(iter);

(3 votes vs 0 votes - "decltype is scary.")

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: test case

Test case is moved to gfx/layers/apz and is pref'd off for platforms with wheel events. The assertions appear to be pre-existing. I did get them when I ran the suite on Linux using xvfb, but I don't see them on Windows and nothing complained on a try run.

Comment 18

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

Comment on attachment 8656175 [details] [diff] [review]
test case

Review of attachment 8656175 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d925443d4cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2333b14cf1

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3906b2dab080

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d925443d4cd
https://hg.mozilla.org/mozilla-central/rev/fb2333b14cf1
https://hg.mozilla.org/mozilla-central/rev/3906b2dab080

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/755b9088813c

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb99456bccb

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1699e68d5efb

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1699e68d5efb

Comment 27

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4315f192ec3a

Comment 28

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

Comment 27 belongs on bug 1209278.