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

handle alt text overflow on broken avatar images #81

Merged
merged 8 commits into from
Oct 20, 2023

Conversation

e-five256
Copy link
Member

@e-five256 e-five256 commented Oct 19, 2023

mainly in firefox, if mbin was unable to fetch the avatar image and it displays a broken image, a single letter column spans the entire length of the alt text.

left chromium, right firefox:

before_img_overflow

I found out this was because max-width/max-height/width/height aren't used on display: inline elements

following some answers here and here, changed it to inline-block and hide the alt text, which shouldn't effect screenreaders to my understanding of the answers

left chromium, right firefox:

EDIT: look below for how it looks now, decision was to not hide alt text completely to differentiate between broken images and no avatar

after_img_overflow

might need more testing on mobile / other browsers if this makes sense

@nobodyatroot nobodyatroot added enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end labels Oct 20, 2023
@nobodyatroot
Copy link
Member

nobodyatroot commented Oct 20, 2023

Nice! It also looks like this change improves the chromium experience as well, so a benefit for both!

I guess one thing to check is when there IS an avatar found, does this break anything?

I see you have this listed as draft, but I wouldn't hesitate to promote it to "ready for review" once you're comfortable with the changes. I don't think we need to exhaustively test each available browser, the big 2 are at least covered by your before and after.

@e-five256
Copy link
Member Author

e-five256 commented Oct 20, 2023

I guess I'm wondering if inline-block is the right one here, block also works. Difference seems well explained here. I think because it's already sort of by itself anyways, either works. I guess it can always be changed if something else is added to the element it's in.

I've been running with img { display: inline-block; overflow: hidden; } in tampermonkey for a while now and haven't noticed any issues, and that's not just comments, I did it to all images. (I read a stackoverflow post saying "just do this and save yourself all headaches" but for the life of me I can't find it. My own fault for always putting my dumb google searches in private browsing.) But felt it was safer to make this more specific, and of course I could've just easily missed something.

Edit: My goal with this change is to not effect accessibility or SEO. From my understanding the change doesn't, but I'm not a super expert at that stuff so if other people notice any reason it would, would appreciate the information

@e-five256 e-five256 marked this pull request as ready for review October 20, 2023 02:48
@e-five256
Copy link
Member Author

For reference this is what it looks like with just display inline block

image

and this with both display inline block and overflow hidden

image

so the indent and nowrap were to move the alt text out, but if that affects anything could also choose not to do that

@asdfzdfj
Copy link
Contributor

this one's more of a personal opinion and quite firefox centric but maybe the alt text doesn't need to be hidden?
mainly because avatar image for users without avatar image set is also an empty outline box, having the alt text still there can be a good indicator for instance operator that something's not right, and personally I feel like there's distinction between users without avatar and user with avatar but it's not load or shown when it should be.

also you might want to check for other places that uses avatar and apply similar fixes where appropriate, like in user hover info box or in the user's page for example.

image

image

move display and overflow settings to include all avatars
remove hiding of alt text
@e-five256
Copy link
Member Author

I definitely might need more eyes on this change than the last one as it's quite different from what I've been running, especially whether the user style sheet is included on all pages... I mean it looks like it just imports all of them but just to be sure.

So there exists a user-avatar class but it is only used on your avatar in the top right if you have one. That also has the overflow problem, so I figured potentially worth reusing that class on the avatars templated into everywhere, or at least I hope it's everywhere or else there's more one ofs somewhere

translate alt text on notifications panel
must have overlooked it while fixing translation
@e-five256
Copy link
Member Author

e-five256 commented Oct 20, 2023

I've added the class everywhere I see user.avatar.filePath used. I also noticed at the same time the notifications panel/threads with avatars enabled doesn't translate the alt text of avatars so changed that while I was there

I think the overflow problem might occur for magazine icons as well, but feel like that should be a separate PR

@asdfzdfj
Copy link
Contributor

although this is not strictly about user avatar image but it looks like user cover image in their profile page could also use this fix.
something like this should do?

.user-box .with-cover.with-avatar .cover {
  display: block;
  overflow: hidden;
}

the rest seems to be looking fine, from what I can see.

@e-five256
Copy link
Member Author

e-five256 commented Oct 20, 2023

Would it be acceptable to make the user cover and magazine icons their own PRs just to keep the change set small and easy to revert?

(I took a quick look and am a little confused with what classes are added in what situation for cover, so want to make sure I get it right, which might take more time. I also am wondering, I see mastodon just has a blanket .image img { display: block } and am like... clearly this is an extensive problem, maybe a more generic solution would be better)

@asdfzdfj
Copy link
Contributor

that should be fine

Copy link
Contributor

@asdfzdfj asdfzdfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, @nobodyatroot what do you think?

@nobodyatroot
Copy link
Member

Agreed, looks good to me as well!

@nobodyatroot nobodyatroot merged commit 480208e into MbinOrg:main Oct 20, 2023
3 checks passed
@e-five256 e-five256 deleted the e5/avatar-overflow branch October 20, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants