jacobwb / hashover-next

This branch will be HashOver 2.0
GNU Affero General Public License v3.0
420 stars 87 forks source link

Use img to load avatars #311

Open da2x opened 2 years ago

da2x commented 2 years ago
jacobwb commented 2 years ago

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.

da2x commented 2 years ago

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.