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 Fullscreen mode support in ViewerOverlay #9596

Merged
merged 3 commits into from
May 24, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented May 23, 2023

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

In the Fullscreen mode, Talk Vue app (#content-vue) is a top layer element. It is not possible to display any other elements outside the Talk app on top of the top layer using CSS.

Both Viewer and ViewerOverlayCallView are rendered outside the Talk app and had this problem. With Viewer it is solved now by a fixme hack with manual moving a DOM element with Viewer to the Talk app.

This PR fixes this problem in a similar way, in a fullscreen mode Teleport is used to move the ViewerOverlay not to the body, but to the Talk app (#content-vue).

A do not like this solution. Formally this is not correct to use Portal/Teleport to move something inside the Vue app. It should only be used to teleport something outside the app and Vue app content should never be mutated directly without Vue and its Virtual DOM.

Alternative solutions could be:

  1. Use the full version of the Portal library
    • Requires adding a heavy library
    • Doesn't solve the same problem with Viewer anyway
  2. Create and use a new child of #content-vue as Talk Vue app, so its parent could be in the Fullscreen allowing to render new elements there
    • Changes the Talk app layout by making #content-vue not a vue app or changes the Talk app server-side template. I'm afraid, it could break many things. I'd consider doing something like these in techdept later.

So I haven't found any better solution :(

🏁 Checklist

@ShGKme
Copy link
Contributor Author

ShGKme commented May 23, 2023

/backport to stable27

@ShGKme
Copy link
Contributor Author

ShGKme commented May 23, 2023

Tested in a call with and without participants in normal and Fullscreen modes

@nickvergessen
Copy link
Member

Work when:

  1. Fullscreen
  2. Join call
  3. Open viewer

Does not work when:

  1. Join call
  2. Open viewer
  3. Fullscreen

@ShGKme
Copy link
Contributor Author

ShGKme commented May 24, 2023

Does not work when:

  1. Join call
  2. Open viewer
  3. Fullscreen

Yes. But this is not related to the viewer overlay. It didn't work even without overlay, because the viewer is moved only once on opening.

Do you prefer to fix this problem in this PR together with the overlay bug?

@ShGKme
Copy link
Contributor Author

ShGKme commented May 24, 2023

Hmm, Portal also doesn't react to the target selector update. So a little bit related.

@ShGKme
Copy link
Contributor Author

ShGKme commented May 24, 2023

A new commit adds support to the fullscreen mode toggle after overlay opening.

But it still requires a fix for the same issue with the viewer.

@ShGKme ShGKme merged commit f8070eb into master May 24, 2023
@ShGKme ShGKme deleted the fix/9444/viewer-overlay-in-fullscreen branch May 24, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewer overlay doesn't support full screen mode
2 participants