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

Unexpected onLoad attribute behaviour in next/head #12984

Closed
joe-bell opened this issue May 16, 2020 · 15 comments
Closed

Unexpected onLoad attribute behaviour in next/head #12984

joe-bell opened this issue May 16, 2020 · 15 comments
Labels
good first issue Easy to fix issues, good for newcomers please add a complete reproduction Please add a complete reproduction.

Comments

@joe-bell
Copy link
Contributor

joe-bell commented May 16, 2020

Bug report

Describe the bug

In my project raam I'm tying set "Inter" as my font by linking to a stylesheet from Google Fonts.

In Head (a component that extends next/head) I'm using:

<link rel="preconnect" href="https://fonts.gstatic.com/" crossOrigin="" />
<link
  href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
  rel="stylesheet"
/>

(note the preconnect to the actual font assets, then the display=swap which is intended to show the fallback font until the loaded font is swapped in)

When I then go to view the page (in Firefox particularly) I see a flash of unstyled/invisible text:
https://raam-git-chore-inter.joebell.now.sh/

Screenshots

ezgif-2-8ea6320f12f1

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Go to https://raam-git-chore-inter.joebell.now.sh/ (in Firefox where this is more visible)
  2. Hard reload and watch the behaviour of fonts

Expected behavior

The system font is seen first, and 'swapped' to Inter on load.

System information

  • OS: macOS
  • Browser (if applies): Firefox 75
  • Version of Next.js: 9.3.3
  • Version of Node.js: 13.8.0

Additional context

I appreciate there's a lot of variables here: Theme UI, Google Fonts, Now.sh, Next.js, but it would be good if we can figure out the root cause here for others using Next.js. If it's something that could be resolved by adding some documentation I'd be more than happy to do so.

Thanks again for all of the team's hard work on Next.js, it's a real pleasure to work with

Prior reading

Example

@joe-bell
Copy link
Contributor Author

joe-bell commented May 16, 2020

Did some testing on other devices: iPad OS and iOS have no issues. Seems like it only occurs on macOS but I can’t verify Windows. I've tried removing the Theme UI hook to see if this was the root, but it appears to make no difference.

@joe-bell
Copy link
Contributor Author

joe-bell commented May 17, 2020

As an update I've tested this new approach, which is a mix of font-loading performance suggestions I've found online:

<link
  rel="preload"
  href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
  as="style"
/>
<link
  href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
  rel="stylesheet"
  media="print"
  // // Next.js doesn't like this but it allows us to load CSS asynchronously
  // @ts-ignore
  onLoad="this.media='all'"
/>
Before After
Screenshot 2020-05-17 at 13 40 49 Screenshot 2020-05-17 at 13 58 06

However a slight flash still remains and TypeScript doesn't like my onLoad="this.media='all'" as it doesn't accept string values. Am I doing this wrong or do I need to raise a PR to update types?

I have a hunch that something in next/head is changing after load on the client, but I'm not experienced enough to know what's happening under the hood to cause this FOUT.


Update #1 - I've tried fg-loadCSS but the same FOUT issue occurs. Whatever approach there is still a small FOUT.

@joe-bell joe-bell changed the title Flash of Invisible/Unstyled Text When Using Custom Fonts Flash of Invisible/Unstyled Text May 18, 2020
@joe-bell
Copy link
Contributor Author

After some further investigation with @csswizardry, we found that when JS is disabled the onload attribute is missing entirely:

Screenshot 2020-05-18 at 16 47 29

What's even stranger is once JS is enabled, this behaves as expected but moves further down the <head> element:

Screenshot 2020-05-18 at 16 48 03

This leads me to think next/head is rendering without onLoad on the server, then shuffling around things upon hydration

@joe-bell joe-bell changed the title Flash of Invisible/Unstyled Text Unexpected onLoad attribute behaviour in next/head May 25, 2020
@timneutkens
Copy link
Member

This leads me to think next/head is rendering without onLoad on the server, then shuffling around things upon hydration

onload is handled by React so I assume it's read as if it's a React event handler.

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels May 25, 2020
@joe-bell
Copy link
Contributor Author

joe-bell commented May 25, 2020

Thanks for getting back to me @timneutkens, I was just about to update this issue as I've managed to build a workaround solution in the meantime: next-google-fonts

I'd still prefer to use onload, but it seems fine for now

@willemmulder
Copy link

Copied from #16065 where I have a problem related to what OP is trying to do. If I want to use

<link rel="preload" href="https://..." as="style" onload="this.onload=null;this.rel='stylesheet'">
<noscript><link rel="stylesheet" href="https://...></noscript>

to make loading CSS async, that does not work because I'd have to rework that to using onLoad with a function, but that function is never executed. So

  1. Is there a way to just insert the above HTML as-is, without having to comply to JSX?
  2. If not, is there a way to make onLoad work?
  3. If not, is there another easy way to make external CSS load async?

wilf312 referenced this issue in wilf312/blog-drill-dev Dec 10, 2020
@dsacramone
Copy link

Copied from #16065 where I have a problem related to what OP is trying to do. If I want to use

<link rel="preload" href="https://..." as="style" onload="this.onload=null;this.rel='stylesheet'">
<noscript><link rel="stylesheet" href="https://...></noscript>

to make loading CSS async, that does not work because I'd have to rework that to using onLoad with a function, but that function is never executed. So

  1. Is there a way to just insert the above HTML as-is, without having to comply to JSX?
  2. If not, is there a way to make onLoad work?
  3. If not, is there another easy way to make external CSS load async?

Did you find a solution to this? I have the same need.

@willemmulder
Copy link

@dsacramone Nope, I did not find anything yet. Maybe the @tsignore annotation referenced in the 'fix build' commit above works?

@ssylvia
Copy link

ssylvia commented Mar 16, 2021

onload is handled by React so I assume it's read as if it's a React event handler.

@timneutkens At least on the client, the react elements are converted to regular DOM elements and all props are converted regular HTML attributes:

function reactElementToDOM({ type, props }: JSX.Element): HTMLElement {
const el: HTMLElement = document.createElement(type)
for (const p in props) {
if (!props.hasOwnProperty(p)) continue
if (p === 'children' || p === 'dangerouslySetInnerHTML') continue
// we don't render undefined props to the DOM
if (props[p] === undefined) continue
const attr = DOMAttributeNames[p] || p.toLowerCase()
if (
type === 'script' &&
(attr === 'async' || attr === 'defer' || attr === 'noModule')
) {
;(el as HTMLScriptElement)[attr] = !!props[p]
} else {
el.setAttribute(attr, props[p])
}
}

This causes the scope of the function to be lost as it is converted to a string with the el.setAttribute. Also, due to the race condition, we would need to make sure the onload is set before the href is set.

@felipedeboni
Copy link

felipedeboni commented Mar 30, 2021

Following seems to work

<style dangerouslySetInnerHTML={{
  __html: `</style>
    <link
      rel="stylesheet"
      href="https://...."
      media="print"
      onload="this.media = 'all';"
    />
    <style>`
}}></style>

@balazsorban44
Copy link
Member

Next.js comes with Font Optimization out-of-the-box now, this should not be necessary.

Please check out our docs: https://nextjs.org/docs/basic-features/font-optimization

@ssylvia
Copy link

ssylvia commented Jan 12, 2022

@balazsorban44 Please reopen this issue. There are several other use cases besides fonts that would benefit from the onLoad case. Also, font optimization only works if the font is known at build time or if it comes from Google Fonts and Typekit. For example, our team runs a website builder on NextJS. The website creators can choose a Google Font configuration per site which is not known at build time. There are also several other popular web font loaders that are not included in font-optimization.

@balazsorban44 balazsorban44 reopened this Jan 12, 2022
@balazsorban44
Copy link
Member

balazsorban44 commented Jan 12, 2022

Could someone add a reproduction that I could clone and test with the newest version of Next.js? (as of writing 12.0.8)? I only see a deployed version that is more than a year old.

I'm happy to investigate further then.

@balazsorban44 balazsorban44 added the please add a complete reproduction Please add a complete reproduction. label Jan 12, 2022
@balazsorban44
Copy link
Member

This issue has been automatically closed after 30 days of inactivity with no reproduction. If you are running into a similar issue, please open a new issue with a reproduction. Thank you.

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers please add a complete reproduction Please add a complete reproduction.
Projects
None yet
Development

No branches or pull requests

8 participants