fix: refactor fullscreen-api and state mediator to make fewer assumpt… #946
Conversation
…ions on env-specific divergences. Fixes bug in modern Safari iOS fullscreen.
|
@cjpillsbury is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
| } | ||
| }; | ||
|
|
||
| const exitFullscreenKey = |
There was a problem hiding this comment.
Per out of band convo, should these "prefer" browser prefix versions or standard versions?
There was a problem hiding this comment.
like this seems good to me, prefer prefix-less
There was a problem hiding this comment.
Yeah, can't say I'm thinking too deeply on this one, but I also prefer prefixless, in the hopes we can just delete code later without changing much more.
There was a problem hiding this comment.
Yeah for me I just wasn't sure if there was an expectation that, if both are around, which would be more reliable. That said, I think the status quo wins by vote, so I'm gonna mash that merge.
cjpillsbury
force-pushed
the
fix/update-fullscreen-ios-reliability
branch
from
d38e3ab
to
9444718
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #946 +/- ##
======================================
Coverage ? 72.35%
======================================
Files ? 79
Lines ? 16213
Branches ? 0
======================================
Hits ? 11731
Misses ? 4482
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Did basic enter/exit smoke testing on:
Not sure if we have a way to validate the Safari Mobile (iOS) < 16.4 specific logic. Should be roughly equivalent, but maybe spend a few extra ticks on code review reasoning through that if we can't validate via tests. Also please do your own smoke testing (even if on the PR deploy apps) just for more eyes. |
| */ | ||
|
|
||
| /** @type {(stateOwners: StateOwners) => Promise<undefined> | undefined} */ | ||
| export const enterFullscreen = (stateOwners) => { |
There was a problem hiding this comment.
This is definitely a nit pick, so take it or leave it, but if we're keeping these as Utils it'd feel better if they weren't tied to the concept of stateOwners and media + fullscreenEl were specific arguments.
There was a problem hiding this comment.
Yeah I def get that intuition. Here was my reasoning:
- If you squint, this is just a common convention for passing in multiple optional arguments in JS (an object with optional key + value pairs).
- The "shape" of that argument object happens to use the same names as
StateOwners, in large part out of convenience (since, aside from one case in platform checks, it's only used by the Media Store architecture) - Even there, it isn't identical to
StateOwners, because it only expects/requires a small subset of what has to be defined on them
I'm gonna leave for now but we can certainly revisit in next week's MCOH (feels like it would fit in the "best practices" category if we wanna define something either way here)
…screen. (#958) Blocked by muxinc/media-chrome#946 Fixes #863 Smoke tested 👍 on both iOS Safari and iOS Chrome, plus desktop browsers for regression testing.
#946) …ions on env-specific divergences. Fixes bug in modern Safari iOS fullscreen.
…ions on env-specific divergences. Fixes bug in modern Safari iOS fullscreen.