Adjust Bootstrap icon usage #6501
Conversation
pablobm
left a comment
pablobm
left a comment
There was a problem hiding this comment.
Thank you for this, it's underappreciated, mind-numbing work! 💪 Left some comments regarding accessibility nuances, let's see what you think.
| <div class="card-text d-flex justify-content-between align-items-center mt-auto"> | ||
| <small class="text-body-secondary d-flex gap-1 align-items-center"> | ||
| <i class="bi bi-chat fs-5 my-n1" aria-hidden="true"></i> | ||
| <i class="bi bi-chat fs-5 my-n1" title="<%= t ".comments_label" %>" aria-hidden="true"></i> |
There was a problem hiding this comment.
Is this specific title needed? Also the span right after. They are followed by an "X comments" line, so I feel they might be redundant.
There was a problem hiding this comment.
It only occurred to me once I was testing something else on https://master.apis.dev.openstreetmap.org/ that the hover tooltips need a title attribute.
And since the Bootstrap icons use special characters in the pseudo-elements, I'm not happy with having this maybe appear on screen readers.
Hence twice.
There was a problem hiding this comment.
In this specific case, what I mean is that I think it's not necessary to have anything working as a title (neither icon title nor span) because they are followed by "X comments" which is self-explanatory.
I just tried with iOS VoiceOver and with the Read Aloud extension on Firefox. In this specific instance both say "comments, zero comments". I think it can do with just saying "zero comments", with no prologue.
Also in my test, it appears to be the span that is read by these tools, so perhaps it can be removed and the icon title left in place, although I don't know that the title serves any purpose. If it's any like the alt attribute in images, then it's best not to use it unless the image adds significant meaning, which is not the case here.
There was a problem hiding this comment.
What do you think of removing the span elements and adding role=img, aria-label=* AND title=* instead of the aria-hidden?
Then there's just one element, even though there's still two translated texts specified.
There was a problem hiding this comment.
In this specific case I would remove both the title and the span. I don't think they add anything. The i18n texts can be removed too then.
| </small> | ||
| <small class="text-body-secondary d-flex gap-1 align-items-center"> | ||
| <i class="bi bi-calendar fs-5 my-n1" aria-hidden="true"></i> | ||
| <i class="bi bi-calendar fs-5 my-n1" title="<%= t ".date_label" %>" aria-hidden="true"></i> |
There was a problem hiding this comment.
Similarly here: perhaps the title and span are not necessary as the content is self-explanatory.
There was a problem hiding this comment.
In the actual blog entry, the date is put in a sentence, so I think both can be dropped.
| <i class="bi bi-star-fill fs-5 role-<%= role %> align-middle" title="<%= t ".revoke.#{role}" %>" aria-hidden="true"></i> | ||
| <span class="visually-hidden"><%= t ".revoke.#{role}" %></span> |
There was a problem hiding this comment.
Related to the above: do we need both the title and the span? I suspect a single one will suffice. As for which one... any a11y experts in the room? 😅
There was a problem hiding this comment.
Following my experiment above, I think in these cases we should leave the span in place and remove the icon title.
There was a problem hiding this comment.
Since this logic branch is only visible for admins, I'd drop the span and leave the title for the tooltips.
There was a problem hiding this comment.
I tested iOS VoiceOver and Read Aloud (on Firefox) again. They have slightly different behaviours that are driving me a bit crazy, particularly how Read Aloud doesn't always reload the page properly 😠
The following seems to work with both:
<i class="bi bi-star-fill fs-5 role-<%= role %> align-middle" title="<%= t ".revoke.#{role}" %>" role="button" aria-pressed="true"><span class="visually-hidden"><%= t ".revoke.#{role}" %></span></i>
On iOS the text is read twice, annoyingly. Not sure if this is best. Read Aloud appears to read what's in the browser's Accessibility Tree so I would think that's relevant...? No idea 😭
| <i class="bi bi-star fs-5 role-<%= role %> align-middle" title="<%= t ".grant.#{role}" %>" aria-hidden="true"></i> | ||
| <span class="visually-hidden"><%= t ".grant.#{role}" %></span> |
| <i class="align-self-baseline bi bi-geo-alt-fill flex-shrink-0 fs-6" title="<%= t ".home_location" %>" aria-hidden="true"></i> | ||
| <span class="visually-hidden"><%= t ".home_location" %></span> |
There was a problem hiding this comment.
Same: probably one is enough.
| <i class="align-self-baseline bi bi-buildings-fill flex-shrink-0 fs-6" title="<%= t ".company" %>" aria-hidden="true"></i> | ||
| <span class="visually-hidden"><%= t ".company" %></span> |
|
@hlfan do you consider this ready, or are you planning further tweaks? |
|
Haven't gotten around to play with the ARIA details... |
3 participants
Addendum to #6487. Touches some adjustments from #6215.