Bug 90498

Created attachment 150677 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 150677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150677&action=review A few details below. > Source/WebCore/html/HTMLMediaElement.cpp:3853 > + if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && !hasEventListeners(eventNames().clickEvent) && canPlay()) { Is !hasEventListeners(eventNames().clickEvent) needed here? I guess folks who handle the click event won't be preventing the default if there isn't one currently... > Source/WebCore/testing/InternalSettings.cpp:371 > +fprintf(stderr, "InternalSettings::setMediaClickToPlayEnabled"); We should probably remove this line before landing. > Source/WebCore/testing/InternalSettings.cpp:373 > + settings()->setMediaClickToPlayEnabled(enabled); Please reset this setting to its default value in InternalSettings::restoreTo > LayoutTests/media/video-click-to-play-enabled.html:14 > + function cleanClickToPlayRequirement() { > + if (window.internals) > + window.internals.settings.setMediaClickToPlayEnabled(false); > + } Rather than doing this in each test, we have the InternalsSettings object do this work for us. > LayoutTests/media/video-click-to-play-enabled.html:21 > + video.dispatchEvent(mouseClick); > + testExpected("video.paused", false); Can you add a test case for when the video element has a click event listener?

Created attachment 150691 [details] Patch

(In reply to comment #3) > (From update of attachment 150677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150677&action=review > > A few details below. > > > Source/WebCore/html/HTMLMediaElement.cpp:3853 > > + if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && !hasEventListeners(eventNames().clickEvent) && canPlay()) { > > Is !hasEventListeners(eventNames().clickEvent) needed here? I guess folks who handle the click event won't be preventing the default if there isn't one currently... > Good point. This should no longer be needed here. In downstream, because clicking the media could pause the playback. So we introduced it to prevent the case that a single click will cause both play() and pause(). Since the behavior is changed when upstreaming this, we no longer need this. > > Source/WebCore/testing/InternalSettings.cpp:371 > > +fprintf(stderr, "InternalSettings::setMediaClickToPlayEnabled"); > > We should probably remove this line before landing. Done, I forgot to remove this running the webkit-patch tool. > > > Source/WebCore/testing/InternalSettings.cpp:373 > > + settings()->setMediaClickToPlayEnabled(enabled); > > Please reset this setting to its default value in InternalSettings::restoreTo Done. > > > LayoutTests/media/video-click-to-play-enabled.html:14 > > + function cleanClickToPlayRequirement() { > > + if (window.internals) > > + window.internals.settings.setMediaClickToPlayEnabled(false); > > + } > > Rather than doing this in each test, we have the InternalsSettings object do this work for us. Done. > > > LayoutTests/media/video-click-to-play-enabled.html:21 > > + video.dispatchEvent(mouseClick); > > + testExpected("video.paused", false); > > Can you add a test case for when the video element has a click event listener? Done. added a listener to the media element for the 2nd click.

Comment on attachment 150691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review > Source/WebCore/testing/InternalSettings.cpp:110 > + , m_mediaClickToPlayEnabled(settings()->mediaClickToPlayEnabled()) nit: I've been using the prefix "original" for these variables to indicate that it's not the current value.

This looks reasonable to me, but let's give Eric Carlson a chance to comment.

> For video on mobile devices, expose a setting so that user can click on a media element to start playing it. This sounds confusing - mobile devices generally don't have controllers that support clicking.

Both chrome on android and android browser has media controllers that support clicking. But the buttons are quite small, especially for smartphone users. This change is to make it easier for user to play a media by just clicking the element. for chrome (In reply to comment #8) > > For video on mobile devices, expose a setting so that user can click on a media element to start playing it. > > This sounds confusing - mobile devices generally don't have controllers that support clicking.

Comment on attachment 150691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3855 > + if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) { > + play(); > + event->setDefaultHandled(); This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button. It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential.

ok, I will try put the functionality into the media control code then. (In reply to comment #10) > (From update of attachment 150691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:3855 > > + if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) { > > + play(); > > + event->setDefaultHandled(); > > This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button. > > It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential.

(In reply to comment #10) > (From update of attachment 150691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150691&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:3855 > > + if (document()->settings() && document()->settings()->mediaClickToPlayEnabled() && event->type() == eventNames().clickEvent && canPlay()) { > > + play(); > > + event->setDefaultHandled(); > > This strikes me as the wrong way to enable this feature. Other (most?) players that enable "click to play" have a UI affordance to help the user understand that clicking will initiate playback, often a large "play" button. If someone wanted to implement that in a WebKit port, they would not be able to use this setting because the play button would intercept the click, and they would presumably want to restrict the behavior to clicks in the play button. > > It seems to me that you are adding a new control function, so the right place for this is in the media control code. It should be easy to add an element to the controls shadow DOM and do the hit testing there. Whether you actually show a button or not is tangential. One problem with shadow DOM is that if the media element doesn't have the controls attribute, then clicking the media element will not work.

(In reply to comment #12) > One problem with shadow DOM is that if the media element doesn't have the controls attribute, then clicking the media element will not work. Unless something like your new setting also enables the shadow DOM, eg. if (controls() || document()->settings()->mediaClickToPlayEnabled()) ...

deprecating this patch as I created a new one to add a shadow DOM change for android: https://bugs.webkit.org/show_bug.cgi?id=92132