Conversation
Ujjwal5705
changed the title
Ujjwal5705
force-pushed
the
full_screen_issue
branch
from
8e56519 to
f17d03d
Compare
|
Please clean up your commit history and post again to request a review. See here for guidelines. |
Ujjwal5705
force-pushed
the
full_screen_issue
branch
from
0482ace to
c4c4a2c
Compare
|
@alya I’ve cleaned up the commit history as per the guidelines and updated the branch. Could you please take another look? |
|
Could you please update your commit message to match the commit style guidelines? |
On macOS, videos embedded in Zulip Desktop could not enter fullscreen
mode because the app's permission handler did not explicitly grant
fullscreen permissions.
This commit updates the `ipcRenderer.on("permission-request")` handler
to include fullscreen, in addition to existing permissions such as
notifications. With this change, video players (e.g., YouTube and
Zulip-embedded media) can properly request fullscreen.
Tested on macOS to confirm fullscreen now works, and on Windows/Linux
to ensure no regressions.
Fixes zulip#1409.
Ujjwal5705
force-pushed
the
full_screen_issue
branch
from
c4c4a2c to
5f9a74d
Compare
|
@alya I've updated the commit message, Could you please take another look? |
|
@shubham-padia Could you please take a look at this one? |
shubham-padia
left a comment
shubham-padia
left a comment
There was a problem hiding this comment.
Thank you for your contribution, please address the feedback noted in the comments.
| ); | ||
| }), | ||
| ) | ||
| ).some(Boolean); |
There was a problem hiding this comment.
While I don't like how hasPermission works currently. Changes should be made in hasPermission instead and not here.
| "version": "5.4.19", | ||
| "resolved": "https://registry.npmjs.org/vite/-/vite-5.4.19.tgz", | ||
| "integrity": "sha512-qO3aKv3HoQC8QKiNSTuUM1l9o/XX3+c+VTgLHbJWHZGeTPVAg2XwazI9UWzoxjIJCGCV2zU60uqMzjeLZuULqA==", | ||
| "version": "5.4.20", |
There was a problem hiding this comment.
Has this changed by mistake? We should not be updating package versions unless necessary.
There was a problem hiding this comment.
Has this changed by mistake? We should not be updating package versions unless necessary.
Yes, It was by mistake.
|
Opened #1469 with the requested changes while keeping the original author the same. I have also updated the email in your commit to link back to your profile. |
Got it. I didn’t mess with the internal logic of hasPermission as I wanted to keep things simple. But I did clean up the calling code to get rid of the endless ternary mess and swapped out Promise.all for a for...of loop. It’s just faster this way. Now, as soon as we find the right webview, we stop right there instead of spinning through every single tab. |
5 participants
Problem
On macOS, videos embedded in Zulip Desktop could not enter fullscreen mode.
This was caused by the app’s permission handler not explicitly granting the
fullscreenpermission.Solution
Updated the
ipcRenderer.on("permission-request")handler to grantfullscreenpermission in addition to existing ones (e.g., notifications).This ensures video players and other elements can properly request fullscreen.
Fixes: #1409
Screen Capture:-
Screen.Recording.2025-09-14.at.5.46.14.PM.mov
Embedding Check:-
Screen.Recording.2025-09-14.at.6.18.38.PM.mov
Platforms Tested
Self-review checklist
Code changes are minimal and scoped to permission handling.
Commit message explains the fix clearly.
Verified fullscreen works on macOS video players (tested on YouTube and Zulip-embedded media).
Tested on Windows/Linux to confirm no regressions.