Closed
Bug 1266799
Opened 8 years ago
Closed 8 years ago
[e10s] Investigate slow fullscreen transition in content DOM for Yahoo music player
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mconley, Assigned: mconley)
References
(Depends on 1 open bug)
Details
(Whiteboard: btpp-followup-2016-05-27)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
xidorn
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
Spun out from bug 1264091. We seem to get into a state where when the user triggers DOM fullscreen, the browser UI does the full screen transition, and once it completes, there is a gap of time where the web content does not go full screen, which is rather jarring. According to Xidorn, there is a full-screen-api.transition.timeout that we must be hitting, which is causing the transition in the browser UI to complete in order to remain responsive to the user. We should find out what is happening in the content process that is causing it to be slow at doing the DOM fullscreen transition.
Assignee | ||
Comment 1•8 years ago
|
||
This is supplanting bug 1264091 as an e10s M9.
tracking-e10s:
--- → m9+
Comment 2•8 years ago
|
||
Could this have something to do with resize perf? Related bugs - Bug 1255968 - Interruptible reflow is broken in e10s Bug 1257869 - Window Resize of large page takes seconds to change viewport
Updated•8 years ago
|
Flags: needinfo?(mconley)
Assignee | ||
Comment 4•8 years ago
|
||
testcase |
I believe this is caused by some JS on the Yahoo player page that's reacting to the mozfullscreenchange. Here's a more minimal test case: 1) Visit https://output.jsbin.com/lituremiga 2) Start the video 3) Click on the fullscreen button to enter fullscreen 4) Once the transition completes, click on it again to exit fullscreen. I've added a busy function for the mozfullscreenchange event, and that's blocking the content from completing its transition in time in the e10s case. In the non-e10s case, the busy loop causes the whole browser to wait inside the "dark" fullscreen transition. Neither is amazing, but perhaps the latter is preferable.
Flags: needinfo?(mconley)
Comment 5•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4) > I believe this is caused by some JS on the Yahoo player page that's reacting > to the mozfullscreenchange. > > Here's a more minimal test case: > > 1) Visit https://output.jsbin.com/lituremiga > 2) Start the video > 3) Click on the fullscreen button to enter fullscreen > 4) Once the transition completes, click on it again to exit fullscreen. > > I've added a busy function for the mozfullscreenchange event, and that's > blocking the content from completing its transition in time in the e10s case. > > In the non-e10s case, the busy loop causes the whole browser to wait inside > the "dark" fullscreen transition. > > Neither is amazing, but perhaps the latter is preferable. This is arguable. There are certain users hate to be thrown in the dark for a long time, and want us to unblack the screen even if it is in an intermediate state. The mozfullscreenchange event is dispatched inside refresh driver tick, and there is no guarantee that such tick would ever complete (e.g. if page is closed inside the tick), so there must be some kind of timeout anyway.
Comment 7•8 years ago
|
||
This feel like an evangelism bug to me, and possibly a dev-doc-needed bug requesting that our MDN docs explicitly state you should not block on mozfullscreenchange. https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API
Comment 8•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #7) > This feel like an evangelism bug to me, and possibly a dev-doc-needed bug > requesting that our MDN docs explicitly state you should not block on > mozfullscreenchange. > > https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API It seems this page really would need an update. We've unprefixed this API in Firefox 47.
Comment 9•8 years ago
|
||
Relevant spec: https://fullscreen.spec.whatwg.org/#api The event timing is actually defined in the spec to happen "As part of the next animation frame task" after we "Resize pending's top-level browsing context's document's viewport's dimensions to match the dimensions of the screen of the output device."
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Chris H-C :chutten from comment #9) > Relevant spec: https://fullscreen.spec.whatwg.org/#api > > The event timing is actually defined in the spec to happen "As part of the > next animation frame task" after we "Resize pending's top-level browsing > context's document's viewport's dimensions to match the dimensions of the > screen of the output device." Classic spec. So my reading on this is that we're supposed to be firing this event _after_ we've done the fullscreen transition in the content area, no? Xidorn, do you know?
Flags: needinfo?(bugzilla)
Comment 11•8 years ago
|
||
The "next animation frame task" after resize is exact the first frame we are in fullscreen, which we really want to finish before we unblack the screen. The spec is written that way to ensure that there is no intermediate state between non-fullscreened and fully-fullscreened.
Flags: needinfo?(bugzilla)
Assignee | ||
Comment 12•8 years ago
|
||
So we're firing the event too early? (Sorry if I'm coming off as dense - just trying to be absolutely clear)
Flags: needinfo?(bugzilla)
Comment 13•8 years ago
|
||
No, I believe we are firing the event at the right time. This event does block the first frame, which blocks unblacking.
Flags: needinfo?(bugzilla)
Assignee | ||
Comment 14•8 years ago
|
||
Interesting. Then I think other browsers might be playing by different rules. Take Chrome, for example. For some reason, the demo page I wrote for this bug doesn't work for it, but pick any YouTube video using the HTML5 video player. Open the Chrome devtools console, and put in: addEventListener("webkitfullscreenchange", function(e) { console.log("Got fullscreenchange"); var then = Date.now(); while(Date.now() - then < 2500) {}; console.log("Done spinning"); }); Press enter, and then fullscreen the YouTube video. The video goes fullscreen right away, though there is a slight border around it for the 2.5 seconds that we're running the busy loop. We also exit immediately, even though the content process is blocked for 2.5 seconds. Are they not to spec? Or are we?
Flags: needinfo?(bugzilla)
Comment 15•8 years ago
|
||
They don't, I believe. Blink doesn't follow the spec for quite a while. Making the next frame block on this event so that the script has a chance to fix anything specific to fullscreen before it is shown to users is intuitively reasonable.
Flags: needinfo?(bugzilla)
Comment 16•8 years ago
|
||
So is this now a TE bug?
Flags: needinfo?(mconley)
Whiteboard: btpp-followup-2016-05-10
Assignee | ||
Comment 17•8 years ago
|
||
I don't know if it is, to be honest. I just tested on Safari, and from the looks of things, they wait until the content transition is complete before doing this "zoom" effect, much like the application fullscreen effect. Exiting fullscreen shows a similar bit of jank while content sorts itself out - the zoom effect reverses, but the video consumes the entire browser space for a half a second or so before it can re-organize itself. Edge expands the application to consume the fullscreen, and then after a short pause, fullscreens the video. It's not too smooth. I don't know if this is a TE bug. This might be a UX bug - the effect in Firefox of fading down and fading up might be emphasizing the fact that content wasn't ready (since the fade up seems to imply "all set to go"). Perhaps this is a matter of increasing the timeout for fade-up slightly from its current setting. For example, if I set full-screen-api.transition.timeout to 1000 from its default of 500, the transition is beautifully done. I wonder if we could / should collect some Telemetry to determine how often the timeout is being hit, and if so, if there's an acceptable value of full-screen-api.transition.timeout that would accommodate most of our users without keeping them in the dark too long. ni?ing verdi, since he worked on bug 1129061.
Flags: needinfo?(mconley) → needinfo?(mverdi)
Comment 18•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17) > I just tested on Safari, and from the looks of things, they wait until the > content transition is complete before doing this "zoom" effect, much like > the application fullscreen effect. I wonder whether we can do this as well. I mean, at least start content side change at the beginning of transition, which would give us ~200ms more time to process things. What I'm concerned is animations on the page driven by the refresh driver. Are we fine to freeze them before the screen goes completely black?
Comment 19•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4) > > I've added a busy function for the mozfullscreenchange event, and that's > blocking the content from completing its transition in time in the e10s case. > > In the non-e10s case, the busy loop causes the whole browser to wait inside > the "dark" fullscreen transition. > > Neither is amazing, but perhaps the latter is preferable. The e10s case is horrible. It looks like Firefox is completely broken. The non-e10s case is not ideal but it just looks like we're a little slow, not completely broken. (In reply to Mike Conley (:mconley) - Needinfo me! from comment #17) > Perhaps this is a matter of increasing the timeout for fade-up slightly from > its current setting. For example, if I set > full-screen-api.transition.timeout to 1000 from its default of 500, the > transition is beautifully done. I'm not sure what the timing of that looks like. Is it faster or slower than the non-e10s case above? > > I wonder if we could / should collect some Telemetry to determine how often > the timeout is being hit, and if so, if there's an acceptable value of > full-screen-api.transition.timeout that would accommodate most of our users > without keeping them in the dark too long. > That seems reasonable if there isn't a way to prevent the problem to begin with.
Flags: needinfo?(mverdi)
Updated•8 years ago
|
Comment 20•8 years ago
|
||
triage note: telemetry probes just landed in nightly (bug 1271160), and should uplift to beta shortly.
Updated•8 years ago
|
Whiteboard: btpp-followup-2016-05-10 → btpp-followup-2016-05-27
Updated•8 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 21•8 years ago
|
||
The Telemetry probes suggest that a wait of about 1.17s should account for about 95% of all use cases. I'm going to suggest we just round that out to 1s, which should account for something like 93%-94% of use cases.
Assignee | ||
Comment 22•8 years ago
|
||
According to the Telemetry probes added in Bug 1271160, 1000ms should account for ~94% of fullscreen transitions. The remaining ~6% tail is where users might see the transition end and then content re-organize itself. I think this is a big improvement over the original 500ms, which covers only about ~80% of cases, according to Telemetry. Review commit: https://reviewboard.mozilla.org/r/57264/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57264/
Attachment #8759284 -
Flags: review?(bugzilla)
Comment 23•8 years ago
|
||
Comment on attachment 8759284 [details] Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms. https://reviewboard.mozilla.org/r/57264/#review54206
Attachment #8759284 -
Flags: review?(bugzilla) → review+
Comment 24•8 years ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/250ad26a44ea Increase DOM fullscreen timeout length to 1000ms. r=xidorn
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/250ad26a44ea
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8759284 [details] Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms. Approval Request Comment [Feature/regressing bug #]: e10s [User impact if declined]: Users that have e10s enabled have a greater probability of seeing an ugly full page re-layout after making some element go fullscreen (like a video). [Describe test coverage new/current, TreeHerder]: There are internal tests for fullscreen that are all passing. [Risks and why]: Very low risk - we're just bumping up a value in a pref to increase the amount of time we're willing to wait before finishing the fullscreen transition. [String/UUID change made/needed]: None.
Attachment #8759284 -
Flags: approval-mozilla-aurora?
Comment 27•8 years ago
|
||
Comment on attachment 8759284 [details] Bug 1266799 - Increase DOM fullscreen timeout length to 1000ms. Improve e10s support, taking it
Attachment #8759284 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 28•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1ca54e2ea78a
You need to log in
before you can comment on or make changes to this bug.
Description
•