Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Videos autoplay with sound on cnn.com pages after refresh in Safari #15423

Conversation

jernoble
Copy link
Contributor

@jernoble jernoble commented Jun 29, 2023

f4cdea7

Videos autoplay with sound on cnn.com pages after refresh in Safari
https://bugs.webkit.org/show_bug.cgi?id=258696
rdar://110343800

Reviewed by Chris Dumez.

The HTML specification defines a very narrow set of gesture events
which will cause activation. WebKit currently triggers activation
for all uses of UserGestureIndicator, which leads to things like
"LeftCmd Up" after a Cmd-R to trigger playback during a reload.

Add a set of helper methods `userGestureTypeForPlatformEvent()` to
return the correct UserGestureType for a particular PlatformEvent.

Update UserGestureIndicator to only set the activation timestamp of
the window when passed a UserGestureType::ActivationTriggering.

Make UserGestureType::ActivationTriggering the default to handle
the currently large set of call sites that expect a user gesture
to be triggered from a programmatic call.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-keyboard-enter-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-keyboard-escape-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-mouse-left-expected.txt:
* Source/WebCore/dom/UserGestureIndicator.cpp:
* Source/WebCore/dom/UserGestureIndicator.h:
* Source/WebCore/page/EventHandler.cpp:
(WebCore::userGestureTypeForPlatformEvent):
(WebCore::EventHandler::handleMousePressEvent):
(WebCore::EventHandler::handleMouseDoubleClickEvent):
(WebCore::EventHandler::handleMouseReleaseEvent):
(WebCore::EventHandler::internalKeyEvent):
(WebCore::EventHandler::handleTouchEvent):

Canonical link: https://commits.webkit.org/265688@main

e012322

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@jernoble jernoble self-assigned this Jun 29, 2023
@jernoble jernoble added the Media Bugs related to the HTML 5 Media elements. label Jun 29, 2023
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the bots are happy.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
if (auto* window = document->domWindow())
// https://html.spec.whatwg.org/multipage/interaction.html#user-activation-processing-model
// When a user interaction causes firing of an activation triggering input event in a Document...
// NOTE: Only activate the relevent DOMWindow when the gestureType is an ActivationTriggering one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: relevent

@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from c3e08e5 to 51232c5 Compare June 30, 2023 06:07
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from 51232c5 to 97acabf Compare June 30, 2023 15:36
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from 97acabf to 06433aa Compare June 30, 2023 15:37
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from 06433aa to 28177ab Compare June 30, 2023 18:28
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 30, 2023
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from 28177ab to 186d761 Compare June 30, 2023 21:46
@jernoble jernoble force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from 186d761 to e012322 Compare June 30, 2023 22:04
@eric-carlson eric-carlson added the merge-queue Applied to send a pull request to merge-queue label Jul 1, 2023
https://bugs.webkit.org/show_bug.cgi?id=258696
rdar://110343800

Reviewed by Chris Dumez.

The HTML specification defines a very narrow set of gesture events
which will cause activation. WebKit currently triggers activation
for all uses of UserGestureIndicator, which leads to things like
"LeftCmd Up" after a Cmd-R to trigger playback during a reload.

Add a set of helper methods `userGestureTypeForPlatformEvent()` to
return the correct UserGestureType for a particular PlatformEvent.

Update UserGestureIndicator to only set the activation timestamp of
the window when passed a UserGestureType::ActivationTriggering.

Make UserGestureType::ActivationTriggering the default to handle
the currently large set of call sites that expect a user gesture
to be triggered from a programmatic call.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-keyboard-enter-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-keyboard-escape-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/user-activation/activation-trigger-mouse-left-expected.txt:
* Source/WebCore/dom/UserGestureIndicator.cpp:
* Source/WebCore/dom/UserGestureIndicator.h:
* Source/WebCore/page/EventHandler.cpp:
(WebCore::userGestureTypeForPlatformEvent):
(WebCore::EventHandler::handleMousePressEvent):
(WebCore::EventHandler::handleMouseDoubleClickEvent):
(WebCore::EventHandler::handleMouseReleaseEvent):
(WebCore::EventHandler::internalKeyEvent):
(WebCore::EventHandler::handleTouchEvent):

Canonical link: https://commits.webkit.org/265688@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch from e012322 to f4cdea7 Compare July 1, 2023 18:33
@webkit-commit-queue
Copy link
Collaborator

Committed 265688@main (f4cdea7): https://commits.webkit.org/265688@main

Reviewed commits have been landed. Closing PR #15423 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit f4cdea7 into WebKit:main Jul 1, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 1, 2023
@jernoble jernoble deleted the eng/Videos-autoplay-with-sound-on-cnn-com-pages-after-refresh-in-Safari branch October 26, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
6 participants