Skip to content

Fix redundant logo alt #1459

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix redundant logo alt #1459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
826b38f
Fix redundant logo alt text
gabalafou Sep 18, 2023
343b415
copy edit
gabalafou Sep 18, 2023
6c43ea8
add back - Home
gabalafou Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/user_guide/branding.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ This title will appear next to the logo image if set.
}
}

.. note:: The ``html_title`` field will work as well if no logo images are specified.
.. note:: If you specify ``text`` then the theme sets the logo alt text to an empty string to avoid screen reader noise.

.. note:: If you provide neither a logo image nor logo text, the theme will use ``html_title``.


Add favicons
Expand Down
3 changes: 1 addition & 2 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/navbar-logo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
{% else %}
{% set href = theme_logo.get("link") %} {# external url #}
{% endif %}

{#- 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 "" %}
Copy link
Collaborator

@drammock drammock Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The logical possibilities here are:

  1. user doesn't specify anything. Value of project + version + "documentation" variables from conf.py are printed as text.
  2. user specifies html_title in conf.py. This is displayed as text instead of project.
  3. user specifies html_theme_options["logo"] = dict(text="foo"). Works basically the same as (2).
  4. user specifies html_logo in conf.py (or specifies html_theme_options["logo"] = dict(image_light="path/to/foo", image_dark="path/to/bar"). Only image, no text.
  5. 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?

Copy link
Collaborator Author

@gabalafou gabalafou Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

{% if is_logo %}
{# Theme switching is only available when JavaScript is enabled.
# Thus we should add the extra image using JavaScript, defaulting
Expand Down