Bug 168515 - [Modern Media Controls] Toggle playback when clicking on the video on macOS
Summary: [Modern Media Controls] Toggle playback when clicking on the video on macOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-17 07:51 PST by Antoine Quint
Modified: 2017-02-22 16:44 PST (History)
1 user (show)

See Also:


Attachments
Patch (18.99 KB, patch)
2017-02-17 08:32 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (14.97 MB, application/zip)
2017-02-17 09:49 PST, Build Bot
no flags Details
Patch (19.00 KB, patch)
2017-02-17 11:31 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2017-02-17 07:51:58 PST
We should toggle playback when clicking on the video on macOS
Comment 1 Radar WebKit Bug Importer 2017-02-17 07:54:17 PST
<rdar://problem/30577441>
Comment 2 Antoine Quint 2017-02-17 08:32:09 PST
Created attachment 301934 [details]
Patch
Comment 3 Build Bot 2017-02-17 09:49:23 PST
Comment on attachment 301934 [details]
Patch

Attachment 301934 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3143858

New failing tests:
media/modern-media-controls/media-controller/media-controller-click-on-video-background-should-pause.html
media/modern-media-controls/media-controller/media-controller-click-on-video-background-to-dismiss-tracks-panel-should-not-toggle-playback.html
Comment 4 Build Bot 2017-02-17 09:49:26 PST
Created attachment 301947 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 5 Antoine Quint 2017-02-17 11:31:07 PST
Created attachment 301963 [details]
Patch
Comment 6 Dean Jackson 2017-02-17 11:57:59 PST
Comment on attachment 301963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301963&action=review

> Source/WebCore/ChangeLog:12
> +        We now listen to "click" events on the media controls on macOS, and should no
> +        other element further down in the controls hierarchy receive that event, we
> +        inform the delegate, in this case the MediaController, that the background
> +        of the controls was clicked and we toggle playback.

This sentence is too long or has too many side notes/comments. How about

Listen to click events on the macOS media controls. Detect any clicks that were on the background instead of the controls widgets themselves, and tell the delegate (MediaController) about them.

> Source/WebCore/Modules/modern-media-controls/controls/macos-media-controls.js:71
> +        // Only notify that the background was clicked when the "mousedown" event
> +        // was also received, which wouldn not happen if the "mousedown" event caused
> +        // the tracks panel to be hidden.

Typo: wouldn't

Maybe you should say:

// In order to call the delegate for a click, we need to make sure that we've seen a mousedown event first. e.g. when...tracks panel...

> Source/WebCore/Modules/modern-media-controls/media/media-controller.js:102
> +        // Toggle playback when clickig on the video but not on any controls on macOS.

Typo: clicking
Comment 7 Antoine Quint 2017-02-17 13:42:29 PST
Committed r212573: <http://trac.webkit.org/changeset/212573>