Bug 163109 – [Modern Media Controls] Buttons

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

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/28668954>

[Antoine Quint]

Created attachment 290923 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Antoine Quint]

Created attachment 290930 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Antoine Quint]

Created attachment 290938 [details]
Patch

[Dean Jackson]

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.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 291142 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291142 [details]
Patch for landing

Clearing flags on attachment: 291142

Committed r207015: <http://trac.webkit.org/changeset/207015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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