1119942 - Notification scroller on lockscreen doesn't scroll properly with APZ enabled in root process

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

With APZ enabled in the root process, the notification scroller on the lockscreen doesn't scroll properly. To get this notification scroller to even be visible you have to have a bunch of uncleared notifications (you can get these by taking many screenshots on-device) and then rebooting the device to the lockscreen.

When this container scrolls it has a mask layer applied to it for the fade-out effect on the top and bottom edges, and this mask layer seems to mess with the event regions. This makes it non-scrollable. Probably related to bug 1071969 so marking it as a dependency for now, but there might be more stuff here that needs fixing.

Comment 1

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

Also a useful tip from sfoster on IRC: build gaia with DEVICE_DEBUG=1 so that you can connect WebIDE without the confirmation prompt. Otherwise you need to unlock the device to get to the confirmation prompt, but unlocking the device will remove the scroller when you go back to the lockscreen.

Comment 2

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

I've been looking into this and not making a huge amount of progress. It doesn't appear to be related to mask layers as far as I can tell, but is related to a frame being both scrollable and having a mask. The display list seem ends up with an SVGEffects display item wrapping the scrollable content (including a ScrollInfoLayer item) but then none of the layers in the layer tree have any FrameMetrics at all. We must be falling through a crack somewhere, trying to track that down.

Comment 3

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

Attachment: Dump of frametree, display list, layer tree

The display item 0xad542710 is the one with the mask and that is overflow:scroll, but ends up not being scrollable.

Comment 4

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

Attachment: Part 1 - Add missing logging

Seems like display items that go into a nested FLB don't end up getting logged properly as part of the FLB_LOG_ stuff.

Comment 5

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

Attachment: Part 2 - WIP

Based on what I can tell the problem appears to be that the SVGEffects display item never produces an active layer, even if it corresponds to an nsGfxScrollFrame and is being actively scrolled. This seems to kinda sorta fix the problem although there's still some choppiness with the scrolling which might be something else. Matt, any thoughts on what the right thing to do here is?

Comment 6

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Based on what I can tell the problem appears to be that the SVGEffects
> display item never produces an active layer, even if it corresponds to an
> nsGfxScrollFrame and is being actively scrolled. This seems to kinda sorta
> fix the problem although there's still some choppiness with the scrolling
> which might be something else. Matt, any thoughts on what the right thing to
> do here is?

The choppiness with the scrolling was just because of the copious amounts of logging I had enabled. With this patch the scrolling is fine, however the mask that is supposed to be applied doesn't get applied.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Created attachment 8548305 [details] [diff] [review]
> Part 2 - WIP
> 
> Based on what I can tell the problem appears to be that the SVGEffects
> display item never produces an active layer, even if it corresponds to an
> nsGfxScrollFrame and is being actively scrolled. This seems to kinda sorta
> fix the problem although there's still some choppiness with the scrolling
> which might be something else. Matt, any thoughts on what the right thing to
> do here is?

The problem is that we don't support SVG masks in the compositor (yet), so we can't build an active layer for this.

The right long term solution is to implement this (along with SVG clipping, blend modes, -moz-element and anything else we don't support in the compositor).

Short term, we probably want to block async scrolling of this element (since we can't) and let the main thread handle it.

Comment 8

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

Ok, so the way to fall back to main thread scrolling is to add the area to a dispatch-to-content region on the layer. I'll see if I can implement that for frames with SVG masks.

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Ok, so the way to fall back to main thread scrolling is to add the area to a
> dispatch-to-content region on the layer. I'll see if I can implement that
> for frames with SVG masks.

Actually that would only work if we could layerize the scrolling content, which we can't do here. Scrolling is always driven from the APZ so I feel like what we need here is the equivalent of a scrollinfo layer in order to force an APZ to be created (except of course with event-regions the APZ code ignores scrollinfo layers). I need to think about this a bit more.

Comment 10

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

Attachment: (Attempt 2) Part 2 - WIP

Comment 11

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

Attachment: (Attempt 2) Part 3 - WIP

Comment 12

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

In a nutshell what this does is take a display list that looks like this:


.. stuff1 ..
SVGEffects
  .. stuff2 ..
  ScrollInfoLayer
.. stuff3 ..

and turn it into this:

.. stuff1 ..
SVGEffects
 .. stuff2 ..
ScrollInfoLayer
.. stuff3 ..

This allows the empty ContainerLayer to get built and NOT get thrown away, and so the APZ can drive scrolling via the main thread (i.e. it generates an APZ with a valid scrollid and makes scrollTo calls and so on, but the async transforms get applied to the empty container layer which is a no-op).

Comment 13

[Markus Stange [:mstange]]

Are we already doing something similar for inactive opacity? Or how does that work?

Comment 14

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

(In reply to Markus Stange [:mstange] from comment #13)
> Are we already doing something similar for inactive opacity? Or how does
> that work?

Most likely that doesn't work. As far as I know it would basically be the same problem as with this bug. I can try to write a testcase for it tomorrow.

Comment 15

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

Oh! Actually most likely for opacity we don't have this problem because it will take its children into account at [1]. So if there's a scrollable frame inside an opacity thing it will activate itself. The SVGEffects item doesn't do that (and what my initial WIP attempted to do, but we can't do SVGEffects in the compositor).

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=650de89062f6#3779

Comment 16

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> So if there's a scrollable frame
> inside an opacity thing it will activate itself.

But only if that scrollable frame is actively scrolled, no? How does it know to activate the scrollframe in the first place?
(This line of thinking probably doesn't bring us closer to a solution, so maybe we should continue this conversation somewhere else.)

Comment 17

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #16)
> 
> But only if that scrollable frame is actively scrolled, no? How does it know
> to activate the scrollframe in the first place?
> (This line of thinking probably doesn't bring us closer to a solution, so
> maybe we should continue this conversation somewhere else.)

This is the same issue I believe.

An inactive scrollframe within an inactive opacity needs to dispatch events to content in order to handle the first scroll.

The difference is after that, where the opacity will become active and we can start async scrolling, and the svg effects will remain inactive and we have to dispatch to content for all scrolls.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Adding roc and dvander to the cc list, since they've both recently looked at this stuff too.

Comment 19

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

Yeah, what Matt said. We activate the scrollframe once the user touches it in TabChild at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=66e037e5ff7d#2617. This makes it layerize the opacity. APZ knows just enough via dispatch-to-content regions to wait for the layerization to take effect before scrolling. If it doesn't layerize then it won't scroll (which is what the patches here are trying to address).

Comment 20

[Markus Stange [:mstange]]

Oh! Nice. Ok.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8549911 [details] [diff] [review]
(Attempt 2) Part 3 - WIP

As I mentioned to you this might have a problem where we take a hoisted item when inside an inactive layer inside the current inactive layer. Putting the hoisted items on the parent container state directly would avoid this.

Comment 22

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

Attachment: Part 2 - Disable scrollinfo layers if event-regions are enabled

Comment 23

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

Attachment: Part 3 - Hoist scrollinfo items out of inactive layers

This one corrects the issue tn pointed out with the previous patch. Also cleaned up a bit, ready for review.

Comment 24

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

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6311902485

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8551503 [details] [diff] [review]
Part 3 - Hoist scrollinfo items out of inactive layers

Add some assertions to make sure that mHoistedItems is empty when the container state object is destructed and after ProcessDisplayItems finishes.

In FrameLayerBuilder::Init call the parameter aContainingContainerState. Actually, "parent container state" might be easier to understand. I realize you just used the same word as "containing painted layer" though.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8551503 [details] [diff] [review]
Part 3 - Hoist scrollinfo items out of inactive layers

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

I've now read the worst hoisted too many times, and am no longer convinced that it's a real word.

Comment 27

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

(In reply to Timothy Nikkel (:tn) from comment #25)
> Comment on attachment 8551503 [details] [diff] [review]
> Part 3 - Hoist scrollinfo items out of inactive layers
> 
> Add some assertions to make sure that mHoistedItems is empty when the
> container state object is destructed and after ProcessDisplayItems finishes.

Added.

> In FrameLayerBuilder::Init call the parameter aContainingContainerState.

Renamed.

> Actually, "parent container state" might be easier to understand. I realize
> you just used the same word as "containing painted layer" though.

Yeah I wanted to keep it parallel with "containing painted layer".

Hoisted the patches onto inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/61c1a3f2bdab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37a5836d9779
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9302dbf3d84b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61c1a3f2bdab
https://hg.mozilla.org/mozilla-central/rev/37a5836d9779
https://hg.mozilla.org/mozilla-central/rev/9302dbf3d84b

Comment 29

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

Comment on attachment 8548304 [details] [diff] [review]
Part 1 - Add missing logging

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 950934 is what i would like this for
User impact if declined: if we turn on APZ in the B2G root process (bug 950934, which is 2.2+) then the list of notifications on the lockscreen becomes unscrollable after the first scroll. however this is just a specific instance of a more general problem that might affect content processes as well - any scrollable element inside an SVG masked element would not be properly scrollable without this patch.
Testing completed: locally
Risk to taking this patch (and alternatives if risky): fairly low risk I think
String or UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c5cacb2b5a76
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ff9fc4297955
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3bf70dcd767f