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

Add wai-aria property to the artist accessibility annotations. #21328

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tonyfast
Copy link

@tonyfast tonyfast commented Oct 10, 2021

PR Summary

This pull request addresses #15971 and adds WAI-ARIA Conventions to the artist class. This change provides a place for figures to include accessibility annotations.

This pull request is inspired by ipython/ipython#12864 which brings alt text to images in the IPython display. With this addition to matplotlib we'll be able improve author's abilities to include alt text with their figures.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@tacaswell tacaswell added this to the v3.6.0 milestone Oct 10, 2021
@tacaswell
Copy link
Member

I guess a question here is do we want to a) only allow a subset we think we know we will want (at this point the aria-label) b) only validate the type of the ones we know about and be permissive on any other keys c) validate and only allow known aria keys?

c) is the most right, a) is probably too restrictive (we do not want to cut off someone who has a good idea) but exposes the least new API and b) is the least work, but leaves a bunch of foot cannons on the field.

@tonyfast
Copy link
Author

tonyfast commented Oct 10, 2021

so i think there is future potential in using these aria features on lower level plot objects when rendering as svg. there is more detail that can be added through aria at that level of detail, that is a distant future, but a cool goal.

since the artist._aria dict maps to html attributes the type is always dict[str, str]. i don't think there is any reason to restrict aria names because y'all haven't found specific things to use them for and, at the end of the day. we know folks are gonna do some bonkers things on the web. the extent of the checking we'd have to do is that the values are something that can be cast to an html string.

i'm feeling a nice to have would be an alias specifically for aria label, to maybe encourage folks to use this feature in their blogs/docs.

@tonyfast
Copy link
Author

hey y'all. i'm working though the the pr checklist. where are good places to add docs for this feature?

lib/matplotlib/artist.py Outdated Show resolved Hide resolved
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

  1. I'm not clear what people may want to do with this. Can you give one or two example usages?
  2. As of now this is only carried around with the artists but not used anywhere. What is the plan for that?
  3. I'm tempted to start very basic and make Artist.aria a simple attribute. That way we don't have to add all the function overhead. If people do strange things with that dict, it'll blow up some time later, but this is a very advanced-level feature that maybe doesn't need babysitting.

@tonyfast
Copy link
Author

  1. I'm not clear what people may want to do with this. Can you give one or two example usages?

the primary purpose of this is to encode web accessibility information (WAI-ARIA) into the artist. there is a new addition to ipython that includes alt text for images, this pull request is meant complement that so we have a place to pull the alt tags from. with the aria attribute we can include alt tech in a lot of documentation images.

  1. As of now this is only carried around with the artists but not used anywhere. What is the plan for that?

these features will primarily be used with ipython, sphinx gallery, sphinx gallery to provide at aria-label/alt text for images.

  1. I'm tempted to start very basic and make Artist.aria a simple attribute. That way we don't have to add all the function overhead. If people do strange things with that dict, it'll blow up some time later, but this is a very advanced-level feature that maybe doesn't need babysitting.

i'd be happy with this solution too if it is easiest. for onboarding new folks to project, i'd really like arist.set_aria_label available though. the aria label is the only part we REALLY need from this. this way we could run sprints that have folks adding plt.set_alt_test on documentation for other projects. the aria system is something that web developers do in fact do weird things with, not sure we can stop that.

@jklymak
Copy link
Member

jklymak commented Oct 12, 2021

I'll repeat the request for examples and a roadmap for how this will be used.

@tacaswell
Copy link
Member

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

@story645 story645 marked this pull request as ready for review Oct 27, 2021
@story645 story645 added New feature status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. labels Oct 27, 2021
@jklymak
Copy link
Member

jklymak commented Feb 1, 2022

At the very least this needs a rebase. Marking as stale for lack of response, and putting in draft. Feel free to move back to active...

@kanderso-nrel
Copy link

I can offer a use case. Some projects use notebooks containing Figures as a component of their sphinx docs, using e.g. nbsphinx to convert a notebook into something sphinx can eventually render as HTML. RdTools is one example: source notebook and built html. What I'd like to be able to do is specify figure-specific alt text in notebook code cells with something like fig.set_aria({'alt': 'Graph showing a time series of ...'}) (or maybe even plt.plot(..., alt='...')?) and have it eventually show up as <img alt="Graph showing a time series of ..." ...> in the sphinx output HTML.

I think one way to get this working would be to store aria metadata on the Artist as this PR does, modify the matplotlib_inline backend to grab that metadata from the Figure and pass it along to the IPython display() call so that it ends up in the notebook cell output metadata, and modify nbsphinx to pass it along to sphinx (see spatialaudio/nbsphinx#646).

Is this PR's approach considered workable? I'd be happy to submit the same patch in a fresh PR if so.

@oscargus
Copy link
Contributor

@kanderso-nrel Feel free to take the actual commit, 2d28564 and start a new PR, keeping @tonyfast as an author (so ideally cherry-pick that commit and start your new PR from there). I believe that there are more things to be added (I haven't followed the discussion, but at least some roadmap ahead seems to be requested, as currently this allows users to add ARIA information, but it is not used anywhere).

@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@tacaswell tacaswell marked this pull request as ready for review Oct 28, 2022
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 28, 2022
Copy link

@kanderso-nrel kanderso-nrel left a comment

Choose a reason for hiding this comment

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

Sorry I never followed through on my offer to reboot this PR, but I am glad to see someone else do it. Thanks @tacaswell!

doc/users/next_whats_new/aria.rst Outdated Show resolved Hide resolved
@tacaswell
Copy link
Member

There is on-going research on how to automatically generate accessible descriptions of plots and wai-aria roles are the industry standard for how to describe this sort of thing.

This is distinct from things like label (which are free-text single strings intended for use in the legend) and figure metadata (which are something between unconstrained and fully constrained userspace depending on the backends and exist in a different place in the file formats) so we need to carry another place to stash this sort of information.

I added the most minimal usage (aria-label in SVGs), but this is the ground work for the ability to add this information.

I did not touch Figure._repr_html_ because that lives over in IPython for historical reasons.


That's ok @kanderso-nrel ! Sorry if you feel like I snatched this from you, but I had spent more time talking about this today than it took to do so I just did it....

@story645
Copy link
Member

story645 commented Oct 28, 2022

I'll repeat the request for examples and a roadmap for how this will be used.

I really really want this in for the what's new cause then I can just copy that alt text for the social media posts instead of writing a new batch. I think another good high priority place to add the release notes is the plot gallery, especially since that populates the front page gallery. I can open an issue w/ all that but I don't want to hold up this PR b/c of it.

I also don't know how to do this but it would be supremely helpful if we could write a hook into sphinx/sphinx gallery that could parse this and inject it as the alt text on the .rst it renders?
Probably would be a step to amend plot_directive to support the alt option of image

Additionally, this directive supports all the options of the `image directive
<https://docutils.sourceforge.io/docs/ref/rst/directives.html#image>`_,
except for ``:target:`` (since plot will add its own target). These include
``:alt:``, ``:height:``, ``:width:``, ``:scale:``, ``:align:`` and ``:class:``.

@@ -317,7 +317,7 @@ def _check_is_iterable_of_str(infos, key):

class RendererSVG(RendererBase):
def __init__(self, width, height, svgwriter, basename=None, image_dpi=72,
*, metadata=None):
*, metadata=None, aria=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a safe extension of the API as we only use this additional optional key in this file where we know which class we are going to be calling. I do not know if other file formats also support aria roles (as they are more of an html thing and tend to live on the DOM outside of what ever we generate). If it is more general, we can push to other backends and up to base, otherwise I think we should keep this slight extension local to the svg backend.

Copy link
Member

Choose a reason for hiding this comment

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

We should consider ripping this out.

@tacaswell tacaswell force-pushed the add-aria-artist branch 2 times, most recently from 2b6f241 to 1ae620f Compare Oct 29, 2022
@story645 story645 removed the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label Nov 15, 2022
@story645 story645 requested a review from timhoffm Nov 15, 2022
All ``Arist`` now carry wai-aria data
-------------------------------------
Copy link
Member

@QuLogic QuLogic Nov 17, 2022

Choose a reason for hiding this comment

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

Suggested change
All ``Arist`` now carry wai-aria data
-------------------------------------
All ``Artist`` now carry wai-aria data
--------------------------------------

if not isinstance(aria, dict):
if aria is not None:
raise TypeError(
f'aria must be dict or None, not {type(aria)}')
Copy link
Member

Choose a reason for hiding this comment

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

This should use _api.check_isinstance.

self._aria = aria

def update_aria(self, **aria):
"""Update WAI-ARIA properties on the artist."""
Copy link
Member

Choose a reason for hiding this comment

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

This is the only one that says WAI-ARIA instead of ARIA.


def set_aria(self, aria):
"""
Set ARIA properties to the artist.
Copy link
Member

Choose a reason for hiding this comment

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

This docstring should define or reference what ARIA means.

for k, v in aria.items():
self._aria[k] = v
Copy link
Member

Choose a reason for hiding this comment

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

Should get a test?

@@ -0,0 +1,23 @@
All ``Arist`` now carry wai-aria data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
All ``Arist`` now carry wai-aria data
All ``Artist``s now carry wai-aria data

f'aria must be dict or None, not {type(aria)}')
self._aria = aria

def update_aria(self, **aria):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want update_aria or only get/set like most of our other methods.

I think a user could then do something like: a.set_aria(a.get_aria() | new_aria_dict)



Matplotlib will use the ``'aria-label'`` role when saving svg output if it is
provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to have an explicit example along with this that shows the structure of the dict() that you're expecting.

aria_dict = {"aria-label": "This is my cool figure"}

But, then we see this in roles: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/textbox_role

  role="textbox"
  contenteditable="true"
  aria-placeholder="5-digit zipcode"
  aria-labelledby="txtboxLabel"

where I'm a bit confused on whether you have a nested dict here or something else?

@anntzer
Copy link
Contributor

anntzer commented Nov 17, 2022

From a quick look it would seem like I'd rather have the aria label being output on every artist that has that property set; if someone really wants to write tree-walking code to generate a single aria string emitted at the toplevel (an option discussed during the call) they can also temporarily erase the artist-level arias before outputting the svg.

@tacaswell tacaswell removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Nov 17, 2022
@tacaswell tacaswell marked this pull request as draft Nov 17, 2022
@tacaswell
Copy link
Member

As discussed on the call this week, we need some more expert feedback on how to make sure this is useful. Things to consider:

  • just have researchers monkey patch?
  • at what level (if any) do we add thing from aria -> svg elements
  • should this be just on the outer most Figure or all Artists?
  • is an 'alt_text' attribute on Figure sufficient for our purposes?

This is not actually a blocker for #24309 as the .. plot directive already supports passing the alt text through from the rst and we need to work with sphinx-gallery to do the same in that context.

@tacaswell
Copy link
Member

Pushed to 3.8 as we are not going to get the right people together to sort this out in the next 2 weeks.

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

Successfully merging this pull request may close these issues.

None yet