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.
<rdar://problem/17048858>
Created attachment 303339 [details] Patch
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?
(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.
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
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
Created attachment 303354 [details] Patch for landing
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: graouts@apple.com and ryanhaddad@apple.com) media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html bug 169159 (authors: graouts@apple.com and ryanhaddad@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 303354 [details] Patch for landing Clearing flags on attachment: 303354 Committed r213400: <http://trac.webkit.org/changeset/213400>
All reviewed patches have been landed. Closing bug.
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.
Re-opened since this is blocked by bug 169163
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.
Created attachment 305457 [details] Patch for landing
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.
Comment on attachment 305457 [details] Patch for landing Clearing flags on attachment: 305457 Committed r214411: <http://trac.webkit.org/changeset/214411>
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?
Reverted r214411 for reason: Two of the LayoutTests for this change time out on ios-simulator. Committed r214429: <http://trac.webkit.org/changeset/214429>
*** Bug 170123 has been marked as a duplicate of this bug. ***
Committed r214468: <http://trac.webkit.org/changeset/214468>