Context Menu shows Exit Full Screen instead of Exit Viewer #29798

danae404 · 2024-06-14T00:48:56Z

`de1667d`

Context Menu shows Exit Full Screen instead of Exit Viewer
https://bugs.webkit.org/show_bug.cgi?id=275460
rdar://126300924

Reviewed by Jer Noble.

This patch adds a context menu item to video elements
that enters and exits Viewer mode. This patch also
removes the toggle fullscreen context menu item when
the video is in Viewer mode.

* Source/WebCore/en.lproj/Localizable.strings:
* Source/WebCore/page/ContextMenuController.cpp:
(WebCore::ContextMenuController::contextMenuItemSelected):
(WebCore::ContextMenuController::populate):
(WebCore::ContextMenuController::checkOrEnableIfNeeded const):
* Source/WebCore/platform/ContextMenuItem.cpp:
(WebCore::isValidContextMenuAction):
* Source/WebCore/platform/ContextMenuItem.h:
* Source/WebCore/platform/LocalizedStrings.h:
* Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm:
(WebCore::contextMenuItemTagEnterVideoViewer):
(WebCore::contextMenuItemTagExitVideoViewer):
* Source/WebCore/rendering/HitTestResult.cpp:
(WebCore::HitTestResult::mediaIsInVideoViewer const):
(WebCore::HitTestResult::toggleVideoViewer const):
* Source/WebCore/rendering/HitTestResult.h:
* Source/WebKit/Shared/API/c/WKContextMenuItemTypes.h:
* Source/WebKit/Shared/API/c/WKSharedAPICast.h:
(WebKit::toAPI):
(WebKit::toImpl):
* Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiers.mm:
* Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiersPrivate.h:
* Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:
(WebKit::menuItemIdentifier):
* Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:
(toAction):
(toTag):
* Source/WebKitLegacy/mac/WebView/WebUIDelegatePrivate.h:

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

61d3bc9

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	⏳ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	⏳ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	⏳ 🧪 vision-wk2
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-06-14T00:49:03Z

EWS run on previous version of this PR (hash 9b94885)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	✅ 🛠 wpe-cairo
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	~~🛠 watch~~
	~~🛠 watch-sim~~

webkit-early-warning-system · 2024-06-14T01:11:01Z

EWS run on previous version of this PR (hash aba38a6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rr-codes · 2024-06-14T01:11:13Z

Source/WebCore/WebCore.xcodeproj/project.pbxproj

@@ -3,7 +3,7 @@
 archiveVersion = 1;
 classes = {
 };
- objectVersion = 54;
+ objectVersion = 55;


Is this ok?

rr-codes · 2024-06-14T03:40:56Z

Source/WebCore/en.lproj/Localizable.strings

@@ -175,6 +175,9 @@
 /* Allow button title in speech recognition prompt */
 "Allow (speechrecognition)" = "Allow";

+/* Allow */


are these ok to add?

This isn't being added, just moved. The script sorts alphabetically.

Though we should preserve the existing comment for this string, which was Display USDZ file.

rr-codes · 2024-06-14T03:42:00Z

Source/WebCore/rendering/HitTestResult.cpp

+ auto* mediaElement(this->mediaElement());
+ auto* videoElement = dynamicDowncast<HTMLVideoElement>(*mediaElement);


I think these should be RefPtr's maybe?

I see a lot of examples in this file of raw pointers being used (for example, HitTestResult::toggleEnhancedFullscreenForVideo()). I am not sure if this is for a particular reason, or if it would still be best practice to use RefPtr's. @pxlcoder do you have thoughts?

All new code should use RefPtrs. We are slowly working towards adopting smart pointers everywhere (which is why others in this file have not yet been updated).

rr-codes · 2024-06-14T03:43:19Z

Source/WebCore/rendering/HitTestResult.cpp

+ if (videoElement->webkitPresentationMode() == HTMLVideoElement::VideoPresentationMode::InWindow)
+ videoElement->webkitSetPresentationMode(HTMLVideoElement::VideoPresentationMode::Inline);
+ else
+ videoElement->webkitSetPresentationMode(HTMLVideoElement::VideoPresentationMode::InWindow);


Suggested change

if (videoElement->webkitPresentationMode() == HTMLVideoElement::VideoPresentationMode::InWindow)

videoElement->webkitSetPresentationMode(HTMLVideoElement::VideoPresentationMode::Inline);

else

videoElement->webkitSetPresentationMode(HTMLVideoElement::VideoPresentationMode::InWindow);

auto newMode = videoElement->webkitPresentationMode() == HTMLVideoElement::VideoPresentationMode::InWindow ? HTMLVideoElement::VideoPresentationMode::Inline : HTMLVideoElement::VideoPresentationMode::InWindow;

videoElement->webkitSetPresentationMode(newMode);

Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiersPrivate.h

pxlcoder · 2024-06-14T16:11:38Z

Source/WebCore/en.lproj/Localizable.strings

@@ -175,6 +175,9 @@
 /* Allow button title in speech recognition prompt */
 "Allow (speechrecognition)" = "Allow";

+/* Allow */


This isn't being added, just moved. The script sorts alphabetically.

Though we should preserve the existing comment for this string, which was Display USDZ file.

pxlcoder · 2024-06-14T16:12:08Z

Source/WebCore/en.lproj/Localizable.strings

@@ -1096,6 +1102,9 @@
 /* Title for Open in External Application Link action button */
 "Open in “%@”" = "Open in “%@”";

+/* Open this 3D model? */


We should probably preserve the original comment, which was Open 3D object in a new window?.

pxlcoder · 2024-06-14T16:12:42Z

Source/WebCore/en.lproj/Localizable.strings

@@ -286,12 +289,12 @@
 /* Button title in Device Orientation Permission API prompt */
 "Cancel (device motion and orientation access)" = "Cancel";

+/* Cancel */


Same comment about preserving the original comment. Cancel USDZ file.

webkit-early-warning-system · 2024-06-14T21:04:05Z

EWS run on previous version of this PR (hash 590540c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rr-codes · 2024-06-14T22:07:01Z

Source/WebCore/rendering/HitTestResult.cpp

+{
+#if PLATFORM(MAC) && ENABLE(VIDEO) && ENABLE(VIDEO_PRESENTATION_MODE)
+ RefPtr mediaElement(this->mediaElement());
+ RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(*mediaElement);


I think this is unsafe is mediaElement is ever null?

I think dynamicDowncast returns a nullptr if mediaElement is null

I think you need to do:

RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(mediaElement);

I.e., without the * dereference.

webkit-early-warning-system · 2024-06-17T16:41:21Z

EWS run on previous version of this PR (hash 5ab3302)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	⏳ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	⏳ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	⏳ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

jernoble · 2024-06-17T18:22:53Z

Source/WebCore/rendering/HitTestResult.cpp

+{
+#if PLATFORM(MAC) && ENABLE(VIDEO) && ENABLE(VIDEO_PRESENTATION_MODE)
+ RefPtr mediaElement(this->mediaElement());
+ RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(*mediaElement);


I think you need to do:

RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(mediaElement);

I.e., without the * dereference.

webkit-early-warning-system · 2024-06-18T00:09:01Z

EWS run on current version of this PR (hash 61d3bc9)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	⏳ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	⏳ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	⏳ 🧪 vision-wk2
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

jernoble · 2024-06-18T16:15:44Z

Source/WebCore/rendering/HitTestResult.cpp

+ RefPtr mediaElement(this->mediaElement());
+ RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(mediaElement);


Nits: Early returns are always ok, so this could bail after the first RefPtr. Also, this could be written either as a simple assignment, or with the modern, curly-brace initializer:

RefPtr mediaElement { this->mediaElement() }; if (!mediaElement) return; RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(mediaElement);

But this would null-check mediaElement twice, so you could add back in the dereference to mediaElement inside dynamicDowncast(). Just a suggestion though.

https://bugs.webkit.org/show_bug.cgi?id=275460 rdar://126300924 Reviewed by Jer Noble. This patch adds a context menu item to video elements that enters and exits Viewer mode. This patch also removes the toggle fullscreen context menu item when the video is in Viewer mode. * Source/WebCore/en.lproj/Localizable.strings: * Source/WebCore/page/ContextMenuController.cpp: (WebCore::ContextMenuController::contextMenuItemSelected): (WebCore::ContextMenuController::populate): (WebCore::ContextMenuController::checkOrEnableIfNeeded const): * Source/WebCore/platform/ContextMenuItem.cpp: (WebCore::isValidContextMenuAction): * Source/WebCore/platform/ContextMenuItem.h: * Source/WebCore/platform/LocalizedStrings.h: * Source/WebCore/platform/cocoa/LocalizedStringsCocoa.mm: (WebCore::contextMenuItemTagEnterVideoViewer): (WebCore::contextMenuItemTagExitVideoViewer): * Source/WebCore/rendering/HitTestResult.cpp: (WebCore::HitTestResult::mediaIsInVideoViewer const): (WebCore::HitTestResult::toggleVideoViewer const): * Source/WebCore/rendering/HitTestResult.h: * Source/WebKit/Shared/API/c/WKContextMenuItemTypes.h: * Source/WebKit/Shared/API/c/WKSharedAPICast.h: (WebKit::toAPI): (WebKit::toImpl): * Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiers.mm: * Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiersPrivate.h: * Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm: (WebKit::menuItemIdentifier): * Source/WebKitLegacy/mac/WebView/WebHTMLView.mm: (toAction): (toTag): * Source/WebKitLegacy/mac/WebView/WebUIDelegatePrivate.h: Canonical link: https://commits.webkit.org/280128@main

webkit-commit-queue · 2024-06-18T17:59:11Z

Committed 280128@main (de1667d): https://commits.webkit.org/280128@main

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

danae404 requested a review from cdumez as a code owner June 14, 2024 00:48

danae404 self-assigned this Jun 14, 2024

danae404 added the Media Bugs related to the HTML 5 Media elements. label Jun 14, 2024

danae404 requested review from jernoble, rr-codes, pxlcoder, aestes, jyavenard and eric-carlson June 14, 2024 00:57

danae404 force-pushed the eng/Context-Menu-shows-Exit-Full-Screen-instead-of-Exit-Viewer branch from 9b94885 to aba38a6 Compare June 14, 2024 01:10

rr-codes reviewed Jun 14, 2024

View reviewed changes

Source/WebKit/UIProcess/API/Cocoa/WKMenuItemIdentifiersPrivate.h Show resolved Hide resolved

pxlcoder reviewed Jun 14, 2024

View reviewed changes

danae404 force-pushed the eng/Context-Menu-shows-Exit-Full-Screen-instead-of-Exit-Viewer branch from aba38a6 to 590540c Compare June 14, 2024 21:04

danae404 requested review from pxlcoder and rr-codes June 14, 2024 21:59

rr-codes reviewed Jun 14, 2024

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 14, 2024

danae404 removed the merging-blocked Applied to prevent a change from being merged label Jun 17, 2024

danae404 force-pushed the eng/Context-Menu-shows-Exit-Full-Screen-instead-of-Exit-Viewer branch from 590540c to 5ab3302 Compare June 17, 2024 16:41

danae404 requested a review from rr-codes June 17, 2024 16:51

jernoble requested changes Jun 17, 2024

View reviewed changes

danae404 force-pushed the eng/Context-Menu-shows-Exit-Full-Screen-instead-of-Exit-Viewer branch from 5ab3302 to 61d3bc9 Compare June 18, 2024 00:08

danae404 requested a review from jernoble June 18, 2024 00:09

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 18, 2024

jernoble approved these changes Jun 18, 2024

View reviewed changes

danae404 added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Jun 18, 2024

webkit-commit-queue force-pushed the eng/Context-Menu-shows-Exit-Full-Screen-instead-of-Exit-Viewer branch from 61d3bc9 to de1667d Compare June 18, 2024 17:59

webkit-commit-queue merged commit de1667d into WebKit:main Jun 18, 2024

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Context Menu shows Exit Full Screen instead of Exit Viewer #29798

Context Menu shows Exit Full Screen instead of Exit Viewer #29798

danae404 commented Jun 14, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jun 14, 2024 •

edited

webkit-early-warning-system commented Jun 14, 2024 •

edited

rr-codes Jun 14, 2024

rr-codes Jun 14, 2024

pxlcoder Jun 14, 2024

rr-codes Jun 14, 2024

danae404 Jun 14, 2024

pxlcoder Jun 14, 2024

rr-codes Jun 14, 2024

pxlcoder Jun 14, 2024

pxlcoder Jun 14, 2024

pxlcoder Jun 14, 2024

webkit-early-warning-system commented Jun 14, 2024 •

edited

rr-codes Jun 14, 2024

danae404 Jun 17, 2024

jernoble Jun 17, 2024

webkit-early-warning-system commented Jun 17, 2024 •

edited

jernoble Jun 17, 2024

webkit-early-warning-system commented Jun 18, 2024 •

edited

jernoble Jun 18, 2024

webkit-commit-queue commented Jun 18, 2024

		auto* mediaElement(this->mediaElement());
		auto* videoElement = dynamicDowncast<HTMLVideoElement>(*mediaElement);

		RefPtr mediaElement(this->mediaElement());
		RefPtr videoElement = dynamicDowncast<HTMLVideoElement>(mediaElement);

Context Menu shows Exit Full Screen instead of Exit Viewer #29798

Context Menu shows Exit Full Screen instead of Exit Viewer #29798

Conversation

danae404 commented Jun 14, 2024 • edited by webkit-early-warning-system

webkit-early-warning-system commented Jun 14, 2024 • edited

webkit-early-warning-system commented Jun 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jun 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jun 17, 2024 • edited

Choose a reason for hiding this comment

webkit-early-warning-system commented Jun 18, 2024 • edited

Choose a reason for hiding this comment

webkit-commit-queue commented Jun 18, 2024

danae404 commented Jun 14, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jun 14, 2024 •

edited

webkit-early-warning-system commented Jun 14, 2024 •

edited

webkit-early-warning-system commented Jun 14, 2024 •

edited

webkit-early-warning-system commented Jun 17, 2024 •

edited

webkit-early-warning-system commented Jun 18, 2024 •

edited