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.

Sign up for GitHub

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

Jump to bottom

Adjust ContextMenu animation #14572

Merged
merged 1 commit into from Jun 2, 2023
Merged

Adjust ContextMenu animation #14572

merged 1 commit into from Jun 2, 2023

Conversation

mwyrzykowski
Copy link
Contributor

@mwyrzykowski mwyrzykowski commented Jun 1, 2023 •
edited by webkit-early-warning-system

4ee917d

Adjust ContextMenu animation
https://bugs.webkit.org/show_bug.cgi?id=257540
<radar://107716428>

Reviewed by Dean Jackson.

Change behavior of context menu animation such that fading is used
instead of morphing when tapping on the preview.

* Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView _contextMenuInteraction:configurationForMenuAtLocation:completion:]):
(-[WKContentView contextMenuInteraction:configuration:dismissalPreviewForItemWithIdentifier:contextMenuInteraction:previewForDismissingMenuWithConfiguration:]):
(-[WKContentView contextMenuInteraction:willPerformPreviewActionForMenuWithConfiguration:animator:]):
(-[WKContentView contextMenuInteraction:willEndForConfiguration:animator:]):

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

ac200b2

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

@mwyrzykowski mwyrzykowski requested a review from cdumez as a code owner June 1, 2023 05:46
@mwyrzykowski mwyrzykowski self-assigned this Jun 1, 2023
@mwyrzykowski mwyrzykowski added the Layout and Rendering For bugs with layout and rendering of Web pages. label Jun 1, 2023
@webkit-early-warning-system
Copy link
Collaborator

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

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

@mwyrzykowski mwyrzykowski marked this pull request as draft June 1, 2023 05:51
@mwyrzykowski mwyrzykowski deleted the eng/Adjust-ContextMenu-animation branch June 1, 2023 05:52
@mwyrzykowski mwyrzykowski restored the eng/Adjust-ContextMenu-animation branch June 1, 2023 06:47
@mwyrzykowski mwyrzykowski reopened this Jun 1, 2023
@mwyrzykowski mwyrzykowski marked this pull request as ready for review June 1, 2023 06:48
@mwyrzykowski mwyrzykowski force-pushed the eng/Adjust-ContextMenu-animation branch from ce378fd to 22c0bd2 Compare June 1, 2023 06:48
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 22c0bd2)

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

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 22c0bd2)

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

mwyrzykowski
mwyrzykowski commented Jun 1, 2023
View reviewed changes
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
@@ -12576,7 +12582,7 @@ - (void)contextMenuInteraction:(UIContextMenuInteraction *)interaction willPerfo
if (!_webView)
return;

[self _stopSuppressingSelectionAssistantForReason:WebKit::InteractionIsHappening];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We call _stopSuppressingSelectionAssistantForReason in -[UIContextMenuInteractionDelegate contextMenuInteraction:willEndForConfiguration:animator:] so I think it's not needed here.

Calling [_textInteractionAssistant activateSelection] (which is called from _stopSuppressingSelectionAssistantForReason) before the animation completes was breaking the animation's origin.

rr-codes
rr-codes reviewed Jun 1, 2023
View reviewed changes
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm Outdated
@@ -12690,15 +12696,22 @@ - (void)contextMenuInteraction:(UIContextMenuInteraction *)interaction willEndFo
_contextMenuHasRequestedLegacyData = NO;
_contextMenuElementInfo = nullptr;

[animator addCompletion:[weakSelf = WeakObjCPtr<WKContentView>(self)] () {
auto lambda = [weakSelf = WeakObjCPtr<WKContentView>(self)] () {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably have a better name than just :lambda:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could name it tearDownAfterAnimation but if animator is nil (like on Catalyst) it happens immediately.

I'm certainly open to suggestions but previously the lambda had no name so using lambda seemed reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

contextFinalizer or something?

@mwyrzykowski mwyrzykowski force-pushed the eng/Adjust-ContextMenu-animation branch from 22c0bd2 to ac200b2 Compare June 1, 2023 22:40
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for ac200b2. Live statuses available at the PR page, #14572

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash ac200b2)

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

@mwyrzykowski mwyrzykowski added the merge-queue Applied to send a pull request to merge-queue label Jun 1, 2023
@mwyrzykowski
Adjust ContextMenu animation
4ee917d 
https://bugs.webkit.org/show_bug.cgi?id=257540
<radar://107716428>

Reviewed by Dean Jackson.

Change behavior of context menu animation such that fading is used
instead of morphing when tapping on the preview.

* Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView _contextMenuInteraction:configurationForMenuAtLocation:completion:]):
(-[WKContentView contextMenuInteraction:configuration:dismissalPreviewForItemWithIdentifier:contextMenuInteraction:previewForDismissingMenuWithConfiguration:]):
(-[WKContentView contextMenuInteraction:willPerformPreviewActionForMenuWithConfiguration:animator:]):
(-[WKContentView contextMenuInteraction:willEndForConfiguration:animator:]):

Canonical link: https://commits.webkit.org/264817@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264817@main (4ee917d): https://commits.webkit.org/264817@main

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

@webkit-commit-queue webkit-commit-queue merged commit 4ee917d into WebKit:main Jun 2, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rr-codes rr-codes rr-codes left review comments

@grorg grorg grorg approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@hortont424 hortont424 Awaiting requested review from hortont424

@whsieh whsieh Awaiting requested review from whsieh

@pxlcoder pxlcoder Awaiting requested review from pxlcoder

Assignees

@mwyrzykowski mwyrzykowski

Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
Milestone
No milestone
5 participants
@mwyrzykowski @webkit-early-warning-system @webkit-commit-queue @grorg @rr-codes