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

Use img to load avatars #311

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Use img to load avatars #311

wants to merge 5 commits into from

Conversation

da2x
Copy link
Contributor

@da2x da2x commented Dec 2, 2021

  • 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.

* 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).
@jacobwb
Copy link
Owner

jacobwb commented Dec 2, 2021

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
Copy link
Contributor Author

da2x commented Dec 2, 2021

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 c9e3d84.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants