1076783 - Setting background-color on screenshot-overlay breaks video controls in fullscreen mode

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[:gerard-majax]

As documented on bug 1071235 comment 24, we exposed that adding a 'background-color' statement, whatever the value, on ".appWindow.scrollable .screenshot-overlay" breaks displaying the video controls when in fullscreen and the user taps on the screen.

After investigating with Vivien, we believe this is exposing an underlying APZC issue. Layers dump taken when user taps on the video shows differences.

Comment 1

[:gerard-majax]

Attachment: Layers tree dump when tapping the video and the bug is reproduced

Comment 2

[:gerard-majax]

Attachment: Layers tree dump when tapping the video and the bug is NOT reproduced

Comment 3

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

Copying over flags from the other bug. I'll investigate this at some point today or tomorrow.

Comment 4

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

Actually based on the layer tree we're given I think the APZ code is doing the right thing. There's basically scrollinfo layer that is sitting on top of everything (that last ContainerLayer in the layer tree) and the APZC for that eats the events, preventing them from getting to content. I'm not sure why the layout code is generating a scrollinfo layer there.

Comment 5

[Tony Chung [:tchung]]

Can we get a fix for this and uplifted for 2.1?   it blocks full screen video controls in bug 1071235, which is a 2.1+ blocker.  Thanks.

Comment 6

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

Somebody from layout needs to look into this.

Comment 7

[Gregor Wagner [:gwagner]]

[Blocking Requested - why for this release]:
Blocking a blocker

Comment 8

[Gregor Wagner [:gwagner]]

Jet, is this your area?

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #8)
> Jet, is this your area?

Can't set the blocking bug because of circular dependencies but this is basically blocking bug 1071235.

Comment 10

[Andrew Overholt [:overholt]]

Blocking a blocker (well, a now-closed blocker but this is the right place to fix it).

Comment 11

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

tn, can you take a look at this?

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Actually based on the layer tree we're given I think the APZ code is doing
> the right thing. There's basically scrollinfo layer that is sitting on top
> of everything (that last ContainerLayer in the layer tree) and the APZC for
> that eats the events, preventing them from getting to content. I'm not sure
> why the layout code is generating a scrollinfo layer there.

Layout currently creates a scroll info layer for every non-root scrollframe that has WantAsyncScroll() == true, or if it has a displayport. What did we decide we wanted to do with scroll info layers?

Comment 14

[Timothy Nikkel (:tnikkel)]

Or is this in an area that we don't expect to be scrollable? It's not clear to me where this bug shows up.

Comment 15

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

I can reproduce this bug on a master build for Flame. You have to download the youtube app from the Marketplace, play a video, and then tap on the video and make it fullscreen. The fullscreen video should show overlay controls when you tap on it, but doesn't. The video takes up the entire screen area and should not be scrollable, so that's why I'm thinking the scroll info layer shouldn't be there. I'll grab a displaylist dump as well in case that helps you. Leaving needinfo on me until I get that.

Comment 16

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

Attachment: Parent process displaylist/layer dump

The scrollinfo layer is coming from the parent process which is a little surprising to me. Here's he displaylist/layer dump from the parent process. The full log that I extracted this from (which includes the child process output as well) is at http://people.mozilla.org/~kgupta/tmp/1076783.txt

Comment 17

[Tony Chung [:tchung]]

hey guys whats the status on this?  this is blocking video controls usage, and has been opened for a almost 2 weeks.   is this getting fixed soon?

Comment 18

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

It didn't have an assignee until yesterday. It will probably take a few more days before it is fixed.

Comment 19

[Tony Chung [:tchung]]

adding the proper keywords now, as this is really a smoketest blocker.   unfortuantely, our smoketest didnt include a step on checking full screen video controls, which is why it wasnt prioritized higher.  the smoketest steps have now been updated to identify this, so we'll be treating this fix with high importance.

If a regression window is needed, please feel free to mark qawanted and regressionwindow-wanted flag.   Thanks.

Comment 20

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

Thanks. I don't think we need a regressionwindow right now since we have one from bug 1071235 but the change that triggered this regression is not the root cause.

Comment 21

[Peter Bylenga [:PBylenga]]

I've updated the test case to now also test Fullscreen mode for smoketests for 2.1 and 2.2 (Master).

https://moztrap.mozilla.org/manage/case/6073/

Comment 22

[Timothy Nikkel (:tnikkel)]

We get a scroll info layer because we have a display port and also scrollgrab (either one is enough).

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Actually based on the layer tree we're given I think the APZ code is doing
> the right thing. There's basically scrollinfo layer that is sitting on top
> of everything (that last ContainerLayer in the layer tree) and the APZC for
> that eats the events, preventing them from getting to content.

What if the user taps on a button/link and doesn't want to start a scroll? Shouldn't these events not get eaten in general?

In bug 1041510 we moved scroll info layers to the top of the scrolled content to fix a bug with merging of scroll layer items and scroll info layer items. We don't have scroll layer items anymore. Moving scroll info layers back to the bottom seems like a good idea and I think it would probably fix this bug.

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #22)
> In bug 1041510 we moved scroll info layers to the top of the scrolled
> content to fix a bug with merging of scroll layer items and scroll info
> layer items. We don't have scroll layer items anymore. Moving scroll info
> layers back to the bottom seems like a good idea and I think it would
> probably fix this bug.

Indeed, that does fix it.

Comment 24

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

Attachment: APZC tree dumps

(In reply to Timothy Nikkel (:tn) from comment #22)
> We get a scroll info layer because we have a display port and also
> scrollgrab (either one is enough).
> 
> What if the user taps on a button/link and doesn't want to start a scroll?
> Shouldn't these events not get eaten in general?
> 

That's an interesting point. I looked at the APZC tree while just scrolling normally through the youtube app and while in full-screen mode, and it explains this discrepancy. I'm attaching the tree dumps. (Note that the tree dumps list siblings in reverse order, with the last sibling first).

In the first one (scrolling normally) there is a div in the parent process that has scrollgrab set on it, and that ends up setting scrollable metrics on three different layers (the ones labelled (1,3,3) - one of these is the scrollinfo layer). The child process has one scrollable layer (5,2,3) which is in the subtree of the RefLayer and so the APZC for that layer ends up a child of the APZC in the root process. Since APZ hit detection always uses the innermost APZC, all touch events in this case end up going to the child process.

In the second one (fullscreen) the scrollable metrics in the parent process does NOT get applied to any actual layers and we only have the scrollinfo layer. This results in a different APZC tree, where the two APZCs are siblings and the parent-process APZC is the later sibling. This means the APZ hit detection will end up finding the parent process APZC and that eats all the events.

When you move the scrollinfo layer to not be last, what that's effectively doing is swapping the order of the sibling APZCs in the fullscreen case, so that the child process APZC ends up receiving the touch events. Another fix might be to restore the scrollable metrics on the RefLayer, which would restore the parent-child relationship between the APZCs. However I'm not really sure what causes the scrollable metrics to be present (or not) on the RefLayer so I don't know how we can trigger that.

Comment 25

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

(In reply to Timothy Nikkel (:tn) from comment #22)
> What if the user taps on a button/link and doesn't want to start a scroll?
> Shouldn't these events not get eaten in general?

Also, in general the touch events don't get eaten by the APZC because of the way event routing works. It's just the mouse and click events that are getting eaten. So in theory this would also not happen if the fullscreen video controls in toolkit/content/widgets/videocontrols.xml relied entirely on touch events. For example, changing [1] from "mouseup" to "touchup" makes the video controls appear and disappear upon tapping while in fullscreen mode. The scrubber even works, since it uses touch evens. However the play/pause, mute, and fullscreen buttons in the video controls don't work because they still rely on click events rather than touchstart/touchend.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml?rev=8c0ced0c0ab1#1793

Comment 26

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

I gave this some more thought and while I could have the root process APZ not "eat" the mouse/click events I don't think that is a correct solution in this case. We might run into other scenarios where the root process APZ being on top of everything will cause issues, so it would be better to fix it either by ensuring the RefLayer has the same scrollable metrics as the scrollinfo layer, or by moving the scrollinfo layer to the back so it's not on top of everything else.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> I gave this some more thought and while I could have the root process APZ
> not "eat" the mouse/click events I don't think that is a correct solution in
> this case. We might run into other scenarios where the root process APZ
> being on top of everything will cause issues, so it would be better to fix
> it either by ensuring the RefLayer has the same scrollable metrics as the
> scrollinfo layer, or by moving the scrollinfo layer to the back so it's not
> on top of everything else.

You're right, I think the reflayer should be getting frame metrics.

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #27)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> > I gave this some more thought and while I could have the root process APZ
> > not "eat" the mouse/click events I don't think that is a correct solution in
> > this case. We might run into other scenarios where the root process APZ
> > being on top of everything will cause issues, so it would be better to fix
> > it either by ensuring the RefLayer has the same scrollable metrics as the
> > scrollinfo layer, or by moving the scrollinfo layer to the back so it's not
> > on top of everything else.
> 
> You're right, I think the reflayer should be getting frame metrics.

Nevermind. The reflayer is created by the iframe containing the remote process (content). When in full screen it is fixed position. It is correctly not getting frame metrics (it's not scrolled by anything). When it's not full screen it is inside of that scroll frame that has a displayport/scrollgrab because it is no longer fixed pos (so it should be getting framemetrics).

When it's fixed pos it still gets put into the scrolled content list (we descend into positioned items through their placeholders) and so the scroll info layer ends up being put on top of it.

So I guess the bug is just that we don't want scroll info layers to be on top of anything so they don't eat events? Is that right, kats? Or is there any other bug that we should be fixing here?

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: move scroll info layers down again

I'll just put an if here for the old way with scroll container layers even though that's dead code that I just haven't gotten around to removing yet.

(Noticed that the old code had a bug in it, the scroll info item should be put into the outlines list of the scrolledContent set, not aLists, but I'm just going to leave well enough alone.)

Comment 30

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

(In reply to Timothy Nikkel (:tn) from comment #28)
> So I guess the bug is just that we don't want scroll info layers to be on
> top of anything so they don't eat events? Is that right, kats? Or is there
> any other bug that we should be fixing here?

I thought that fixed position elements that are children of something scrollable would still get scrollable metrics and would also be marked as fixed position. The code in AsyncCompositionManager is supposed to prevent it from moving if the user tries to scroll. From the layer dump though it doesn't look like the RefLayer is flagged as fixed position. I'm not sure if that's a problem or not.

There's a few sanity checks i should run with the scroll info layer change to make sure it doesn't break subframe scrolling. I'll try to do that today.

Comment 31

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

Sanity checks seem to be ok.

I still feel like maybe the RefLayer should have both fixed-position flags and scrollable metrics, though. I ran into what I think is a devtools bug when I tried to see why the iframe was position:fixed so I'm still unsure about this.

Comment 32

[Timothy Nikkel (:tnikkel)]

We definitely don't give fixed pos stuff scroll metrics now. Not since multi layer apzc at least. Do we need scroll metrics on them? That would be confusing I think when we have fixed pos stuff in an iframe so that they do need the scroll metrics from scroll frames in the parent document.

Comment 34

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> Sanity checks seem to be ok.
> 
> I still feel like maybe the RefLayer should have both fixed-position flags
> and scrollable metrics, though. I ran into what I think is a devtools bug
> when I tried to see why the iframe was position:fixed so I'm still unsure
> about this.

fullscreen makes it position fixed I believe. I think it's this css rule
http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#261

Comment 35

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

(In reply to Timothy Nikkel (:tn) from comment #34)
> fullscreen makes it position fixed I believe. I think it's this css rule
> http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#261

I wasn't sure if :-moz-fullscreen applied just to the element that was fullscreened or the ancestor elements too. Also in the screenshot at https://bug1085119.bugzilla.mozilla.org/attachment.cgi?id=8507518 you can see that there's another rule that tries to make it position:absolute so I wasn't really sure what was going on.

But since this patch works, land it, and we can revisit if we run into other problems.

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #35)
> (In reply to Timothy Nikkel (:tn) from comment #34)
> > fullscreen makes it position fixed I believe. I think it's this css rule
> > http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#261
> 
> I wasn't sure if :-moz-fullscreen applied just to the element that was
> fullscreened or the ancestor elements too. Also in the screenshot at
> https://bug1085119.bugzilla.mozilla.org/attachment.cgi?id=8507518 you can
> see that there's another rule that tries to make it position:absolute so I
> wasn't really sure what was going on.

The position: fixed rule in ua.css is !important so it will override the position absolute that the element has normally when not in full screen.

> But since this patch works, land it, and we can revisit if we run into other
> problems.

Will do.

Comment 37

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4f4862589d

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d4f4862589d

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for b2g34 approval when you get a chance.

Comment 40

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8507496 [details] [diff] [review]
move scroll info layers down again

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 #): I think a gaia change exposed an existing gecko issue, I'm not totally clear on the history.
User impact if declined: can't use video controls in youtube fullscreen
Testing completed: kats and I tested the patch, on mozilla-central now.
Risk to taking this patch (and alternatives if risky): should be pretty low risk
String or UUID changes made by this patch: none

Comment 41

[bhavana bajaj [:bajaj]]

ryan/:tn,

Can either of you please land this on b2g34 repo asap so this can get into 2.1 nightlies  which get kicked off at 4pm PT. I'd hate two see continuous smokestest failure 3 days in a row.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/564e2cda4a6d

Comment 43

[Martin Shuman [:Marty]]

Verified fixed on the latest Flame 2.1 build.
Video controls appear and function correctly in Full Screen mode.

Environmental Variables:
Device: Flame 2.1
BuildID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

--------------------------------------------------------

Currently blocked from verifying on Flame 2.2 by bug 1085787.

Comment 45

[Andrew Sutherland [:asuth] (he/him)]

Regression-window hunting at https://bugzilla.mozilla.org/show_bug.cgi?id=1087453#c6 has identified this bug as the cause of that bug.

Comment 46

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2.
The control appears and is fully functional on fullscreen mode on YouTube. The fullscreen is not scrollable as expected. 

Flame 2.2 

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023040204
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: 88adcf8fef83
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0