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 pa11y testing and reporting #294

Merged
merged 31 commits into from
Aug 27, 2021
Merged

Add pa11y testing and reporting #294

merged 31 commits into from
Aug 27, 2021

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Dec 11, 2020

Hi folks!

This is an 80% solution to the up-front work of integration pa11y-ci as proposed in #292. I can split this up, if needed, but kinda felt like getting it working end-to-end (up to CI reporting) was important for demonstrating value. Note that it shouldn't fail right now, as I've whitelisted all the known error codes... that's where the 20% comes in, and each of them will likely require a design decision.

  • dependencies
    • adds sphinx-sitemap to docs/requirements.txt
    • adds pa11y-ci and pa11y-ci-reporter-html to package.json
    • add .yarnrc to do a local, offline package cache
      • the pa11y stack brings in a fair number of new dependencies, this helps reduce the impact
  • docs/conf.py
    • use sphinx-sitemap (naievly) to generate a (simple) sitemap.xml
      • this is skipped on RTD, as they generate a better one with timestamps, etc.
  • docs/a11y.py
    • runs pa11y-ci and the report generator
    • reads docs/a11y-roadmap.txt
      • the built-in stuff just hides the reports for codes... we want to see them, so we can fix them, but not fail builds while in-progress
  • workflows/tests.yml
    • adds python 3.9, and uses it for most tasks
    • adds pip/yarn caching
    • adds pip check so we don't get "fun" surprises, especially with the new pip solver
    • normalize the ports for the tornado server
    • call docs/a11y.py
    • upload report
  • docs/*.rst
    • add metadata which ends up in the meta tags on the built HTML to maintain lighthouse score
  • docs/accessibility.rst
    • adds some of this stuff (definitely the most WIP)
  • docs/contributing.rst
    • update with how to run locally, etc.

@bollwyvl
Copy link
Collaborator Author

I realized ignoring all of the a11y roadmap items doesn't make for good review, have commented out one to show some ❌ . Also, haven't run down the lighthouse issue, but it probably good to show that both audits run, even if one fails.

The audit step was already the long pole in CI at ~3min... we could split them up, but it would be even more duplication, etc.

@bollwyvl bollwyvl changed the title [wip] Add pa11y testing and reporting Add pa11y testing and reporting Dec 11, 2020
@bollwyvl
Copy link
Collaborator Author

Ok, I'm pretty happy with my self-review of the docs, and the output that shows up in both the log and the annotations. Probably no further work planned on this, gonna bop over to #293...

@bollwyvl
Copy link
Collaborator Author

Just updated this to master...

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks really nice and I love these docs on accessibility for the user :-) - my main concern is that the CI/CD builds now feel much more complex to me (e.g. I can read and understand the old ones with a casual glance as a non-expert in github actions, while these versions look much scarier to me (and thus will look like this for other non-expert maintainers as well, I suspect). Perhaps others can tell me if I am just being too conservative here?

But in general, I think this looks really nice, thanks for putting in this work to make sure the theme is closer to meeting the a11y goals that we should have for our communities.

I know @jorisvandenbossche may have some time to think about this theme in the next week, perhaps we can plan to discuss then?

- name: Cache pip
uses: actions/cache@v1
with:
path: ${{ steps.pip-cache.outputs.dir }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we now need this much more complex install config? It's fine if it's really needed, but just looking at it seems much less-maintainable to an average person to me 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, can drop the cache.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well if it's really useful then we should keep it, I just think we should be in the habit of justifying added complexity and maintenance burden, so I'm wondering what benefit it brings! (not out of skepticism, just because I do not know)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

simplified on 8e122bb etc

Copy link
Collaborator Author

@bollwyvl bollwyvl Dec 18, 2020

Choose a reason for hiding this comment

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

Using the CI cache makes lighter use of community resources, e.g. pypi. It's not making a detectable speed difference, however in this case.

To really drive down CLI complexity, I can highly recommend doit... it's nice for multi-language projects like this.

Auditing accessibility
======================

The accessibility checking tools are installed along with the rest of the `yarn`
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add in a link to pa11y and maybe a description of what we mean by "auditing accessibility"? Many devs may have no experience with a11y or UI audits, so this is all new to them. (maybe this is just fixed by linking to the user-facing docs on a11y in this PR?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update on 970ddd5

docs/contributing.rst Show resolved Hide resolved
----------------

If not using a more robust `internationalization approach <https://www.sphinx-doc.org/en/master/usage/advanced/intl.html>`__ ,
speciying at least the baseline natural language will assistive technology
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
speciying at least the baseline natural language will assistive technology
speciying at least the baseline natural language will help assistive technology

different content appears on a website.

If using a service like `ReadTheDocs <https://readthedocs.com>`__, these files
will be created for your, but for some of the other approaches below, it's handy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
will be created for your, but for some of the other approaches below, it's handy
will be created for you automatically, but for some of the other approaches below, it's handy

@bollwyvl
Copy link
Collaborator Author

I'll also go ahead and un-comment the demonstration rule in the roadmap, so that the audits start passing.

@bollwyvl
Copy link
Collaborator Author

Alright, back to 💚... hardly seems like i could let some of the scores slide on this particular PR!

@choldgraf
Copy link
Collaborator

Great - this looks really nice @bollwyvl, thanks :-) I'll await @jorisvandenbossche's (and maybe @hoetmaaiers if he's interested) thoughts since it's a big-ish change to the CI/CD and we should get some team buy-in

@isabela-pf
Copy link

Is there something still blocking this PR? I'd love to see this merged since so many projects use this theme to produce documentation and this could be a great way to start bringing accessibility into more communities.

@choldgraf
Copy link
Collaborator

Mostly I think it is waiting a 👍 from others in the team (maybe in particular @jorisvandenbossche ) since it is a big ish change to the codebase

@MarsBarLee
Copy link

I can't comment on the changes since I don't know much about CI, but NumPy would certainly appreciate this!

@bollwyvl
Copy link
Collaborator Author

All back up to running/reporting... I'd be happy to start fixing some of these things (like investigating the lighthouse regressions) once we start putting these things under test...

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me - I don't have a ton of experience with a11y but am fine merging this in and iterating rather than having a big ol' PR dangling here.

# specifying the natural language populates some key tags
language = "en"

# ReadTheDocs has its own way of generating sitemaps, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't we just use sphinx_sitemap anyway? Do they clash or something? I'd be fine just using the extension rather than special-casing RTD unless it's noticeably better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the RTD one is generally better. is aware of versions/languages, etc.

@@ -555,3 +555,8 @@ And here we demonstrate ``xarray`` to ensure that it shows up properly.
coords={"x": [10, 20]}, attrs={"foo": "bar"}
)
data


.. meta::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this like a section-specific metadata description or something? I haven't seen these in Sphinx world before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this populates the for-real meta tag in the head of a document: https://docutils.sourceforge.io/docs/ref/rst/directives.html#metadata

docs/user_guide/accessibility.rst Outdated Show resolved Hide resolved
The features and plans for addressing accessibility concerns on pydata-sphinx-theme

*************
Accessibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a quick thought but doesn't need to block this PR - I wonder if the ReadTheDocs team (👋 @ericholscher @astrojuanlu) would be interested in maintaining a guide like this for the broader docs community, so it's not just in this theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

welp, we've already seen a number of thumbs on this PR from various impactful downstreams, so there's interest. If anything, i'd rather see the baseline sphinx themes become more aware of accessible HTML5, but i haven't dipped my toe in that community, yet.

one thing we've been discussing is extracting the reporting/roadmapping capability into something standalone (e.g. local and in CI), potentially under the jupyter/acessibility banner... we've already replicated much of this over there, though it hasn't been exercised very often to keep a stack under continuous assessment. Probably out of scope for docs sites, but for complex things like web apps, there's a lot more that can be done with the keyword-based features, e.g. Click on #search-input, Type foo bar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bollwyvl you may be interested in @pradyunsg's effort to build a new "base sphinx template" that follows more modern best practices: https://github.com/pradyunsg/sphinx-basic-ng

We are even considering re-factoring this theme to build off of that one (potentially something that @damianavila will look into soon!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that would be great. Whatever it is, throwing as much possible spec validation at the lowest substantive level will yield the greatest impacts for authors and, ultimately, readers. If a subtheme comes along and hacks a span soup on top of a beautifully-organized semantic layout, then the project actually trying to publish some spec-compliant docs won't get much benefit.

As to adopting that theme: having this testing in place, at this level, starting to fix roadmap issues, and blocking merges when audits fail would make it more reasonable to do a large-scale dependency swap, and catch regressions.

Copy link
Collaborator

@choldgraf choldgraf Aug 26, 2021

Choose a reason for hiding this comment

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

yeah - I agree with all of those points.

One challenge that I foresee here is that this project already has a problem of stagnation. We have a lot of open PRs that are dangling and have gone stale (as witnessed by this PR!)...I do worry that yet another thing that raises the energy barrier to merging things will also make it less likely that anything changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe one example is the CI/CD in this PR. It seems that something called "lighthouse" is causing the CICD to fail. I'm not really sure what that means but I think it means we are below some kind of quality standard on accessibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, it looks like CI hasn't run successfully for the last two months, or so.

So somewhere in the last few months, the codebase (or one or more of its dependencies that actually make DOM) regressed on performance, privacy, best practices, and/or accessibility metrics that are reflected in the lighthouse report from some "high water" mark (presumably what it was doing when I originally made the PR). We can track back to #216, when lighthouse was added, and compare against the data from when the issue arose, and start clawing value for readers back. But if nobody's looking, it can't get any better, basically the whole premise of this PR.

I see robust tests and checks as a key enabler to improving the process of accepting high-quality contributioons: there's enough important stuff reliant on this code that "move fast and break things," probably isn't the right mantra anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with all of this - I'm not advocating for moving fast, just trying to be realistic given the very limited developer resources this theme has on an ongoing basis.

It sounds like from an a11y perspective, and given that the tests were broken before, that the best approach would be to merge this in and then see if there are more actionable things to do to get the tests passing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I could unbreak some more findings (that's why that meta stuff is there, for example), but again, master isn't green right now. Once we have forensic data to start looking at, we can move forward more confidently.

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
@choldgraf
Copy link
Collaborator

Note for others (particularly @jorisvandenbossche I suspect). I'll plan on merging this unless there are really significant changes in 48hrs. If you object please say so and I'm happy to hold off!

@choldgraf
Copy link
Collaborator

OK - as we have (mostly) passing tests (except for one we expect to be broken on master), and since a11y is important, and since this PR has been open for 8 months, and since nobody has objected in the last two days. I'm going to merge this :-)

thanks @bollwyvl for slogging through this one!

@choldgraf choldgraf merged commit 9232480 into pydata:master Aug 27, 2021
@isabela-pf
Copy link

Hooray! Thanks @bollwyvl for sticking with this and @choldgraf for getting it reviewed and merged! This is a big step to helping a lot of projects identify and fix inaccessibility in their documentation. 💕

@melissawm
Copy link
Contributor

Great news! Thank you @bollwyvl, @choldgraf and all involved for your work on this!

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

Successfully merging this pull request may close these issues.

5 participants