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

Delay caching first page until after DOM load event #475

Closed
mevanloon opened this issue Aug 15, 2022 · 15 comments · Fixed by #480
Closed

Delay caching first page until after DOM load event #475

mevanloon opened this issue Aug 15, 2022 · 15 comments · Fixed by #480
Labels

Comments

@mevanloon
Copy link

When a first page-load has <style> tags in a container used by Swup, it will not add them to the cache. Navigating back will result in the page not having the <style> tags that it initially existed.

This issue does not present itself with subsequent page-loads, loaded by mouse-over, for example. It that case, the tags get added to the cache properly.

Workaround

For now, you can turn off the cache:

const options = {
  cache: true
}

This isn't ideal (the caching Swup does is the reason I'm using it, and it rocks when it works), but this will prevent any lost styles. The issue is especially pungent on Wordpress 5.9+ sites that insert dynamic <style> blocks.

@hirasso
Copy link
Member

hirasso commented Aug 15, 2022

Thanks for reporting this! I just tried to reproduce your problem but in my case it works as expected. Could you post a reduced version of your markup and the code that you are using to initialize swup? Even better would be a reduced testcase on glitch.com. Thanks!

@hirasso
Copy link
Member

hirasso commented Aug 15, 2022

Ok, I took this opportunity to create a template from where you can start testing your issue:

https://codesandbox.io/s/swup-test-case-template-6ci5si

Please create a fork of this sandbox and post a link that reproduces your issue here.

@mevanloon
Copy link
Author

Hey! Thanks for taking the effort of making a sandbox.

I've forked it and added style tags to try and replicate the issue, like so on index.html:

    <div id="swup" class="transition-main">
      <p>
        This is a template for creating reduced test cases for
        <a href="https://swup.js.org">Swup</a>. Please fork this sandbox, try to
        reproduce your issue and post the link to the
        <a href="https://github.com/swup/swup/issues/">swup issues</a>.
      </p>
      <p>
        Lorem ipsum dolor sit amet consectetur, adipisicing elit. Doloribus,
        veniam odit hic eos deleniti iure corrupti fugit ad commodi? Magni eius
        eligendi adipisci tempore provident minus nobis corrupti esse ex!
      </p>
      <style>
        body {
          font-weight: bold!important;
        }
      </style>
    </div>

Strange thing is that it replicated the first few times (home page load, navigate to and from page 1, homepage now not having bold text anymore), but I couldn't get it te replicate again after that 🤨

I kept fiddling with it, but I'm not sure what exactly is the issue here.

@daun
Copy link
Member

daun commented Aug 16, 2022

Thanks for providing a sandbox @mevanloon. I just tried it out and it's working as expected in most of the browsers on my system. Any other idea what this could be related to? Browser cache? Service worker? Syntax error in the generated style tag?

@mevanloon
Copy link
Author

My first instinct would point to Swup's way of caching. I am again fiddling with it on my machine, and when looking at Swup's own cache (notably, swup.cache.last, which is the only entry) notice that on the first page load, it did not have any style tags in it. Neither .originalContent or the entry in .blocks.

Also notable is that this is the third container (my footer) where this is happening -- data-swup="2".

@daun
Copy link
Member

daun commented Aug 16, 2022

Sounds like your style tags were added/moved/removed in JS. Swup will cache the first page but it will cache the state of the DOM on load, with all modifications that happened before it got called. You could look into the order of initialization of your scripts. I tend to be on the safe side and just uncache the first page for that reason:

  const swup = new Swup({ ...options })

  // Empty cache so the first view isn't cached with modified dom
  swup.cache.empty()

@mevanloon
Copy link
Author

That's a good point, but isn't what's happening. I checked the source (the raw html, not the Spector), and the style tags are there.

I am one step further to finding it: it cuts off after a script tag with a src attribute. Without that script tag, the style tags are included in the case. Still trying to replicate it in a controlled environment.

@daun
Copy link
Member

daun commented Aug 16, 2022

Have you run it through a HTML validator? Feel free to paste the original source here.

@mevanloon
Copy link
Author

Did some more fiddling, and it turns out that it's not just style tags, but every element after the script tag, though proper HTML hierarchy is preserved. I tried putting the same script tag in the testing sandbox, but it just keeps working there.

To note: it is the script tag that contains the Swup code. Ran my HTML through the W3 validator, but it doesn't return anything of note (warnings about attributes that aren't needed mostly).

UPDATE: Found it! It has to do with running the script before the rest of the document has loaded. This is assumed to always be after the DOM has loaded, but this isn't always the case. That's why document.documentElement.outerHTML (which Swup uses to get the original content) didn't contain anything past the script, since it obviously wasn't loaded.

This is also why the sandbox environment didn't show the same issue: it encapsulates the scripts after a load event by itself (which is notably the reason that load events will not fire).

Possible fixes could be to check if the load event has already fired, or only adding the current page to the cache after the whole page has loaded. In my case, I'll be making sure that Swup loads after the window's load event.

@daun
Copy link
Member

daun commented Aug 16, 2022

Oh nice, good catch! This is indeed something we should consider when caching the first page. I'll rename the issue to reflect that.

@daun daun changed the title <style> tags are not cached on initial page load Delay caching first page until after DOM load event Aug 16, 2022
@daun
Copy link
Member

daun commented Aug 16, 2022

As a side note: the script that loads swup itself should probably always be outside of any containers that swup replaces, just to be on the safe side.

@mevanloon
Copy link
Author

Good idea. Didn't think it'd be this big, but this could have a bunch of unwanted side-effects, so it may be a good idea to wrap it in a little onLoad.

@mevanloon
Copy link
Author

As a side note: the script that loads swup itself should probably always be outside of any containers that swup replaces, just to be on the safe side.

Yeah, that makes sense. The reason it's in there is because Wordpress prints all its footer scripts and style tags for the page at the same spot (a hook called wp_footer). I could decouple that with some wrangling, but eh, why bother if the script won't execute again anyway, right? My fix in the meantime is to make sure this script echoes at the very end, after anything else.

@daun daun added the bug label Aug 16, 2022
This was referenced Aug 16, 2022
@hirasso
Copy link
Member

hirasso commented Aug 23, 2022

This should be fixed in the next release.

@mevanloon
Copy link
Author

That's some fast turnaround on the bug fixing gang, well done, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants