Now that we have the foundations to display buttons in place (see https://bugs.webkit.org/show_bug.cgi?id=162868 and https://bugs.webkit.org/show_bug.cgi?id=162970), we need to add specific buttons for the various icons that we want to display in the media controls on macOS (inline and fullscreen) and iOS.
<rdar://problem/28668954>
Created attachment 290923 [details] Patch
Comment on attachment 290923 [details] Patch Attachment 290923 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2237526 New failing tests: media/modern-media-controls/airplay-button/airplay-button.html
Created attachment 290928 [details] Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 290930 [details] Patch
Comment on attachment 290930 [details] Patch Attachment 290930 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2237982 New failing tests: media/modern-media-controls/airplay-button/airplay-button.html
Created attachment 290931 [details] Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 290938 [details] Patch
Comment on attachment 290938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290938&action=review > Source/WebCore/ChangeLog:57 > + * Modules/modern-media-controls/controls/airplay-button.css: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (button.airplay.on): > + * Modules/modern-media-controls/controls/airplay-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (AirplayButton): > + (AirplayButton.prototype.set on): > + * Modules/modern-media-controls/controls/aspect-ratio-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (AspectRatioButton): > + (AspectRatioButton.prototype.get scalesToFill): > + (AspectRatioButton.prototype.set scalesToFill): > + * Modules/modern-media-controls/controls/forward-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (ForwardButton): > + * Modules/modern-media-controls/controls/fullscreen-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (FullscreenButton): > + * Modules/modern-media-controls/controls/icon-service.js: > + * Modules/modern-media-controls/controls/mute-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (MuteButton): > + (MuteButton.prototype.get muted): > + (MuteButton.prototype.set muted): > + * Modules/modern-media-controls/controls/pip-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (PiPButton): > + * Modules/modern-media-controls/controls/play-pause-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (PlayPauseButton): > + (PlayPauseButton.prototype.get playing): > + (PlayPauseButton.prototype.set playing): > + * Modules/modern-media-controls/controls/rewind-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (RewindButton): > + * Modules/modern-media-controls/controls/skip-back-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (SkipBackButton): > + * Modules/modern-media-controls/controls/start-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > + (StartButton): > + * Modules/modern-media-controls/controls/tracks-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. Get rid of all this "Copied from". It's rubbish anyway. > Source/WebCore/Modules/modern-media-controls/controls/airplay-button.css:27 > + background-color: -apple-wireless-playback-target-active; Maybe you should have a variable for colors, and set them all from a single place? > Source/WebCore/Modules/modern-media-controls/controls/airplay-button.js:33 > + this.element.classList.add("airplay"); Maybe IconButton's constructor could take a list of classnames to append? You're doing this is many classes. Same with the iconName. > Source/WebCore/Modules/modern-media-controls/controls/mute-button.js:50 > + get muted() > + { > + return this.iconName === Icons.VolumeMuted; > + } > + > + set muted(flag) > + { > + if (this.muted === flag) > + return; > + > + this.iconName = flag ? Icons.VolumeMuted : Icons.Volume; > + } I'm not sure I like the idea of the button having a "muted" property, mostly because it doesn't actually mute anything or contact the media element. But I can't think of a better name. "enabled" maybe? > Source/WebCore/Modules/modern-media-controls/controls/play-pause-button.js:39 > + get playing() Same here. > Source/WebCore/Modules/modern-media-controls/controls/start-button.js:36 > + this.element.appendChild(document.createElement("div")).className = "background"; > + this.element.appendChild(new Image).src = iconService.urlForIconNameAndLayoutTraits(Icons.Start, this.layoutTraits); I suggest doing these over multiple lines.
(In reply to comment #9) > Comment on attachment 290938 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290938&action=review > > > Source/WebCore/ChangeLog:57 > > + * Modules/modern-media-controls/controls/tracks-button.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/icon-service.js. > > Get rid of all this "Copied from". It's rubbish anyway. Will do. > > Source/WebCore/Modules/modern-media-controls/controls/airplay-button.css:27 > > + background-color: -apple-wireless-playback-target-active; > > Maybe you should have a variable for colors, and set them all from a single > place? You mean a CSS variable? Or do it via JS? > > Source/WebCore/Modules/modern-media-controls/controls/airplay-button.js:33 > > + this.element.classList.add("airplay"); > > Maybe IconButton's constructor could take a list of classnames to append? > You're doing this is many classes. > > Same with the iconName. That's one possibility. I chose to have subclasses because it's cheap, and it makes it easy to debug since each IconButton is a clear subclass, not requiring to inspect some of its properties to see what it is. > > Source/WebCore/Modules/modern-media-controls/controls/mute-button.js:50 > > + get muted() > > + { > > + return this.iconName === Icons.VolumeMuted; > > + } > > + > > + set muted(flag) > > + { > > + if (this.muted === flag) > > + return; > > + > > + this.iconName = flag ? Icons.VolumeMuted : Icons.Volume; > > + } > > I'm not sure I like the idea of the button having a "muted" property, mostly > because it doesn't actually mute anything or contact the media element. But > I can't think of a better name. "enabled" maybe? It's a MuteButton and it has a muted state where it shows the muted icon. I think this is appropriate, but I'm happy to name it differently. > > Source/WebCore/Modules/modern-media-controls/controls/play-pause-button.js:39 > > + get playing() > > Same here. Same comments. > > Source/WebCore/Modules/modern-media-controls/controls/start-button.js:36 > > + this.element.appendChild(document.createElement("div")).className = "background"; > > + this.element.appendChild(new Image).src = iconService.urlForIconNameAndLayoutTraits(Icons.Start, this.layoutTraits); > > I suggest doing these over multiple lines. Agreed. Will split it out.
Created attachment 291142 [details] Patch for landing
Comment on attachment 291142 [details] Patch for landing Clearing flags on attachment: 291142 Committed r207015: <http://trac.webkit.org/changeset/207015>
All reviewed patches have been landed. Closing bug.
Two of the tests added with this change are extremely flaky. media/modern-media-controls/airplay-button/airplay-button.html media/modern-media-controls/play-pause-button/play-pause-button.html https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r207023%20(449)/results.html