From dec507c977b4ac012531761a0cc1c332259de7c3 Mon Sep 17 00:00:00 2001
From: Mateusz Krzeszowiak <mateusz.krzeszowiak@gmail.com>
Date: Tue, 7 Mar 2023 11:48:17 +0100
Subject: [PATCH] Improve rule for lazy loading to prevent developers from
 overusing it

---
 docs/checks/img_lazy_loading.md            | 12 +++++++-----
 lib/theme_check/checks/img_lazy_loading.rb |  6 +-----
 test/checks/img_lazy_loading_test.rb       |  4 ++--
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/docs/checks/img_lazy_loading.md b/docs/checks/img_lazy_loading.md
index 0b231e7e..ec5af075 100644
--- a/docs/checks/img_lazy_loading.md
+++ b/docs/checks/img_lazy_loading.md
@@ -8,17 +8,19 @@ Very often, webpages contain many images that contribute to data-usage and how f
 
 _Quoted from [MDN - Lazy loading][mdn]_
 
+As a general rule you should apply `loading="lazy"` to elements that are **not initially visible** when the page loads. Only images that require user interaction (scrolling, hovering, clicking etc.) to be seen can be safely lazy loaded without negatively impacting the rendering performance.
+
 ## Check Details
 
-This check is aimed at defering loading non-critical images.
+This check is aimed at deferring loading non-critical images.
 
 :-1: Examples of **incorrect** code for this check:
 
 ```liquid
 <img src="a.jpg">
 
-<!-- Replaces lazysize.js -->
-<img src="a.jpg" class="lazyload">
+<!-- Replaces lazysizes.js -->
+<img data-src="a.jpg" class="lazyload">
 
 <!-- `auto` is deprecated -->
 <img src="a.jpg" loading="auto">
@@ -29,7 +31,7 @@ This check is aimed at defering loading non-critical images.
 ```liquid
 <img src="a.jpg" loading="lazy">
 
-<!-- `eager` is also accepted, but prefer `lazy` -->
+<!-- `eager` is also accepted -->
 <img src="a.jpg" loading="eager">
 ```
 
@@ -44,7 +46,7 @@ ImgLazyLoading:
 
 ## When Not To Use It
 
-If you don't want to defer loading of images, then it's safe to disable this rule.
+There should be no cases where disabling this rule is needed. When it comes to rendering performance, it is generally better to specify `loading="eager"` as a default. You may want to do that for sections that are often placed in different parts of the page (top, middle, bottom), which makes it hard to reason about which value should be used.
 
 ## Version
 
diff --git a/lib/theme_check/checks/img_lazy_loading.rb b/lib/theme_check/checks/img_lazy_loading.rb
index b359814a..ad693cb5 100644
--- a/lib/theme_check/checks/img_lazy_loading.rb
+++ b/lib/theme_check/checks/img_lazy_loading.rb
@@ -10,11 +10,7 @@ class ImgLazyLoading < HtmlCheck
     def on_img(node)
       loading = node.attributes["loading"]&.downcase
       return if ACCEPTED_LOADING_VALUES.include?(loading)
-      if loading == "auto"
-        add_offense("Prefer loading=\"lazy\" to defer loading of images", node: node)
-      else
-        add_offense("Add a loading=\"lazy\" attribute to defer loading of images", node: node)
-      end
+      add_offense("Use loading=\"eager\" for images visible in the viewport on load and loading=\"lazy\" for others", node: node)
     end
   end
 end
diff --git a/test/checks/img_lazy_loading_test.rb b/test/checks/img_lazy_loading_test.rb
index e80c2ae7..0178d387 100644
--- a/test/checks/img_lazy_loading_test.rb
+++ b/test/checks/img_lazy_loading_test.rb
@@ -24,7 +24,7 @@ def test_reports_missing_loading_lazy_attribute
         END
       )
       assert_offenses(<<~END, offenses)
-        Add a loading="lazy" attribute to defer loading of images at templates/index.liquid:1
+        Use loading="eager" for images visible in the viewport on load and loading="lazy" for others at templates/index.liquid:1
       END
     end
 
@@ -36,7 +36,7 @@ def test_prefer_lazy_to_auto
         END
       )
       assert_offenses(<<~END, offenses)
-        Prefer loading="lazy" to defer loading of images at templates/index.liquid:1
+        Use loading="eager" for images visible in the viewport on load and loading="lazy" for others at templates/index.liquid:1
       END
     end
   end