rdar://problem/19823121 The issue is that we still call "hideControls" while we are hovering, which leaves us in a state of thinking the controls are hidden while they are actually visible. I attempted to remedy the issue by detecting for hover over the controls using mouse over and out events but this was unreliable. Another suggestion was to rip out the controlsAreHidden/forceUpdate logic in the updateTime and updateProgress as it doesn't seem to offer us much of a performance benefit anyways.
Created attachment 252641 [details] patch
Created attachment 252648 [details] alternate patch
Are we regressing https://bugs.webkit.org/show_bug.cgi?id=137763 on purpose?
Created attachment 252649 [details] alternate patch
(In reply to comment #3) > Are we regressing https://bugs.webkit.org/show_bug.cgi?id=137763 on purpose? Yes. That patch caused a number of behavior regressions which require workarounds whose implementations negate the benefits of that patch. If we want to reduce unnecessary painting, we should make sure instead that the hidden controls do not have renderers.
Comment on attachment 252649 [details] alternate patch Obsoleting the alternate patch, found a case where it doesn't work and just makes the controls appear forever.
(In reply to comment #5) > (In reply to comment #3) > > Are we regressing https://bugs.webkit.org/show_bug.cgi?id=137763 on purpose? > > Yes. That patch caused a number of behavior regressions which require > workarounds whose implementations negate the benefits of that patch. > > If we want to reduce unnecessary painting, we should make sure instead that > the hidden controls do not have renderers. Right now we just opacity 0 the controls. I'm not sure why would be be doing extra painting when opacity is 0 but if so, then we could go a step further and display:none the controls after the transition animation has ended.
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > Are we regressing https://bugs.webkit.org/show_bug.cgi?id=137763 on purpose? > > > > Yes. That patch caused a number of behavior regressions which require > > workarounds whose implementations negate the benefits of that patch. > > > > If we want to reduce unnecessary painting, we should make sure instead that > > the hidden controls do not have renderers. > > Right now we just opacity 0 the controls. > I'm not sure why would be be doing extra painting when opacity is 0 but if > so, then we could go a step further and display:none the controls after the > transition animation has ended. After the opacity animation completes, we set the controls to display:none. I've verified that on current nightlies, the render tree doesn't have renderers for the media controls elements. So there may not be much extra work to do here.
Simon says that there are 2 things we're trying to avoid doing are a) painting (which we've dealt with already via display:none) b) css style recalls He also says to touch your nose. I've been trying to get query selector to work to detect the :hover property on the control panel, but to no avail. Maybe it's another shadow dom issue.
Comment on attachment 252641 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=252641&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:288 > + // When unhiding the controls, the offsetWidth and offsetHeight are briefly 0. > + // This causes the timeline to not update properly. > + // Make sure we retain our old timeline metrics state if this is the case. > + if (this.timelineMetricsNeedsUpdate && this.controls.timeline.offsetWidth && this.controls.timeline.offsetHeight) { This is not a great strategy. We need to do the update when we know it is correct to do it. It seems possible that a value of 0 is not the only kind of incorrect value we might see if we look at the wrong time.
Created attachment 253224 [details] patch
Comment on attachment 253224 [details] patch Attachment 253224 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5440415067013120 New failing tests: media/track/track-insert-after-load-crash.html http/tests/contentextensions/text-track-blocked.html media/track/track-delete-during-setup.html http/tests/media/hls/hls-audio-tracks-has-audio.html media/track/track-mode-triggers-loading.html media/track/media-element-enqueue-event-crash.html http/tests/media/hls/hls-audio-tracks-locale-selection.html media/track/track-remove-by-setting-innerHTML.html http/tests/media/hls/hls-audio-tracks.html media/track/track-kind.html media/track/track-cue-rendering-horizontal.html http/tests/media/video-redirect.html media/track/track-mode-not-changed-by-new-track.html
Created attachment 253226 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 253224 [details] patch Attachment 253224 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5094411964776448 New failing tests: media/track/track-insert-after-load-crash.html http/tests/media/hls/hls-audio-tracks.html media/track/track-delete-during-setup.html http/tests/media/hls/hls-audio-tracks-has-audio.html media/track/track-mode-triggers-loading.html media/track/media-element-enqueue-event-crash.html http/tests/media/hls/hls-audio-tracks-locale-selection.html media/track/track-remove-by-setting-innerHTML.html media/track/track-kind.html media/track/track-cue-rendering-horizontal.html http/tests/media/video-redirect.html media/track/track-mode-not-changed-by-new-track.html
Created attachment 253228 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 253255 [details] patch
Looks like the patch doesn't apply. Other than that, the approach looks good.
Created attachment 253262 [details] patch
Comment on attachment 253262 [details] patch Attachment 253262 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6121600638255104 New failing tests: media/track/track-cue-rendering-horizontal.html
Created attachment 253264 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 253262 [details] patch Attachment 253262 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5724196441161728 New failing tests: media/track/track-cue-rendering-horizontal.html http/tests/contentextensions/text-track-blocked.html
Created attachment 253265 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 253262 [details] patch This is making some tests fail. Need to upload a new version that does not have that effect.
Created attachment 253390 [details] patchh
Comment on attachment 253390 [details] patchh View in context: https://bugs.webkit.org/attachment.cgi?id=253390&action=review > Source/WebCore/ChangeLog:12 > + It should only happen when the mouse leaves the video entirely. What about fullscreen video? > Source/WebCore/ChangeLog:17 > + remove controls entirely from the tree. Why?
(In reply to comment #25) > Comment on attachment 253390 [details] > patchh > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253390&action=review > > > Source/WebCore/ChangeLog:12 > > + It should only happen when the mouse leaves the video entirely. > > What about fullscreen video? This is also true of fullscreen video. If your mouse leaves the fullscreen video (I guess this would happen with multiple monitors) the controls should disappear. If you only have one screen then they will always appear as soon as you move the mouse anywhere on the screen and can only disappear as a result of no mouse movement for a certain amount of time. > > > Source/WebCore/ChangeLog:17 > > + remove controls entirely from the tree. > > Why? This is the strategy that Simon, Jer and I finally settled on for avoiding unnecessary paints and style recalcs, simply to remove the controls entirely when they are hidden and re-add them back in when they are shown. This way we can call updateTime and progress as much as we want while the controls are hidden with none of those performance penalties.
Comment on attachment 253390 [details] patchh View in context: https://bugs.webkit.org/attachment.cgi?id=253390&action=review r=me, with nits. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:210 > + controlsAreHidden: function() > + { > + // Controls are only ever actually hidden when they are removed from the tree > + return !this.controls.panelContainer.parentElement; > + }, > + It's unfortunate that we have to duplicate this work in the iOS controls. Can we refactor out this.controls.panelContainer into this.controls.panel, and remove this method? > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:397 > + handlePanelTransitionEnd: function(event) > + { > + var opacity = window.getComputedStyle(this.controls.panel).opacity; > + if (!parseInt(opacity) && !this.controlsAlwaysVisible()) { > + this.base.removeChild(this.controls.inlinePlaybackPlaceholder); > + this.base.removeChild(this.controls.panelContainer); > + } > + }, > + Ditto. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:562 > + showControls: function() > + { > + this.updateShouldListenForPlaybackTargetAvailabilityEvent(); > + if (this.showInlinePlaybackPlaceholderOnly()) > + return; > + > + this.updateForShowingControls(); > + if (this.shouldHaveControls()) { > + this.base.appendChild(this.controls.inlinePlaybackPlaceholder); > + this.base.appendChild(this.controls.panelContainer); > + } > + }, > + Ditto.
Will land refactoring in a followup patch.
Looks like this caused assertions all over the place: https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r184599%20(11871)/results.html
This is forcing layout at a bad time, but I thought jer made didAddUserAgentShadowRoot async?
(In reply to comment #30) > This is forcing layout at a bad time, but I thought jer made > didAddUserAgentShadowRoot async? That patch was never landed; it caused layout test regressions of its own.
Landed same patch rolling out part of my previous change to the controlsAreHidden method, which should prevent updateTimelineMetricsIfNeeded from querying offsetWidth. Unfortunately this seems to be a timing issue and does not repro on my machine and there don't appear to be debug EWS bots so I'll have to hope for the best here: http://trac.webkit.org/changeset/184667
Missed a check in showControls in r184667 that broke scrubbing. Fixed it here in a followup patch here: http://trac.webkit.org/changeset/184682 Refactoring patch still incoming...
r184682 broke media/media-controls-timeline-updates.html on WebKit2, which times out now: <>https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fmedia-controls-timeline-updates.html>. Rolling out both patches.
(In reply to comment #34) > r184682 broke media/media-controls-timeline-updates.html on WebKit2, which > times out now: > <>https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=media%2Fmedia-controls-timeline-updates.html>. > > Rolling out both patches. Did http://trac.webkit.org/changeset/184667 break any tests?
Created attachment 253508 [details] patch
http://trac.webkit.org/changeset/184722
The refactoring is more obnoxious than I expected. I'll do that under a separate bug.
Comment on attachment 253508 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=253508&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:828 > + if (!parseInt(opacity) && !this.controlsAlwaysVisible()) { > + this.base.removeChild(this.controls.inlinePlaybackPlaceholder); > + this.base.removeChild(this.controls.panel); This change has regressed accessibility (bug 145684) for VoiceOver and Switch Control (probably Orca, too). Once you start playing a video the controls disappear and you can't get them back without a mouse hover.