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

Occluded initial-viewport images should get fetchpriority=low #1309

Closed
westonruter opened this issue Jun 22, 2024 · 12 comments · Fixed by #1482
Closed

Occluded initial-viewport images should get fetchpriority=low #1309

westonruter opened this issue Jun 22, 2024 · 12 comments · Fixed by #1482
Assignees
Labels
[Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Feature A new feature within an existing module

Comments

@westonruter
Copy link
Member

Feature Description

When an image is in the initial viewport but is hidden, it should get fetchpriority=low. For example, this would apply to images which appear inside of a mega menu or images which are part of hidden carousel slides. Such images should not get loading=lazy since they should still be loaded and ready to go the moment the user interacts with the page to reveal it, but they should not get the same priority as images which are visible when first loading the page.

See https://web.dev/articles/fetch-priority#lower-carousel-priority

@westonruter westonruter added [Type] Feature A new feature within an existing module [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) labels Jun 22, 2024
@devansh016
Copy link
Contributor

@westonruter
I would like to contribute on this. Can you assign me this issue?

@westonruter
Copy link
Member Author

@devansh016 Actually I don't think it's going to be as simple as what you've proposed in #1320. Namely if I understand correctly, your PR would add fetchpriority=low to every image that is currently also getting loading=lazy. I don't think the URL metrics actually capture the necessary information yet to implement what we need here, and that is to detect images which are not visible while also being "above the fold". These would be non-initial images in a carousel which appears in the initial viewport, or images which appear inside of a mega menu which is not open until clicked. We'll need to think some more about how best to capture this information in detect.js.

@felixarntz
Copy link
Member

@westonruter So currently, Optimization Detective will also consider any of those "hidden carousel images" for example as above the fold right? So they won't have loading="lazy", is that correct?

On a related note, what is even the right way to mark such images? Should they have fetchpriority="low"? Or rather also get loading="lazy"? If I remember correctly, lazy-loading implementations of the browser now support horizontally "hidden" images too, so shouldn't we use the latter?

@westonruter
Copy link
Member Author

westonruter commented Aug 8, 2024

@felixarntz Good question. I just tested, and actually the hidden carousel images do get loading=lazy. I tried with the Image Slider Block as part of the Ultimate Blocks plugin and after populating the URL Metrics it correctly added fetchpriority=high and omitted loading=lazy from the first image, but the subsequent hidden carousel images get loading=lazy:

<div class="ub_image_slider swiper-container wp-block-ub-image-slider" id="ub_image_slider_aaa72125-78c1-40f4-88c9-f0f5068f5f8b" data-swiper-data='{"speed":300,"spaceBetween":20,"slidesPerView":1,"loop":true,"pagination":{"el": ".swiper-pagination" , "type": "bullets", "clickable":true} ,"navigation": {"nextEl": ".swiper-button-next", "prevEl": ".swiper-button-prev"}, "keyboard": { "enabled": true }, "effect": "slide","simulateTouch":false}'><div class="swiper-wrapper">
  <div class="swiper-wrapper">
    <figure class="swiper-slide">
      <img
        data-od-added-fetchpriority
        fetchpriority="high"
        decoding="async"
        src="http://localhost:8888/wp-content/uploads/2024/06/bison3-scaled.jpg"
        alt=""
      />
      <figcaption class="ub_image_slider_image_caption"></figcaption>
    </figure>
    <figure class="swiper-slide">
      <img
        data-od-added-loading
        loading="lazy"
        decoding="async"
        src="http://localhost:8888/wp-content/uploads/2024/06/bison2-scaled.jpg"
        alt=""
      />
      <figcaption class="ub_image_slider_image_caption"></figcaption>
    </figure>
    <figure class="swiper-slide">
      <img
        data-od-added-loading
        loading="lazy"
        decoding="async"
        src="http://localhost:8888/wp-content/uploads/2024/06/bison1-scaled.jpg"
        alt=""
      />
      <figcaption class="ub_image_slider_image_caption"></figcaption>
    </figure>
  </div>
...

This is not ideal because it means that as soon as the next carousel slide is displayed, the image may have to download from the beginning, as there are no heuristics for the browser to detect when the image is nearing the viewport.

According to the aforementioned Web.dev article, the best markup for this is:

<ul class="carousel">
  <img src="img/carousel-1.jpg" fetchpriority="high">
  <img src="img/carousel-2.jpg" fetchpriority="low">
  <img src="img/carousel-3.jpg" fetchpriority="low">
  <img src="img/carousel-4.jpg" fetchpriority="low">
</ul>

I believe this is especially good for when only one image can be displayed at a time. If, however, there is a horizontally-scrolled container in which the other images come into the viewport gradually, this would be a case where loading=lazy might make sense.

I haven't had a breakthrough yet on how to detect when an image should get fetchpriority=low instead of loading=lazy. In the case of a mega menu in which case there are images shown in response to opening submenus, I think this could be solved by checking if the img ancestor element is a header (or any element with role=banner) which is not contained inside of main, article, or section.

But how to handle the case of detecting occluded img tags in a carousel/slider? I was thinking perhaps it could be that when a hidden element has a sibling which is visible that in this case we add fetchpriority=low instead of loading=lazy. But this doesn't work in the case of a carousel of Image blocks which have a figure wrapper tag, since the img won't be a sibling to any other img tags. The only other idea that comes to mind is that while we are walking over the HTML structure, we could look at the id and class attributes for elements to see if they have known values that indicate a slider/carousel (e.g. swiper). If we store that, we could then know whether a given IMG tag is inside of a carousel, and if there is another IMG inside that tree which is visible, then all of the other IMG tags in that tree should get fetchpriority=low instead of loading=lazy. This would involve researching the most popular slider libraries to be able to build up a list possibilities for what carousel markup looks like.

@westonruter
Copy link
Member Author

westonruter commented Aug 8, 2024

Oh, I just had another idea for a carousel which is much less hacky. I think we actually may be collecting the information we need in the end, and it's not in the intersectionRect but rather in the clientBoundingRect. In looking at the Swiper carousel, slides which aren't displayed aren't actually hidden-hidden (with display:none). They are just hidden in the overflow of the parent element:

Screen.recording.2024-08-07.18.04.51.webm

Here are the three elements stored in URL Metrics for the Swiper carouse:

[
  {
    "isLCP": true,
    "isLCPCandidate": true,
    "xpath": "/*[1][self::HTML]/*[2][self::BODY]/*[2][self::DIV]/*[2][self::MAIN]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[1][self::FIGURE]/*[1][self::IMG]",
    "intersectionRatio": 1,
    "intersectionRect": {
      "x": 1275.4166259765625,
      "y": 343.1510314941406,
      "width": 650,
      "height": 250,
      "top": 343.1510314941406,
      "right": 1925.4166259765625,
      "bottom": 593.1510314941406,
      "left": 1275.4166259765625
    },
    "boundingClientRect": {
      "x": 1275.4166259765625,
      "y": 343.1510314941406,
      "width": 650,
      "height": 250,
      "top": 343.1510314941406,
      "right": 1925.4166259765625,
      "bottom": 593.1510314941406,
      "left": 1275.4166259765625
    }
  },
  {
    "isLCP": false,
    "isLCPCandidate": false,
    "xpath": "/*[1][self::HTML]/*[2][self::BODY]/*[2][self::DIV]/*[2][self::MAIN]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[2][self::FIGURE]/*[1][self::IMG]",
    "intersectionRatio": 0,
    "intersectionRect": {
      "x": 0,
      "y": 0,
      "width": 0,
      "height": 0,
      "top": 0,
      "right": 0,
      "bottom": 0,
      "left": 0
    },
    "boundingClientRect": {
      "x": 1945.4166259765625,
      "y": 343.1510314941406,
      "width": 650,
      "height": 250,
      "top": 343.1510314941406,
      "right": 2595.4166259765625,
      "bottom": 593.1510314941406,
      "left": 1945.4166259765625
    }
  },
  {
    "isLCP": false,
    "isLCPCandidate": false,
    "xpath": "/*[1][self::HTML]/*[2][self::BODY]/*[2][self::DIV]/*[2][self::MAIN]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[3][self::FIGURE]/*[1][self::IMG]",
    "intersectionRatio": 0,
    "intersectionRect": {
      "x": 0,
      "y": 0,
      "width": 0,
      "height": 0,
      "top": 0,
      "right": 0,
      "bottom": 0,
      "left": 0
    },
    "boundingClientRect": {
      "x": 2615.41650390625,
      "y": 343.1510314941406,
      "width": 650,
      "height": 250,
      "top": 343.1510314941406,
      "right": 3265.41650390625,
      "bottom": 593.1510314941406,
      "left": 2615.41650390625
    }
  }
]

Note how the first element has isLCP: true and intersectionRatio: 1, whereas the subsequent images all have intersectionRatio: 0. But we can see that they all have the same intersectionRect.top and intersectionRect.y values with differing intersectionRect.x values, indicating that the elements are all on a horizontal line. This implies that these elements with intersectionRatio: 0 should get fetchpriority=low instead of loading=lazy!

@felixarntz
Copy link
Member

@westonruter I like your last idea: For images that aren't visible, but they're on the same horizontal line as a visible image, use fetchpriority=low instead of loading=lazy. (You mentioned a few times importance=low, I assume you mean fetchpriority=low?)

A related but separate question: What happens to images that are completely hidden (either by them or a containing element having display:none)? If their inner-most visible container is in the viewport, should such images have fetchpriority=low too? Can we detect that?

@westonruter
Copy link
Member Author

westonruter commented Aug 8, 2024

For images that aren't visible, but they're on the same horizontal line as a visible image, use fetchpriority=low instead of loading=lazy. (You mentioned a few times importance=low, I assume you mean fetchpriority=low?)

Oh yes, oops. I've corrected my comments to say fetchpriority instead of importance.

I also just realized that it's probably even simpler than I suggested. Instead of detecting whether an intersectionRatio: 0 image is on the same horizontal line as another image with a non-zero intersectionRatio, we can simply check if boundingClientRect.top is less than the height of the entire viewport. These images then can automatically get fetchpriority=low.

This also could be relevant for images that are placed outside the viewport in a negative direction to the top, left, or right. I recall this is a more accessible way to style dropdown menus since the submenu items remain focusable with the keyboard even when the submenu is not displayed. But maybe this is old news as I see with the current dropdown menus in Gutenberg the submenus are styled with opacity:0 and visibility:hidden. Surprisingly, I'm now learning that IntersectionObserver is reporting that such hidden elements have a non-zero intersectionRatio! It seems that the only way to detect if such an element is actually visible is to get the computed style. (This being the case, if hidden images in the header generally have a non-zero intersectionRatio anyway, then we wouldn't need to do what I proposed above to force hidden images located in the header to have fetchpriority=low.)

To summarize:

  • Situations to add fetchpriority=low:
    • If an element has a non-zero intersectionRatio and the element has a computed style of visibility:hidden or opacity:0, then it should get fetchpriority=low.
    • If an element has an intersectionRatio of zero and yet its boundingClientRect.top is less than the viewport height, then it should also get fetchpriority=low.
    • If an element has an intersectionRatio of zero and its boundingClientRect indicates that it is in the negative area of the viewport to the top, left or right, then it should get fetchpriority=low since it may be part of an off-screen submenu that gets displayed.
  • Alternatively, if an element has an intersectionRatio of zero and its boundingClientRect.top is greater than the viewport height (or if all of the values in boundingClientRect are zero, indicating a display:none element), it should get loading=lazy. But read on...

A related but separate question: What happens to images that are completely hidden (either by them or a containing element having display:none)? If their inner-most visible container is in the viewport, should such images have fetchpriority=low too? Can we detect that?

I was thinking about this when trying to think of an approach to images appearing in a mega menu above, but now that I see that core's built-in submenus get visibility:hidden as well as the example mega menu block from the WordPress Developer Blog, this doesn't seem necessary.

But let's say that's not always the case, that sometimes images in a mega menu get display:none when they are not shown, or for what is a much easier situation to test for, a Details block in the initial viewport which is collapsed and which contains an image. Currently I see that WordPress core is incorrectly adding fetchpriority=high to this image:

<details
  class="wp-block-details is-layout-flow wp-block-details-is-layout-flow"
  open=""
>
  <summary>Check out this image</summary>
  <figure class="wp-block-image size-large">
    <img
      fetchpriority="high"
      decoding="async"
      width="1024"
      height="668"
      src="http://localhost:10023/wp-content/uploads/2024/08/bison1-1024x668.jpg"
      alt=""
      class="wp-image-8"
      srcset="
        http://localhost:10023/wp-content/uploads/2024/08/bison1-1024x668.jpg  1024w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-300x196.jpg    300w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-768x501.jpg    768w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-1536x1002.jpg 1536w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-2048x1336.jpg 2048w
      "
      sizes="(max-width: 1024px) 100vw, 1024px"
    />
  </figure>
</details>

Image Prioritizer fixes this by removing fetchpriority=high and adding loading=lazy:

<details
  class="wp-block-details is-layout-flow wp-block-details-is-layout-flow"
>
  <summary>Check out this image</summary>
  <figure class="wp-block-image size-large">
    <img
      data-od-added-loading
      data-od-removed-fetchpriority="high"
      loading="lazy"
      decoding="async"
      width="1024"
      height="668"
      src="http://localhost:10023/wp-content/uploads/2024/08/bison1-1024x668.jpg"
      alt=""
      class="wp-image-8"
      srcset="
        http://localhost:10023/wp-content/uploads/2024/08/bison1-1024x668.jpg  1024w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-300x196.jpg    300w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-768x501.jpg    768w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-1536x1002.jpg 1536w,
        http://localhost:10023/wp-content/uploads/2024/08/bison1-2048x1336.jpg 2048w
      "
      sizes="(max-width: 1024px) 100vw, 1024px"
    />
  </figure>
</details>

I'd say this is definitely better than the status quo, but would it be even better if the image inside the details element were given fetchpriority=low instead of loading=lazy? For the Details element specifically, we could add a tag visitor for it so that we can track its intersectionRect in the URL Metrics. Then when we encounter an image inside of a Details block, we can add fetchpriority=low to it instead of loading=lazy if the contained Details block is in the viewport. But there are countless other scenarios in which an element may be hidden with display:none than just with the Details block, so I don't know how much value this special case has.

Even so, let's say we do add this special case for the Details block. Does adding fetchpriority=low instead of loading=lazy really help the UX overall? If a Details element is outside the viewport, expanding it would reveal an image with loading=lazy, causing the browser to start downloading it at the moment of interaction. It's probably not going to be super common for a Details element to be in the initial viewport anyway, but even so, I'm not sure why expanding a Details element in the initial viewport should have a better UX than expanding a Details element outside the initial viewport.

@felixarntz
Copy link
Member

Sounds great.

If an element has an intersectionRatio of zero and yet its boundingClientRect.top is greater than the viewport height, then it should also get fetchpriority=low.

Shouldn't this say "less than the viewport height" instead, like you say earlier? Just want to make sure the summary is accurate.

Regarding the display:none situation, I'd say let's break that out into a separate issue to explore further. It seems with your above summary we're getting very close to a defined solution for otherwise hidden images that are in the viewport.

@westonruter
Copy link
Member Author

Shouldn't this say "less than the viewport height" instead, like you say earlier? Just want to make sure the summary is accurate.

Oh yes, you're right. Updating my comment to reflect this. Good catch.

Regarding the display:none situation, I'd say let's break that out into a separate issue to explore further. It seems with your above summary we're getting very close to a defined solution for otherwise hidden images that are in the viewport

Sounds good!

@westonruter
Copy link
Member Author

Aside: I was confused why WordPress stopped adding fetchpriority=high to the initial large Image block in my post. Then I realized it's because I added a large Image block to the mega menu I was testing with, and WordPress is adding fetchpriority=high to it even though it is not visible (since it is only displayed when opening the mega menu). Nevertheless, Image Prioritizer successfully removed fetchpriority=high from this image and added it to the actual LCP image later in the page.

@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Aug 13, 2024
@westonruter westonruter moved this from Not Started/Backlog 📆 to To Do 🔧 in WP Performance 2024 Aug 13, 2024
@westonruter westonruter added this to the image-prioritizer n.e.x.t milestone Aug 15, 2024
@westonruter westonruter changed the title Add fetchpriority=low to occluded initial-viewport images Occluded initial-viewport images should get fetchpriority=low Aug 19, 2024
@westonruter
Copy link
Member Author

Now implemented in #1482:

  • If an element has an intersectionRatio of zero and yet its boundingClientRect.top is less than the viewport height, then it should also get fetchpriority=low.
  • Alternatively, if an element has an intersectionRatio of zero and its boundingClientRect.top is greater than the viewport height (or if all of the values in boundingClientRect are zero, indicating a display:none element), it should get loading=lazy.

@westonruter
Copy link
Member Author

I've split this off into #1587:

  • If an element has a non-zero intersectionRatio and the element has a computed style of visibility:hidden or opacity:0, then it should get fetchpriority=low.

@westonruter westonruter moved this from In Progress 🚧 to Code Review 👀 in WP Performance 2024 Oct 15, 2024
@github-project-automation github-project-automation bot moved this from Code Review 👀 to Done 😃 in WP Performance 2024 Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Feature A new feature within an existing module
Projects
Status: Done 😃
3 participants