Bug 169145 – [Modern Media Controls] Improve media documents across macOS, iPhone and iPad

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

We should look at improving handling of media documents across macOS, iPhone and iPad, where macOS and iPad would behave similarly and iPhone would use the full width available to display media.

[Antoine Quint]

<rdar://problem/17048858>

[Antoine Quint]

Created attachment 303339 [details]
Patch

[Dean Jackson]

Comment on attachment 303339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303339&action=review

> Source/WebCore/ChangeLog:33
> +        As a result of the new styles applied by the modern-media-controls module, media documents have a
> +        similar behavior on macOS and iPad, where we only enforce a min-width for video allowing them
> +        to play at their natural size otherwise, and enforcing a fixed width for audio. On iPhone however,
> +        we want to always play the media at full width, with some padding in the case of audio.
> +

Did you check all this against the spreadsheet in Quip?

> Source/WebCore/Modules/modern-media-controls/controls/media-document.css:45
> +:host(.media-document.audio.ipad) {
> +    width: 650px !important;
> +}

What about iPads with different widths? Should it be the padding that is constant, or the overall width?

> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:73
> +        const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY;
> +        const hasVideoTracks = this.media.videoWidth != 0;
> +        return !isLiveBroadcast && !hasVideoTracks;

I don't like that all live broadcasts get a video player. I can imagine live "radio" being fairly popular - bringing radio to the internet.

Could we swap dynamically?

> Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js:60
> +        media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform);

Ugggh. I don't like this. Could we get it from the mediaController instead?

> Source/WebCore/html/MediaDocument.cpp:-113
> -    if (RuntimeEnabledFeatures::sharedFeatures().modernMediaControlsEnabled()) {
> -        StringBuilder bodyStyle;
> -        bodyStyle.appendLiteral("margin: 0; padding: 0;");
> -        bodyStyle.appendLiteral("background-color: rgb(38, 38, 38);");
> -        bodyStyle.appendLiteral("display: flex; justify-content: center; align-items: center;");
> -        body->setAttribute(styleAttr, bodyStyle.toString());
> -    }

Goodbye Dean code.

> Source/WebCore/html/MediaDocument.cpp:113
> +    videoElement->setAttributeWithoutSynchronization(playsinlineAttr, emptyAtom);

Do we want this on iPhone?

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 303339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303339&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/media-document.css:45
> > +:host(.media-document.audio.ipad) {
> > +    width: 650px !important;
> > +}
> 
> What about iPads with different widths? Should it be the padding that is
> constant, or the overall width?

Always the same width on all iPads, with a max-width of 100%.

> > Source/WebCore/Modules/modern-media-controls/media/media-controller.js:73
> > +        const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY;
> > +        const hasVideoTracks = this.media.videoWidth != 0;
> > +        return !isLiveBroadcast && !hasVideoTracks;
> 
> I don't like that all live broadcasts get a video player. I can imagine live
> "radio" being fairly popular - bringing radio to the internet.
> 
> Could we swap dynamically?

There is no way we can determine that HLS is ever audio-only. Video tracks could appear at any moment I'm afraid.

> > Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js:60
> > +        media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform);
> 
> Ugggh. I don't like this. Could we get it from the mediaController instead?

We could add those as layout traits, would that be good?

> > Source/WebCore/html/MediaDocument.cpp:113
> > +    videoElement->setAttributeWithoutSynchronization(playsinlineAttr, emptyAtom);
> 
> Do we want this on iPhone?

I think so.

[Build Bot]

Comment on attachment 303339 [details]
Patch

Attachment 303339 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3231744

New failing tests:
media/modern-media-controls/media-documents/media-document-audio-ios-sizing.html

[Build Bot]

Created attachment 303348 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Antoine Quint]

Created attachment 303354 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 303354 [details]:

media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html bug 169158 (authors: [e-mail] and [e-mail])
media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html bug 169159 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 303354 [details]
Patch for landing

Clearing flags on attachment: 303354

Committed r213400: <http://trac.webkit.org/changeset/213400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused very frequent assertion failures on several media tests (media/modern-media-controls/media-documents/click-on-video-should-not-pause.html, http/tests/media/media-document.html, possibly more).

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fmedia%2Fmedia-document.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=media%2Fmodern-media-controls%2Fmedia-documents%2Fclick-on-video-should-not-pause.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r213410%20(3609)/fast/events/media-focus-in-standalone-media-document-crash-log.txt

Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 169163

[Antoine Quint]

It seems the line of code that triggers `ASSERT(!needsLayout())` in FrameView::paintContents is this:

media.classList.add(window.navigator.platform === "MacIntel" ? "mac" : window.navigator.platform)

I'm not sure why that is, but maybe having this class set in MediaDocumentParser::createDocumentStructure() where we already have some platform-specific code.

[Antoine Quint]

Created attachment 305457 [details]
Patch for landing

[Antoine Quint]

Raised https://bugs.webkit.org/show_bug.cgi?id=170113 to figure out why we were hitting this ASSERT in the first place. The patch is working around it by using a requestAnimationFrame call before setting the offending CSS classes.

[WebKit Commit Bot]

Comment on attachment 305457 [details]
Patch for landing

Clearing flags on attachment: 305457

Committed r214411: <http://trac.webkit.org/changeset/214411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 305457 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=305457&action=review

> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:72
> +        const isLiveBroadcast = this.media.duration === Number.POSITIVE_INFINITY;

Why test this here, a live stream can be audio-only?

[Ryan Haddad]

Reverted r214411 for reason:

Two of the LayoutTests for this change time out on ios-simulator.

Committed r214429: <http://trac.webkit.org/changeset/214429>

[Ryan Haddad]

*** Bug 170123 has been marked as a duplicate of this bug. ***

[Antoine Quint]

Committed r214468: <http://trac.webkit.org/changeset/214468>