Closed Bug 1193557 Opened 4 years ago Closed 4 years ago

Using mix-blend-mode flattens classic scrollbars with APZ

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached file 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.
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #8649590 - Attachment is obsolete: true
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).
Attachment #8653096 - Attachment is obsolete: true
Attachment #8653157 - Flags: feedback?(mstange)
Attachment #8653157 - Flags: feedback?(mstange) → review?(mstange)
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.
Attachment #8653157 - Attachment description: patch → fix hoisting, part 1
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.
Attachment #8654433 - Flags: review?(bugmail.mozilla)
Attached patch part 3, test case (obsolete) — Splinter Review
Tests both issues in this bug.
Attachment #8654434 - Flags: review?(bugmail.mozilla)
Comment on attachment 8654433 [details] [diff] [review]
part 2, fix event regions

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

LGTM
Attachment #8654433 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8654434 - Flags: review?(bugmail.mozilla)
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 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/)
Attachment #8653157 - Flags: review?(mstange)
(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.
(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.
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.
Attachment #8655093 - Flags: review?(mstange)
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 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).
Attachment #8655093 - Flags: review?(mstange) → review+
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.")
Attached patch test caseSplinter Review
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.
Attachment #8654434 - Attachment is obsolete: true
Attachment #8656175 - Flags: review?(bugmail.mozilla)
Comment on attachment 8656175 [details] [diff] [review]
test case

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

Thanks!
Attachment #8656175 - Flags: review?(bugmail.mozilla) → review+
Blocks: 1192581
No longer blocks: 1192581
Duplicate of this bug: 1192581
Depends on: 1192581
You need to log in before you can comment on or make changes to this bug.