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

Fixed bug where videos were not showing up on Safari #3562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tyleralsbury
Copy link
Contributor

PR Summary:

The video section was not loading Youtube videos properly on Safari. This is because of a rendering bug with iframes that made it so that the video just didn't show up - audio would play, the element would exist, but it just wouldn't render. This fixes it.

What approach did you take?

The element needs to be repainted to have it show up. So I just apply a display value into the style attribute, then put back whatever was there before (should usually be nothing, but just in case). That part needs to be done in a setTimeout to force it to execute after the completion of the part that adds display: block, otherwise it happens before the repaint and doesn't work. See mdn for details on how that stuff works.

Other considerations

It shouldn't affect other things that use DeferredMedia since it's just adding and removing a style really quickly, but we should thoroughly test that.

Testing steps/scenarios

  • On Dawn 15.0.0, add a video section and put a Youtube video in
  • Click to load the video.
  • Notice that it doesn't work.
  • On this branch, do the same
  • Notice it does! Yay!

Demo links

@@ -710,6 +710,12 @@ class DeferredMedia extends HTMLElement {
// force autoplay for safari
deferredElement.play();
}

Choose a reason for hiding this comment

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

Maybe adding a comment here like // Workaround for safari iframe bug, would prevent devs from thinking this is a useless block.

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.

2 participants