Closed
Bug 1119942
Opened 10 years ago
Closed 10 years ago
Notification scroller on lockscreen doesn't scroll properly with APZ enabled in root process
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
BenWa
:
review+
mattwoodrow
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
botond
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
mattwoodrow
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
The display item 0xad542710 is the one with the mask and that is overflow:scroll, but ends up not being scrollable.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8548304 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8548304 -
Flags: review?(matt.woodrow) → review+
Comment 7•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8548305 -
Flags: feedback?(matt.woodrow) → feedback-
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8548305 -
Attachment is obsolete: true
Attachment #8549910 -
Flags: feedback?(tnikkel)
Attachment #8549910 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8549911 -
Flags: feedback?(tnikkel)
Attachment #8549911 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Are we already doing something similar for inactive opacity? Or how does that work?
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
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•10 years ago
|
||
(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.)
Comment 17•10 years ago
|
||
(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•10 years ago
|
||
Adding roc and dvander to the cc list, since they've both recently looked at this stuff too.
Assignee | ||
Comment 19•10 years ago
|
||
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•10 years ago
|
||
Oh! Nice. Ok.
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
Comment 25•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8551501 -
Flags: review?(tnikkel) → review+
Comment 26•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8551501 -
Flags: review?(botond) → review+
Assignee | ||
Comment 27•10 years ago
|
||
(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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61c1a3f2bdab
https://hg.mozilla.org/mozilla-central/rev/37a5836d9779
https://hg.mozilla.org/mozilla-central/rev/9302dbf3d84b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 29•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8551501 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8551503 -
Flags: approval-mozilla-b2g37?
Updated•10 years ago
|
Attachment #8548304 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8551501 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8551503 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 30•10 years ago
|
||
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
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox36:
--- → wontfix
status-firefox37:
--- → wontfix
status-firefox38:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•