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 uploaded videos don't load on safari #1593

Merged
merged 4 commits into from
Nov 17, 2024

Conversation

Soxasora
Copy link
Contributor

Description

By removing an incompatible-between-renders width: min-content, uploaded videos now show and play on Safari for iOS and macOS, fixing #1569.

Screenshots

Safari on macOS:
Safari on macOS

Safari on iOS/PWA:
Safari on iOS

Additional Context

Removing width: min-content didn't cause any structural change and actually didn't do anything (?) other than satisfy Safari.

In the issue I talked about media-or-link.js

rendering all possible resolutions of an image that actually is a video

I can confirm that this wasn't the problem and it was just that CSS line

Checklist

Are your changes backwards compatible? Please answer below:
Yes, this edit doesn't break functionality nor UX/UI

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7, I tested on various browsers for various platforms, this edit didn't change whatsoever the past behavior on other browsers.

For frontend changes: Tested on mobile? Please answer below:
Tested mainly on iOS, videos now load effortlessly

Did you introduce any new environment variables? If so, call them out explicitly here: No

@ekzyis ekzyis self-requested a review November 15, 2024 14:36
@ekzyis
Copy link
Member

ekzyis commented Nov 15, 2024

I can confirm videos now load on Safari iOS sometimes but more often than not, they are detected as images that only take up space:

loads correctly as video:

signal-2024-11-15-213948

loads as image:

signal-2024-11-15-213951

For some reason, it initially seemed to work more as video (but I think once there were no controls) than not but now it always loads as an image 🤔

Does this also happen on your end?

I also applied this patch for testing since the local imgproxy is not able to process videos:

diff --git a/components/item-full.js b/components/item-full.js
index 5a148ea3..e610d6af 100644
--- a/components/item-full.js
+++ b/components/item-full.js
@@ -65,7 +65,7 @@ function ItemEmbed ({ url, imgproxyUrls }) {
     const srcSet = imgproxyUrls?.[url]
     return (
       <div className='mt-3'>
-        <MediaOrLink src={src} srcSet={srcSet} topLevel linkFallback={false} />
+        <MediaOrLink src={src} topLevel linkFallback={false} />
       </div>
     )
   }
diff --git a/components/text.js b/components/text.js
index 43b9a387..c83728a8 100644
--- a/components/text.js
+++ b/components/text.js
@@ -212,7 +212,7 @@ function MediaLink ({

   const srcSet = imgproxyUrls?.[url]

-  return <MediaOrLink srcSet={srcSet} src={src} rel={rel} {...props} />
+  return <MediaOrLink src={src} rel={rel} {...props} />
 }

 function Table ({ node, ...props }) {

Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

forgot to publish the above as a review

@Soxasora
Copy link
Contributor Author

Soxasora commented Nov 15, 2024

Does this also happen on your end?

Yes, that's why I held back this PR for so long, if you check the console you can see that it throws 422 (Unprocessable Entity) from imgproxy and Safari just stops there lol

edit: I think that in prod this shouldn't (?) happen

@ekzyis
Copy link
Member

ekzyis commented Nov 15, 2024

Does this also happen on your end?

Yes, that's why I held back this PR for so long, if you check the console you can see that it throws 422 (Unprocessable Entity) from imgproxy and Safari just stops there lol

edit: I think that in prod this shouldn't (?) happen

I updated my comment and added a patch for this, I think this is unrelated. The 422 error happens for any video that was uploaded locally once srcSet is set which means the imgproxy processed the link.

I think that in prod this shouldn't (?) happen

Yes, we have the pro version in prod but @huumn knows more about this

@Soxasora
Copy link
Contributor Author

I updated my comment and added a patch for this, I think this is unrelated.

Tried it just now and it works which indicates that this may be inconsistent, could be the fact that it tries to load the video as image before replacing it with video?

@ekzyis
Copy link
Member

ekzyis commented Nov 15, 2024

Tried it just now and it works which indicates that this may be inconsistent, could be the fact that it tries to load the video as image before replacing it with video?

Yeah something like this might be the case but it's hard to tell without a browser console on mobile. USB debugging might be required for this issue.

I wouldn't be surprised if you need a Mac for this though ...

@Soxasora
Copy link
Contributor Author

I wouldn't be surprised if you need a Mac for this though ...

I am using a Mac indeed because I didn't know better but you sparked my curiosity and there are tools like Inspect that should do this.

Adding to inconsistencies, it seems like that iOS stops at the image stage and macOS can reach the video stage (of the media-or-link pipeline)

@huumn
Copy link
Member

huumn commented Nov 16, 2024

Cool! IIRC I put this there for image/video gridding. So I'll just need to verify this doesn't break that.

@Soxasora
Copy link
Contributor Author

Soxasora commented Nov 16, 2024

So I'll just need to verify this doesn't break that.

It does, sorry didn't think of uploading more videos

This is the behavior with min-content
with min-content

And this is the behavior without min-content
without min-content

I found out that with min-content videos will be shown if you force reload the page (just noticed thanks to pull-to-refresh), if you follow the router flow it won't be shown unless you disable min-content.
I suspect that Safari stops rendering after loading the "image", disabling min-content would prevent Safari from calculating it on the image instead of the video

Safari also consistently will just wait for the "image" to finish playing (in my case 30s videos) and then render it as video, this is because of how Safari handles media rendering. If the image is not retrieved or validated, it won't go further.

@Soxasora
Copy link
Contributor Author

I can confirm that Safari queues the video tag after the image has finished playing (???lol)

As a proof-of-concept I flipped the image-or-video logic and added a dynamic min-content.
Safari/PWA will load the video reliably and also respect min-content for content-sizing:
photo_2024-11-16_11-56-11

@huumn
Copy link
Member

huumn commented Nov 16, 2024

For some reason, videos don't load for me in desktop safari now. Left if brave. Right is desktop safari.

lol I was still on master

@huumn
Copy link
Member

huumn commented Nov 17, 2024

Nice work! Do you have a lightning address I can send sats to?

@huumn huumn merged commit c08088b into stackernews:master Nov 17, 2024
3 checks passed
@Soxasora
Copy link
Contributor Author

Thanks! Here's the address: soxasora@blink.sv

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.

3 participants