Lazy loading images

[mikegeyser]

Closes #351.

Changed the generated script, as well as all of the static pages, to use loading='lazy' for all images. I hope I wasn't too heavy handed with the loading="lazy", but it seems reasonable to me and doesn't affect the experience of the site in any way.

[tunetheweb]

Suggested removing lazy-loading from images that will mostly be above the fold.

[ymschaap]

Didn't see this ticket, but I also added lazyload on the files in here:

https://github.com/HTTPArchive/almanac.httparchive.org/tree/tables-lazyload-fonts

Let me know what works better (and I will remove).

[tunetheweb]

Didn't see this ticket, but I also added lazyload on the files in here:

https://github.com/HTTPArchive/almanac.httparchive.org/tree/tables-lazyload-fonts

Let me know what works better (and I will remove).

Well as per above conversation, I think your version is overly aggressive and prefer to be selective on which images are lazy-loaded for reasons discussed above.

Does anyone know if it's bad to over use lazy-loading or not? Raised this query to ask Google for clarification: GoogleChrome/web.dev#1850

[AymenLoukil]

s are lazy-loaded for reasons discussed abov

We could use eager for the hero images as described in the documentation of native lazy loading.

[tunetheweb]

s are lazy-loaded for reasons discussed abov

We could use eager for the hero images as described in the documentation of native lazy loading.

Changed my review above to do that!

[rviscomi]

Getting SyntaxError: Invalid regular expression flags when running this

[mikegeyser]

Hi @rviscomi, I'm not sure why that would be, I didn't think we were using anything esoteric. Could it be the /s (dotall) flag that requires a later node version? It seems pretty new.

This functionality seems to be causing quite a few problems, perhaps we shouldn't be doing this right now? We can abandon this PR if you think that would be better.

[tunetheweb]

Hi @rviscomi, I'm not sure why that would be, I didn't think we were using anything esoteric. Could it be the /s (dotall) flag that requires a later node version? It seems pretty new.

Works for me with node v12.13.0

This functionality seems to be causing quite a few problems, perhaps we shouldn't be doing this right now? We can abandon this PR if you think that would be better.

What's the problem with this functionality (other than this)? You're not getting it confused with the timestamp functionality are you?

[rviscomi]

Hmmm ok. If it works for everyone else it must be me. @mikegeyser could you resolve the merge conflicts and then we can ship this?

[tunetheweb]

Hmmm ok. If it works for everyone else it must be me. @mikegeyser could you resolve the merge conflicts and then we can ship this?

Isn't it kind of important for you to be able to build chapters? Did you try removing the s from /s to confirm if that was it? And what version of node are you on?

[rviscomi]

Here's the full error:

$ npm run generate
...
almanac.httparchive.org/src/tools/generate/lazy_load_images.js:5
const images_inside_figures = /<figure(?: [^<>]+)?>(?:(?:(?!<figure).)*)(<img)(?:(?:(?!<figure).)*)<\/figure>/gis;
                              ^

SyntaxError: Invalid regular expression flags

I'm using node v8.7.0.

---

I've updated to v12.13.0 and now the script runs but generates errors having to do with the HTML, for example:

Generating chapter: en, 2019, markup
SyntaxError: Unexpected closing tag "figure". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags (249:1)
  247 |     <iframe width="600" height="371" seamless="" frameborder="0" scrolling="no" src="https://docs.google.com/spreadsheets/d/e/2PACX-1vTbHgqcSepZye6DrCTpifFAUYxKT1hEO56585awyMips8oiPMLYu20GETuIE8mALkm814ObJyktEe2P/pubchart?oid=2141583176&amp;format=interactive"></iframe>
  248 |   <figcaption>Figure 3. Element frequencies as of 2019.</figcaption>
> 249 | </figure>
      | ^
  250 | <p>Comparing the latest data in Figure 3 to that of Hixie's report from 2005 in Figure 2, we can see that the average size of DOM trees has gotten bigger.</p>
  251 | <img loading="lazy"
  252 | <figure id="fig5">

Note the weird stray <img loading="lazy" on line 251. I do think something is broken with the script.

[tunetheweb]

I'm using node v8.7.0.

That explains it! Apparently you have to use the --harmony flag in Node v8 for dotall support. So like this I guess?

npm --harmony run generate

Or upgrade to a decent version of node and stop living in the past 😀

[mikegeyser]

Can we perhaps park this change for the moment? I don’t feel entirely comfortable with the PR, it feels like something isn’t quite right.

It’s not actually essential for the release, but could risk destabilization of the generation script over the weekend. I honestly feel like the most responsible thing to do would be to revisit it next week, with fresh eyes, when the rush is off.

[rviscomi]

@mikegeyser +1

[tunetheweb]

I've updated to v12.13.0 and now the script runs but generates errors having to do with the HTML, for example:

Generating chapter: en, 2019, markup
SyntaxError: Unexpected closing tag "figure". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags (249:1)
  247 |     <iframe width="600" height="371" seamless="" frameborder="0" scrolling="no" src="https://docs.google.com/spreadsheets/d/e/2PACX-1vTbHgqcSepZye6DrCTpifFAUYxKT1hEO56585awyMips8oiPMLYu20GETuIE8mALkm814ObJyktEe2P/pubchart?oid=2141583176&amp;format=interactive"></iframe>
  248 |   <figcaption>Figure 3. Element frequencies as of 2019.</figcaption>
> 249 | </figure>
      | ^
  250 | <p>Comparing the latest data in Figure 3 to that of Hixie's report from 2005 in Figure 2, we can see that the average size of DOM trees has gotten bigger.</p>
  251 | <img loading="lazy"
  252 | <figure id="fig5">

Note the weird stray <img loading="lazy" on line 251. I do think something is broken with the script.

I can't repeat this on this branch.