XKit-Rewritten icon indicating copy to clipboard operation
XKit-Rewritten copied to clipboard

Panorama: Fix media embed (youtube) aspect ratios

Open marcustyphoon opened this issue 3 years ago • 4 comments

Concept

Right now, Youtube iframes are embedded with a hardcoded max-width based on post width (540px) and a hardcoded height calculate in pixels based on the video aspect ratio. Panorama overrides the max-width, but the height stays the same, resulting in the iframe not being tall enough.

Our supported browsers all support aspect-ratio (https://css-tricks.com/almanac/properties/a/aspect-ratio/), so we can fix this without the percentage vertical padding hack. My thought was adding a mutation listener targeting the youtube iframe, calculating its intended aspect ratio from the inline styles on the element, then overriding height to unset !important and setting aspect-ratio. I guess we don't want to overwrite the inline height property so this would have to be via a classname and a dynamic style element?

Unless, that is, all youtube embed iframes on the site are 16:9 regardless of their contents, which would make this very simple.

marcustyphoon avatar Nov 22 '22 23:11 marcustyphoon

const a = t.height / t.width,
  l = e === k.G6.Youtube,
  d = Math.min(T().postWidth, o),
  c = l
    ? {
        maxWidth: d,
        height: a * d,
      }
    : {},
  h = {
    src: t.url,
    title: e,
    allowFullScreen: !0,
    className: n()(Z, {
      [R]: !l,
    }),
    style: c,
  };

marcustyphoon avatar Nov 22 '22 23:11 marcustyphoon

This issue is unconfirmed, and has been labelled as stale due to inactivity. It will be closed automatically if no further activity occurs.

A project maintainer can mark an issue as confirmed by adding the help wanted label, the wontfix label, or an assignee.

stale[bot] avatar Dec 23 '22 02:12 stale[bot]

I guess we don't want to overwrite the inline height property so this would have to be via a classname and a dynamic style element?

Nope, we can set and unset a CSS property. Very satisfying solution, IMO.

(Not a PR yet because it would merge conflict all over the place.)

const aspectRatioVar = '--panorama-aspect-ratio';

// style element
`iframe[style*="${aspectRatioVar}"] {
  aspect-ratio: var(${aspectRatioVar});
  height: unset !important;
}`

const processVideoIframes = iframes => iframes.forEach(iframe => {
  const { maxWidth, height } = iframe.style;
  iframe.style.setProperty(
    aspectRatioVar,
    `${maxWidth.replace('px', '')} / ${height.replace('px', '')}`
  );
});

// main()
pageModifications.register(
  `${keyToCss('videoBlock')} iframe[style*="max-width"][style*="height"]`,
  processVideoIframes
);

// clean()
pageModifications.unregister(processVideoIframes);
[...document.querySelectorAll(`iframe[style*="${aspectRatioVar}"]`)].forEach(el =>
  el.style.removeProperty(aspectRatioVar)
);

marcustyphoon avatar Feb 22 '23 09:02 marcustyphoon

Note to self: you need to do the exact opposite thing for Spotify embeds, too, as they currently have a fixed aspect ratio but need a fixed height, wheeeeeee

marcustyphoon avatar Mar 21 '23 21:03 marcustyphoon