Fix redundant logo alt #1459
Fix redundant logo alt #1459
Conversation
| {#- Logo HTML and image #} | ||
| <a class="navbar-brand logo" href="{{ href }}"> | ||
| {# get all the brand information from html_theme_option #} | ||
| {% set is_logo = "light" in theme_logo["image_relative"] %} | ||
| {% set alt = theme_logo.get("alt_text", "Logo image") %} | ||
| {% set alt = theme_logo.get("alt_text", "Logo image") if not theme_logo.get("text") else "" %} |
There was a problem hiding this comment.
The logical possibilities here are:
- user doesn't specify anything. Value of
project+version+ "documentation" variables fromconf.pyare printed as text. - user specifies
html_titleinconf.py. This is displayed as text instead ofproject. - user specifies
html_theme_options["logo"] = dict(text="foo"). Works basically the same as (2). - user specifies
html_logoinconf.py(or specifieshtml_theme_options["logo"] = dict(image_light="path/to/foo", image_dark="path/to/bar"). Only image, no text. - user specifies both logo (as in (4) above) and additional text (via
html_theme_options["logo"] = dict(text="foo")).
I'm inferring that options 1,2,3 clearly don't need alt text (no images), option 4 does, and option 5 does not? Is that right @gabalafou? To me it would make sense to include alt text in option 5 as well, since we don't necessarily know what additional text the user is providing. For example if they show a logo and the additional text says "the best package for MRI analysis", I think it would still be good for the logo alt text to say "name_of_package - Home" or something. WDYT?
There was a problem hiding this comment.
Yeah, it didn't feel quite right to me to give the user the ability to specify both fields but only use one. In other words, it seems wrong to me make the combo of (alt_text provided, text provided) equivalent to (alt_text not provided, text provided). I could almost hear the frustrated user in my head: no but really, trust me, I know what I'm doing, I want to use both alt text and regular text (as in your example of "the best package for MRI analysis").
My approach here is probably too heavy handed.
A lighter approach would be to check if the text and alt text are partial matches of each other and if so, warn the user that what they're doing is probably bad for accessibility, but not go so far as to prevent them from doing it.
I'm just not sure how to get the warning across in a way that's efficacious. I feel like a lot of people using a third-party dependency ignore the warnings from the dependency unless something looks broken to them in the end result. And that's the problem here: to an abled user it won't look broken, but to a subset of disabled readers, it will sound broken, and not making that bug more apparent to abled users feels to me like a small injustice.
I thought about adding a flag to the logo dictionary that a user could set to mean: no really, I know what I'm doing, use both fields. If they don't include that flag, then the logo text nullifies alt_text (just like in this PR). But adding such a flag also feels like config creep to me.
I feel like there must be a more elegant solution, but it eludes me at the moment.
On a related note, this logic is complicated enough that I think I need to add a test to this PR eventually, but without some help, given my lack of Python experience, I think it's going to take a lot of time for me. Would someone be willing to pair program with me for an hour to help me add tests for the logo component?
There was a problem hiding this comment.
I think the approach of checking for alt-text and extra-text being matched, and warning if they are, is a good one.
I'm just not sure how to get the warning across in a way that's efficacious. I feel like a lot of people using a third-party dependency ignore the warnings from the dependency unless something looks broken to them in the end result.
What the user does with our warnings is outside of our control. IMO it is our responsibility to raise awareness that duplicating the extra text in the alt-text is anti-adaptive, but we're not responsible for whether users heed or ignore our advice. I'd vote to raise a warning and also add advice to the docs.
There was a problem hiding this comment.
On a related note, this logic is complicated enough that I think I need to add a test to this PR eventually, but without some help, given my lack of Python experience, I think it's going to take a lot of time for me. Would someone be willing to pair program with me for an hour to help me add tests for the logo component?
Normally I'd volunteer but I'm super swamped --- teaching workshop thursday, running week-long training next week, then traveling+working the week after. Any other volunteers?
There was a problem hiding this comment.
I think all of that is fair. This also isn't a critical accessibility issue, so there's no real urgency around it.
In the meantime, I will submit two separate PRs to speed along more simple fixes, one for the test suite, one for the docs.
|
I'm going to close this PR since it is essentially superseded by #1472 |
Completes one of the tasks in #1428.
Alternative text of images should not be repeated as text.
The theme allows the user to specify both logo text and alt text. This PR changes the theme so that if the user provides text, it takes precedence over alt text.