Summary: | [Modern Media Controls] Improve media documents across macOS, iPhone and iPad | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||
Component: | Media | Assignee: | Antoine Quint <graouts> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, ryanhaddad, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Safari Technology Preview | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=170113 | ||||||||||||
Bug Depends on: | 169163 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Antoine Quint
2017-03-03 13:41:02 PST
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> All reviewed patches have been landed. Closing bug. 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> |