Closed Bug 1119942 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
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.
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.
Assignee: nobody → bugmail.mozilla
No longer depends on: 1071969
The display item 0xad542710 is the one with the mask and that is overflow:scroll, but ends up not being scrollable.
Seems like display items that go into a nested FLB don't end up getting logged properly as part of the FLB_LOG_ stuff.
Attachment #8548304 - Flags: review?(matt.woodrow)
Attachment #8548304 - Flags: review?(bgirard)
Attached patch Part 2 - WIP (obsolete) — Splinter Review
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?
Attachment #8548305 - Flags: feedback?(matt.woodrow)
Attachment #8548304 - Flags: review?(bgirard) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8548304 - Flags: review?(matt.woodrow) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8548305 - Flags: feedback?(matt.woodrow) → feedback-
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch (Attempt 2) Part 2 - WIP (obsolete) — Splinter Review
Attachment #8548305 - Attachment is obsolete: true
Attachment #8549910 - Flags: feedback?(tnikkel)
Attachment #8549910 - Flags: feedback?(matt.woodrow)
Attached patch (Attempt 2) Part 3 - WIP (obsolete) — Splinter Review
Attachment #8549911 - Flags: feedback?(tnikkel)
Attachment #8549911 - Flags: feedback?(matt.woodrow)
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).
Are we already doing something similar for inactive opacity? Or how does that work?
(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.
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.)
(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.
Adding roc and dvander to the cc list, since they've both recently looked at this stuff too.
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).
Oh! Nice. Ok.
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.
Attachment #8549910 - Attachment is obsolete: true
Attachment #8549910 - Flags: feedback?(tnikkel)
Attachment #8549910 - Flags: feedback?(matt.woodrow)
Attachment #8551501 - Flags: review?(tnikkel)
Attachment #8551501 - Flags: review?(botond)
This one corrects the issue tn pointed out with the previous patch. Also cleaned up a bit, ready for review.
Attachment #8549911 - Attachment is obsolete: true
Attachment #8549911 - Flags: feedback?(tnikkel)
Attachment #8549911 - Flags: feedback?(matt.woodrow)
Attachment #8551503 - Flags: review?(tnikkel)
Attachment #8551503 - Flags: review?(matt.woodrow)
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.
Attachment #8551503 - Flags: review?(tnikkel) → review+
Attachment #8551501 - Flags: review?(tnikkel) → review+
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.
Attachment #8551503 - Flags: review?(matt.woodrow) → review+
Attachment #8551501 - Flags: review?(botond) → review+
(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 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
Attachment #8548304 - Flags: approval-mozilla-b2g37?
Attachment #8551501 - Flags: approval-mozilla-b2g37?
Attachment #8551503 - Flags: approval-mozilla-b2g37?
Attachment #8548304 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551501 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551503 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.