Use nested react-animated-dataset

[dkaoster]

Implements the updated nesting feature in freedomofpress/react-animated-dataset#1

Everything appears to work the same, maybe would be a good time to see if this is a better approach for accessibility @SaptakS.

The main goal of doing this is outlined in this issue, as this should allow for better composability and accessibility when using react-animated-dataset, enabling us to do something like:

<AnimatedDataset
  dataset={['Hello World', 'Goodbye World']}
  tag="a"
  attrs={{ href: "#test" }}
  keyFn={t => t}
  disableAnimation
>
  <AnimatedDataset
    tag="text"
    attrs={{ text: t => t }}
    keyFn={t => t}
    disableAnimation
  />
</AnimatedDataset>

[sssoleileraaa]

To test this, should one just make sure there are no regressions? Or is the plan to do most of the testing once it's merged and before deployment, as mentioned here: freedomofpress/react-animated-dataset#1 (comment)?

I have cycles to help out with testing if needed.

[harrislapiroff]

@sssoleileraaa Hm, unfortunately, I think this one might be tricky for you to test because it really depends on an assessment of whether we like the new code APIs @dkaoster has introduced into react-animated-dataset and whether they meet our needs for making more accessible charts. Intimate knowledge of the code base is helpful here. We can keep an eye out for PRs that might be a better introduction if you're excited to get your eyes on the code a bit!

[sssoleileraaa]

Ah, yes, that makes sense @harrislapiroff. Thanks for keeping an eye out - in general, I'm happy to help test new stuff before deployments. :)

[SaptakS]

@dkaoster though I haven't started looking at it yet, it would be great if you could rebase and resolve the merge conflicts.

[SaptakS]

Firstly, I think the nested react-animated-dataset works great. However, some of the ways it has been used here is what I am kind of reviewing.

It does work using keyboard navigation, but the accessibility tree is not very promising in certain cases. I do want to do some more testing using screen reader itself, hence not requesting any immediate changes, but few of the things I noted:

• The <a> tag in svg doesn't have a proper role. So in the tree, it shows role="nothing", accessible name=null. I am not sure how things will be announced in screenreader, I will test that as well.
  • Solution to this might be to add a role and accessible name attributes
• In some cases, like in the treemap, the <a> tag actually doesn't wrap the text. The text hence appears separately in the accessibility tree. I think we should try to wrap the text with the tag if possible. That might make the screenreader sound okay even without adding an accessible name (and also makes more sense structurally.

Like I said before, I do think that the nested react-animated-dataset is definitely helpful, though we might need to figure out the exact structuring

[SaptakS]

As I had expected, while tabbing through the different elements in a chart, there is no screenreader feedback at all. It just tells that there is a focus element but no details on it.

What the final HTML should ideally look like is this:

Notice the role attribute and aria-labelledBy attributes. I tried adding them in the react-animated-dataset tag and it didn't seem to work, so I am not sure if more work might be needed to be done in the other PR to make it possible to have these attributes. But I think we definitely want to have role="link" whenever we have a link, and have an aria-labelledBy attribute pointing to the element which has the text in it.

Ideally, for better structure, it would be nice to have the elements with the text appear inside the anchor tag if possible.

[dkaoster]

@SaptakS Just added the role and aria-label in, it is totally configurable and easy to do. I think the point about restructuring so that the text is inside the anchor links might be a little bit more challenging but I'll look into that!

[SaptakS]

This looks really good to me. I am going to approve this. I think some snapshots are failing the tests that need to be fixed?
Also, @harrislapiroff what is the plan with react-animated-dataset changes? Should I merge this as is and then the plan is to open a PR to the upstream repo?

[harrislapiroff]

@SaptakS Yes, let's merge as is and then live with the changes for a couple weeks and then, if we're happy with them, we'll put in a PR upstream. (So leave it in blocked for now.)

[SaptakS]

@harrislapiroff okay. Once @dkaoster updates the tests to fix CI, I will merge it.

[dkaoster]

whoops! that should do it