-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: document fullscreenElement, pictureInPictureElement and visibilityState binding #8507
Conversation
fullscreenElement and visibilityState binding
@ThaUnknown is attempting to deploy a commit to the Svelte Team on Vercel. A member of the Team first needs to authorize it. |
fix: undefined properties feat: tests
d0af9f5
to
17a3d50
Compare
unrelated tests fail? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Since pictureInPictureElement
is not standardized (I think?) and since it sounds confusing in behavior, I suggest to just remove it from the PR - would that be ok?
it is standardized, see this, just not implemented by firefox, since they added their own implementation for it [for some reason], even safari/opera supports it
why? it behaves identically to fullscreenElement, like actually 1:1, I'd much prefer to keep it, again, especially for conditional UI like displaying a different button when PiP is active, or hiding a video element when a custom PiP implementation is used, I originally made this PR for the PiP element, not fullscreen/visibilityState, so I'd prefer if we could keep it in |
Is it a standard though? It's an editor's draft, not a recommendation, which I think means it's not officially standardized yet. I was also proposing to remove it because you said it's confusing that the new picture in picture API uses different things. |
the new document PiP API isn't standardised, and only supported by new versions of chromium, so that was more of a note of "not to be confused with" |
This is a working draft which still is not the recommended stage. I mean everyone except Firefox implemented it already anyway, so maybe it's fine. |
yeah, that's usually as far as new things will go, I agree that PiP isn't the most uuuuh agreed on thing, but there's very little stopping svelte from adding support for it, which would be nice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's still in working draft and since the current TypeScript definitions of this repo also don't contain it, I removed the picture-in-picture binding. The rest looks good, so I'll merge - thank you!
that's insanely sad |
The |
This PR adds binding for document's
fullscreenElement
,visibilityState
andpictureInPictureElement
, they are all however read only. This is useful for conditional UI which shows different things depending on fullscreen state etcConsiderations:
would actually be
but I don't think that's a good idea, so I'm leaving the properties read only.
window.documentPictureInPicture.window
[????] instead ofdocument.pictureInPictureElement
to report what element is active as the current PiP window, questionable decision imho