Skip to content

VIDSOL-599: Hide local screenshare view when sharing entire screen to prevent infinite mirror effect#381

Open
ydumburs wants to merge 15 commits intodevelopfrom
VIDSOL-599-hide-entire-screenshare
Open

VIDSOL-599: Hide local screenshare view when sharing entire screen to prevent infinite mirror effect#381
ydumburs wants to merge 15 commits intodevelopfrom
VIDSOL-599-hide-entire-screenshare

Conversation

@ydumburs
Copy link
Copy Markdown

What is this PR doing?

Hides the local publisher screenshare preview when the user shares the entire screen (displaySurface==='monitor') to prevent infinite mirror effect. The behavior only applies to entire screen mode. Tab and Window sharing remain unchanged.

How should this be manually tested?

  1. Start a meeting.
  2. Share Entire Screen.
  3. Verify that
  • The local preview is hidden.
  • No infinite mirror effect appears.
  • The remote participant can see the shared screen normally.
  1. Stop sharing and confirm the UI returns to normal.
  2. Share a Tab or Window and verify the local preview is still visible.

What are the relevant tickets?

A maintainer will add this ticket number.

Resolves VIDSOL-599

Checklist

[x] Branch is based on develop (not main).
[ ] Resolves a Known Issue.
[ ] If yes, did you remove the item from the docs/KNOWN_ISSUES.md?
[ ] Resolves an item reported in Issues.
If yes, which issue? Issue Number?

@VZaphod VZaphod requested a review from a team February 24, 2026 14:36
@VZaphod VZaphod requested a review from a team February 24, 2026 14:51
@ydumburs
Copy link
Copy Markdown
Author

Thanks for the suggestions. I've pushed the updates addressing the comments.

@OscarFava
Copy link
Copy Markdown
Contributor

Pd: tests are failing, maybe you need to execute yarn test:integration update ?

Copy link
Copy Markdown
Contributor

@Hossein-Movahed Hossein-Movahed left a comment

Choose a reason for hiding this comment

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

Hey, great work on this! A few things to address.

@ydumburs
Copy link
Copy Markdown
Author

Thanks for the suggestion. I've updated the logic as recommended. The update has been pushed.

Copy link
Copy Markdown
Contributor

@Hossein-Movahed Hossein-Movahed left a comment

Choose a reason for hiding this comment

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

Thank you @ydumburs, just a few comments, and please add more tests, we need minimum 80% coverage for new code, and make sure the E2E test are passing

@ydumburs
Copy link
Copy Markdown
Author

Hi @Hossein-Movahed @OscarFava, I can't access the SonarCloud details page from my account. Could you please share which files or lines are uncovered so I can add the missing tests?

@OscarFava
Copy link
Copy Markdown
Contributor

Hi @Hossein-Movahed @OscarFava, I can't access the SonarCloud details page from my account. Could you please share which files or lines are uncovered so I can add the missing tests?

frontend/src/hooks/useScreenShare.tsx

@Hossein-Movahed
Copy link
Copy Markdown
Contributor

@ydumburs the E2E test is failing, the test now expects "You are sharing your screen." on the publisher side (Chrome always picks entire screen in E2E) instead of the User One's screen

      await pageOne.getByTestId('screen-publisher-container').getByText(`User One's screen`)
    ).toBeVisible();```

@ydumburs
Copy link
Copy Markdown
Author

Thank you @Hossein-Movahed . Updated the screenshare test.

Copy link
Copy Markdown
Contributor

@Hossein-Movahed Hossein-Movahed left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@OscarFava OscarFava left a comment

Choose a reason for hiding this comment

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

Awesome work, minor comment.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@OscarFava OscarFava left a comment

Choose a reason for hiding this comment

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

Minor comments

>
<ScreenShareNameDisplay name={streamName} box={box} />
{isEntireScreen ? (
<div className="absolute inset-0 flex items-center justify-center text-xl font-medium bg-vera-dark-background text-vera-on-background pointer-events-none">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

text-xl is wrong. You can use text-vera-heading-4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same with font-medium. You should use our tokens.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'll update the text and font.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this fixed? I do not see the changes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I fixed this locally but still waiting for @Hossein-Movahed 's answer on the next thread which I may need to change this part as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi, I've addressed the requested change.

const mediaStream = videoEl.srcObject as MediaStream | null;
const track = mediaStream?.getVideoTracks?.()[0];
const displaySurface = track?.getSettings?.()?.displaySurface;
setIsEntireScreen(displaySurface === 'monitor');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The displaySurface detection doesn't work in Firefox since getSettings() exists but displaySurface is not part of Firefox's implementation, so isEntireScreen will always be false there, causing the mirror effect.

A reliable fix is to fall back to comparing the track dimensions against the screen dimensions when displaySurface is undefined:

const settings = track?.getSettings?.();
const displaySurface = settings?.displaySurface;
// Firefox does not support displaySurface — fall back to dimension comparison
const isMonitor =
  displaySurface === 'monitor' ||
  (!displaySurface && settings?.width === screen.width && settings?.height === screen.height);
setIsEntireScreen(isMonitor);

Don't forget to add a test for the Firefox fallback path (where displaySurface is undefined but dimensions match the screen).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'm trying to hide the mirror effect for full screen sharing while keeping the Firefox PiP icon visible. Since the PiP icon seems to be browser-controlled and tied to the video element, hiding the video also hides the icon.
Do we want to:

  1. Fully hide the mirror and lose PiP, or
  2. Keep PiP visible and partially mask the screen?

What would you prefer from a UX perspective?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @Hossein-Movahed , just following up on this. Could you check the comment and let me know your thoughts when you have time?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd go with fully hiding the mirror. The mirror effect is a clear visual bug that affects all full-screen sharers, while PiP on a local screenshare preview is rarely used. The overlay already gives enough feedback. The trade off is worth it i guess

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it, thanks for the confirmation. I'll fully hide the video element.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi, I've addressed the requested changed - fixed firefox fallback. Also, resolved lint and conflict issues. Could you please take another look?

@Hossein-Movahed
Copy link
Copy Markdown
Contributor

vcr:deploy

@github-actions
Copy link
Copy Markdown
Contributor

✅ Deployed to VCR

URL: https://neru-ca37991c-vonage-video-react-app-vera-pr-381.euw1.runtime.vonage.cloud

Commit: a46d764e805175f2f274bcfc81f61c553f37d28c

View build logs

Copy link
Copy Markdown
Contributor

@Hossein-Movahed Hossein-Movahed left a comment

Choose a reason for hiding this comment

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

@ydumburs , we need the string local in de as well

@ydumburs ydumburs force-pushed the VIDSOL-599-hide-entire-screenshare branch from 863b15c to 37d0f29 Compare March 27, 2026 15:54
@ydumburs ydumburs requested a review from VZaphod March 27, 2026 16:46
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.

5 participants