hashover-next icon indicating copy to clipboard operation
hashover-next copied to clipboard

Use img to load avatars

Open da2x opened this issue 3 years ago • 2 comments

  • Stop sending referrer request data to Gravatar (reduces data sharing with Gravatar, improves user privacy).
  • Enable lazy loading avatar images (reduces image download request priority, defers downloading off-screen images).
  • Remove the image from screen narrations (maintains current behavior).
  • The change allows for sites to use stricter content security policies.

da2x avatar Dec 02 '21 02:12 da2x

I appreciate this commit, it would solve two problems, security and accessibility. Unfortunately, I don't have time to look into this fully right now. I will soon, though. The changes look good, the only change I would make is to add a value for the alt attribute, not having that is a problem with the current implementation. You may be able to set it to something like {name} - Avatar in both places, but I have not tested that.

HashOver used to use images for the avatars, and I would prefer to use images myself as well, but they were removed because they caused considerable performance degradation.

You can read the changelog about this from mid 2015: changelog:L1264-L1269.

At that time lazy loading was probably not well supported, I would be interested to know if it would solve the performance issues. If you could test your changes with 1,000 comments vs. using background images, with and without an ad-blocker, and verify that lazy loading the avatars doesn't impact performance, I will gladly accept your commit.

jacobwb avatar Dec 02 '21 07:12 jacobwb

The changes look good, the only change I would make is to add a value for the alt attribute […]

alt attributes are hard. Reading “Image, avatar” is meaningless. Reading “Image, avatar of Example Person. Example Person.” would just duplicate the name (which is the very next thing being read.) I can’t convey what information is in the image, so it’s difficult to know what to say. Is it a fun image? Is it sexy? Is it blocky pixel art? Is it someone’s wearing a horse head mask? Ultimately, the image is small. It’s not the main information. Hiding it (empty alt attribute) seems like the best option.

HashOver used to use images for the avatars, and I would prefer to use images myself as well, but they were removed because they caused considerable performance degradation.

This is due to the layout shifts caused by missing height and width attributes. It would need to load the image to determine its sizes. As long as it’s set in CSS (or directly on the element) it shouldn’t cause layout shifts. The default theme sets the attributes in CSS, so all is fine. I added it to the element too via c9e3d84e397436b56a154af00db42083ac2ad9cc.

If a content blocker blocks Gravatar then it’s one on the network request level, so it doesn’t matter if it’s an image or a background element. Some older adblockers added style=display:none which would cause ayout shift, but this must be 15 years ago.

To speed up performance in complex documents (lots of comments), then this should help tremendously (explainer video — TL;DR: the browser can cheat and assume the height of elements and doesn’t need to render them if they’re far off screen until you scroll down to them):

.hashover-comment {
  contain: content;
  contain-intrinsic-size: 0 170px;
  content-visibility: auto
}

I can submit this as another patch as to not piggyback too many things in one pull request. Pull request #327.

da2x avatar Dec 02 '21 09:12 da2x