Bug 163109 - [Modern Media Controls] Buttons
Summary: [Modern Media Controls] Buttons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-07 05:47 PDT by Antoine Quint
Modified: 2016-10-10 16:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (125.57 KB, patch)
2016-10-07 05:52 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (11.05 MB, application/zip)
2016-10-07 07:06 PDT, Build Bot
no flags Details
Patch (126.97 KB, patch)
2016-10-07 07:42 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2 (6.42 MB, application/zip)
2016-10-07 08:55 PDT, Build Bot
no flags Details
Patch (129.49 KB, patch)
2016-10-07 10:06 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (131.29 KB, patch)
2016-10-10 12:40 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-07 05:47:18 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-10-07 05:47:56 PDT
<rdar://problem/28668954>
Comment 2 Antoine Quint 2016-10-07 05:52:58 PDT
Created attachment 290923 [details]
Patch
Comment 3 Build Bot 2016-10-07 07:06:00 PDT
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
Comment 4 Build Bot 2016-10-07 07:06:03 PDT
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
Comment 5 Antoine Quint 2016-10-07 07:42:26 PDT
Created attachment 290930 [details]
Patch
Comment 6 Build Bot 2016-10-07 08:55:21 PDT
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
Comment 7 Build Bot 2016-10-07 08:55:24 PDT
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
Comment 8 Antoine Quint 2016-10-07 10:06:18 PDT
Created attachment 290938 [details]
Patch
Comment 9 Dean Jackson 2016-10-10 11:58:48 PDT
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.
Comment 10 Antoine Quint 2016-10-10 12:04:51 PDT
(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.
Comment 11 Antoine Quint 2016-10-10 12:40:22 PDT
Created attachment 291142 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-10-10 13:17:18 PDT
Comment on attachment 291142 [details]
Patch for landing

Clearing flags on attachment: 291142

Committed r207015: <http://trac.webkit.org/changeset/207015>
Comment 13 WebKit Commit Bot 2016-10-10 13:17:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Ryan Haddad 2016-10-10 16:01:18 PDT
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