Bug 144770 – Media Controls stop update after hovering for a few seconds

• Component: Media
• Status: RESOLVED FIXED

[Roger Fong]

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.

[Roger Fong]

Created attachment 252641 [details]
patch

[Roger Fong]

Created attachment 252648 [details]
alternate patch

[Simon Fraser (smfr)]

Are we regressing https://bugs.webkit.org/show_bug.cgi?id=137763 on purpose?

[Roger Fong]

Created attachment 252649 [details]
alternate patch

[Jer Noble]

(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.

[Roger Fong]

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.

[Roger Fong]

(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.

[Jer Noble]

(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.

[Roger Fong]

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.

[Darin Adler]

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.

[Roger Fong]

Created attachment 253224 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Roger Fong]

Created attachment 253255 [details]
patch

[Jer Noble]

Looks like the patch doesn't apply. Other than that, the approach looks good.

[Roger Fong]

Created attachment 253262 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

Comment on attachment 253262 [details]
patch

This is making some tests fail. Need to upload a new version that does not have that effect.

[Roger Fong]

Created attachment 253390 [details]
patchh

[Jon Lee]

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?

[Roger Fong]

(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.

[Jer Noble]

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.

[Roger Fong]

Will land refactoring in a followup patch.

[Alexey Proskuryakov]

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

[Simon Fraser (smfr)]

This is forcing layout at a bad time, but I thought jer made didAddUserAgentShadowRoot async?

[Jer Noble]

(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.

[Roger Fong]

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

[Roger Fong]

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...

[Alexey Proskuryakov]

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.

[Roger Fong]

(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?

[Roger Fong]

Created attachment 253508 [details]
patch

[Roger Fong]

http://trac.webkit.org/changeset/184722

[Roger Fong]

The refactoring is more obnoxious than I expected. I'll do that under a separate bug.

[James Craig]

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.