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

Fixes #2919 Fixes #2673 Fixes #2641 Hide Youtube fixes #3394

Merged
merged 4 commits into from
May 28, 2020

Conversation

keianhzo
Copy link
Collaborator

@keianhzo keianhzo commented May 19, 2020

Fixes #2919 Fixes #2947 Fixes #2673 Hide YouTube overlays in immersive

@keianhzo keianhzo self-assigned this May 19, 2020
@keianhzo keianhzo requested a review from bluemarvin May 19, 2020 17:20
Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

This PR seems to break auto immersive 360 video.

STR:

  1. Click play on Mickey & Minnie's Runaway Railway
  2. Click on fullscreen icon

Actual results:
Video goes fullscreen

Expected results:
Video goes into immersive 360 mode.

@bluemarvin
Copy link
Contributor

This also doesn't seem to address #2673 as when I manually select a projection, it stays in that projection for the next video that is played.

@keianhzo keianhzo added this to the #11 polish milestone May 20, 2020
@keianhzo
Copy link
Collaborator Author

keianhzo commented May 20, 2020

@bluemarvin I Don't seem to be able to reproduce the fullscreen issue with the provided STRs.

Regarding:

This also doesn't seem to address #2673 as when I manually select a projection, it stays in that projection for the next video that is played.

AFAIK that's always been like that. We currently auto-select the projection based on a custom mozVideoProjection URL param that we append to the URL when the user plays the video, then we parse it when entering immersive video mode.
If another video is played while in immersive, we don't update the video projection. We could add webext communication to notify on the new playback start and send the projection parameter to update the projection mode.

@bluemarvin
Copy link
Contributor

@bluemarvin I Don't seem to be able to reproduce the fullscreen issue with the provided STRs.

Regarding:

This also doesn't seem to address #2673 as when I manually select a projection, it stays in that projection for the next video that is played.

AFAIK that's always been like that. We currently auto-select the projection based on a custom mozVideoProjection URL param that we append to the URL when the user plays the video, then we parse it when entering immersive video mode.
If another video is played while in immersive, we don't update the video projection. We could add webext communication to notify on the new playback start and send the projection parameter to update the projection mode.

@keianhzo It has always been that way as far as I know. That's why I was not clear how this PR addresses #2673. If we don't plan to address it, we should close with a won't fix tag. I'll review again to see if I can reproduce the issue I was seeing.

Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

I don't see any breakages but I'm not sure what this is fixing.

@keianhzo
Copy link
Collaborator Author

keianhzo commented May 28, 2020

@bluemarvin I've updated I was missing a case and you were probably hitting it. I've also added a fix for a missing div in #2947

@keianhzo keianhzo requested a review from bluemarvin May 28, 2020 09:59
@keianhzo keianhzo changed the title Fixes #2673 Hide Youtube overlays in immersive Fixes #2947 Fixes #2673 Hide Youtube overlays in immersive May 28, 2020
@keianhzo keianhzo changed the title Fixes #2947 Fixes #2673 Hide Youtube overlays in immersive Fixes #2947 Fixes #2673 Fixes #2641 Hide Youtube overlays in immersive May 28, 2020
@keianhzo keianhzo changed the title Fixes #2947 Fixes #2673 Fixes #2641 Hide Youtube overlays in immersive Fixes #2919 Fixes #2673 Fixes #2641 Hide Youtube overlays in immersive May 28, 2020
@keianhzo keianhzo changed the title Fixes #2919 Fixes #2673 Fixes #2641 Hide Youtube overlays in immersive Fixes #2919 Fixes #2673 Fixes #2641 Hide Youtube fixes May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment