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(fonts): wait for fonts to be fully loaded #337

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicojs
Copy link
Contributor

@nicojs nicojs commented Jun 20, 2024

Fixes #336

@nicojs
Copy link
Contributor Author

nicojs commented Jun 20, 2024

This seems to work, but it doesn't solve my original problem.

There is an issue with slide 18:
http://localhost:3000/input/revealjs-demo/#/18 (after running node test/run-server.js 3000).

image

In the demo pdf, you can see that a different font is used. I'm pretty sure this is a race condition. The page is loading the font in an iframe and the screenshot is taken before those fonts are loaded.

image

I can see 2 solutions:

  • Support iframes properly. This is difficult since we cannot "see" when exactly an iframe is loaded, but maybe we can leverage the load event?
  • Don't support iframes, in which case we could better update the test not to have an iframe background.

@astefanutti what do you think?

@astefanutti
Copy link
Owner

I'd lean towards trying to support iframes if that's not too convoluted. Otherwise I agree we can fallback to updating the test as you suggested.

@nicojs
Copy link
Contributor Author

nicojs commented Jun 20, 2024

I did a quick test, but it won't work. When loading cross-origin iframes, it seems impossible to know when the frame is loaded. The iframe.contentDocument.readyState is always 'complete', no matter what the actual readyState is. This is for security concerns.

I can add this code for the same-origin iframes. It might actually be useful. What do you think?

Detect frame loaded code
/** @param {import('@playwright/test').Page} page */
async function loaded(page) {
  return page.evaluate(
    async () => {
      const frames = document.querySelectorAll("iframe");
      const promises = [];
      for (const frame of frames) {
        if (frame.checkVisibility()) {
          console.log("visible iframe detected!");
          const doc = frame.contentDocument;
          if (!doc) {
            console.log("no contentDocument!");
            continue;
          }
          promises.push(
            new Promise((res) => {
              if (doc.readyState === "complete") {
                console.log("was already complete!");
                res();
              } else {
                function eventListener() {
                  frame.removeEventListener("load", eventListener);
                  console.log("not yet complete, but now is!");
                  res();
                }
                console.log("waiting for load!", frame.readyState);
                frame.addEventListener("load", eventListener);
              }
            })
          );
        }
      }
      await Promise.all(promises);
    }
  );
}

@nicojs
Copy link
Contributor Author

nicojs commented Jun 20, 2024

This seems to work, but it doesn't solve my original problem.

There is an issue with slide 18:

No matter what I do, the screenshot always comes out this way. I think it has to do with the viewport being too small, so unrelated to fonts or iframes loading. What do you want to do with all this waiting code? We can simply abandon all this or add it... up to you 🤷‍♂️

@astefanutti
Copy link
Owner

What do you want to do with all this waiting code? We can simply abandon all this or add it... up to you 🤷‍♂️

For the iframes part, I'd be inclined to keep things simple and avoid introducing complex logic that we don't know if it fixes issues, or provide any real benefits.

For waiting for the fonts to be loaded, I think that's redundant with the pause at each next slide. Ideally that pause should be removed, and the logic only rely on such events, but I'm not sure what are the set of events it takes to be sure the slide is ready for export.

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.

Bug: Decktape doesn't wait on webfonts being loaded
2 participants