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

fix(mac): expose getDisplayMedia() permission decision handler #1196

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

pewsheen
Copy link
Contributor

Related issue: #1195

Temporary fix for the issue. Needs to find the affected range in the future.

@pewsheen pewsheen requested a review from a team as a code owner March 20, 2024 04:16
src/wkwebview/mod.rs Outdated Show resolved Hide resolved
@pewsheen pewsheen force-pushed the fix/media-perm-request-14-0 branch from 1e562dd to 90a5447 Compare March 20, 2024 07:53
wusyong
wusyong previously approved these changes Mar 20, 2024
Comment on lines 813 to 816
(14, 0, _) => {
// Do not suppress media request on 14.0:
// https://github.com/tauri-apps/wry/issues/1101
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(14, 0, _) => {
// Do not suppress media request on 14.0:
// https://github.com/tauri-apps/wry/issues/1101
}
(14, 0, _) | (14, 1, _) => {
// Do not suppress media request on 14.0 & 14.1:
// https://github.com/tauri-apps/wry/issues/1101
}

#1195 (comment)

Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

(just blocking this because of ongoing discussion in #1195)

@pewsheen pewsheen marked this pull request as draft March 20, 2024 13:16
@pewsheen
Copy link
Contributor Author

pewsheen commented Mar 23, 2024

@FabianLars Your private API (or maybe just not document yet?) hint is mega helpful

After I read some WebKit source code
https://github.com/WebKit/WebKit/blob/b7a7805ab3a2a1b719af3112657934e837ee3df5/Source/WebKit/UIProcess/Cocoa/UIDelegate.mm#L1260

bool respondsToRequestMediaCapturePermission = [delegate respondsToSelector:@selector(webView:requestMediaCapturePermissionForOrigin:initiatedByFrame:type:decisionHandler:)];
bool respondsToRequestUserMediaAuthorizationForDevices = [delegate respondsToSelector:@selector(_webView:requestUserMediaAuthorizationForDevices:url:mainFrameURL:decisionHandler:)];
bool respondsToRequestDisplayCapturePermissionForOrigin = [delegate respondsToSelector:@selector(_webView:requestDisplayCapturePermissionForOrigin:initiatedByFrame:withSystemAudio:decisionHandler:)];

I guess they are using _webView:requestDisplayCapturePermissionForOrigin:initiatedByFrame:withSystemAudio:decisionHandler: for getDisplayMedia and yes!

The reason why it's not working on your commit is because the typo LOL

- _webView:requestDisplayCapturePermissionForSecurityOrigin:initiatedByFrame:withSystemAudio:decisionHandler:
+ _webView:requestDisplayCapturePermissionForOrigin:initiatedByFrame:withSystemAudio:decisionHandler:

But it's still not showing the window / screen select panel, just window OR screen.

@FabianLars
Copy link
Member

nooo, not a freaking typo, I just can't 😂

@pewsheen
Copy link
Contributor Author

Just update the implementation, will expose a closure and let them pick the decision

@pewsheen pewsheen changed the title fix(mac): unsuppressing media permission request only for macOS 14.0 fix(mac): expose getDisplayMedia() permission decision handler Mar 27, 2024
@pewsheen pewsheen marked this pull request as ready for review April 8, 2024 03:02
@pewsheen
Copy link
Contributor Author

pewsheen commented Apr 8, 2024

Maybe it can just store the decision instead of a closure, since there's only a capture type parameter?

}
}

pub(crate) fn drop_decision_hanlder(webview: *mut Object) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub(crate) fn drop_decision_hanlder(webview: *mut Object) {
pub(crate) fn drop_decision_handler(webview: *mut Object) {

(the usages of this function have the same typo)

@FabianLars
Copy link
Member

Maybe it can just store the decision instead of a closure, since there's only a capture type parameter?

I'm not 100% sure i really understand. When we use a variable only the developer could decide right? The good thing about the handler is that wkwebview will call that itself and then the dev can implement something that will let the actual app user decide if they want a screen or window prompt. I don't see how we could mimic this with just a variable.

@pewsheen
Copy link
Contributor Author

pewsheen commented Apr 8, 2024

Maybe it can just store the decision instead of a closure, since there's only a capture type parameter?

I'm not 100% sure i really understand. When we use a variable only the developer could decide right? The good thing about the handler is that wkwebview will call that itself and then the dev can implement something that will let the actual app user decide if they want a screen or window prompt. I don't see how we could mimic this with just a variable.

In this case, the developer will need to implement something like a prompt to let the user choose to capture a screen or a window and call a setter like set_capture_target(Window or Screen), and then call the getDisplayMedia().

Closure is good and may not need an extra set step like the variable approach. But I think it will be a bit more complicated managing the lifetime and ownership when implementing logic inside the closure.

@FabianLars
Copy link
Member

Hmm, another problem that comes to mind is that this makes it harder for use cases where you load external websites that you don't control since then you don't really know when you ask the user what they want to share. I still think this is still a bit of a problem even if it's your website because then you have to communicate between js and rust to know when to prompt the user.

Closure is good and may not need an extra set step like the variable approach. But I think it will be a bit more complicated managing the lifetime and ownership when implementing logic inside the closure.

Hmm, yeah that could be a problem. I thought/hoped that the main thing to do here would be to simply call rfd in that closure and leave it at that 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants