plotly.js icon indicating copy to clipboard operation
plotly.js copied to clipboard

add Chart2Music keyboard and sound accessibility features

Open aliwelchoo opened this issue 1 year ago • 39 comments

Use data and layout to call the chart2music API inside makePlot.

This pull request is for scatter plots only

aliwelchoo avatar Jul 21 '23 17:07 aliwelchoo

  • Closed captions element is required to be visible on the screen to function so need to find a way to integrate it more cleanly
  • Need to rebase

aliwelchoo avatar Jul 22 '23 11:07 aliwelchoo

Excited for this! FYI @julianna-langston @spmealin - I think we can sort out most of what we need but your feedback is of course welcome!

@archmoj thoughts about packaging? Is a 50kb increase in minified bundle acceptable or should we keep chart2music external like MathJax? If we did make it external we'd probably want it included by default in plotly.py output, and we could have Dash detect and autoload it, but plain plotly.js users would need to explicitly load it

alexcjohnson avatar Jul 24 '23 21:07 alexcjohnson

Where we're stuck

Warning: These problems will increase in frequency because ESM is highly recommended

You cannot require() (commonjs) a .mjs (ESM) because the require() will block and wait for the .mjs to load and the .mjs may await expecting that execution would continue without it.. but a) it won't and b) sync code can't await.

Despite that your web-build configuration would nullify that issue, (theory:) the webpack resolver sees you're require()ing a .mjs and simply fails silently (anticipating above) and decides not to include it in the dependency tree, which is why none of the loaders pick it up to do transpilations. It's just not being included in your input or output.

Furthermore, the babel plugin to do a ESM-->commonjs transpilation will also fail silently if any of the imports of the input code are ESM only-- but luckily, chart2music has no dependencies at all! But that's not the only obstacle to using babel to transpile it: (theory:) the package.json insists chart2music is an ESM module and the babel plugin doesn't modify that, so webpack still chokes the moment it gets to package.json of that module.

Solutions

There is a commonjs dynamic import() compatible with ESM but the linter is blocking it. Using it works! but would cause, for the first time, your bundler to output two separate chunks (click and zoom pls):

image (Master branch at bottom)

Since you guys distribute whole files, until you're ready (lets get there soon) for a real refactor and optimization for plotly delivery, you might want to tell webpack no chunking so it doesn't create separate files (as seen in my branch).

Or, if you were going to include this as a button on the mode bar, you would use dynamic import() to fetch the dependency and redo _doPlot() on-click.

Running the babel-cli and changing the package.json to not insist on being ESM does work, so it could be included in the module. Could make a pull on the main repo. (done)

Original Dep Tree of Master Branch: image

Back to all this on friday-ish.

ayjayt avatar Jan 17 '24 01:01 ayjayt

chart2music merged in my request to support commonjs, so I submitted a pull request to @aliwelchoo that should make this go green.

edit: welp, so much for green lol. needs some undefined checks..... later....... or we can just use es6 like before.

ayjayt avatar Jan 17 '24 22:01 ayjayt

Alright, I fixed the obvious stuff (including plot-schema) from those errors, but non-obvious stuff is:

  1. What should Plotly.purge() do with the closed-caption div. Should we change the test to expect an empty closed-caption div?
1) should return the graph div in its original state
     Test plot api Plotly.purge
     Error: Expected '<div id="cc" class="closed_captions" aria-live="assertive" role="status" aria-atomic="true" aria-relevant="additions text"></div>' to equal ''.
    at <Jasmine>
    at eval (webpack://plotly.js/./test/jasmine/tests/plot_api_test.js?:2340:30)
  1. No clue what baseline tests are:
domain_refs
Transform failed with error code 525: Cannot read property 'x' of undefined
retry 1 / 2
Transform failed with error code 525: Cannot read property 'x' of undefined
retry 2 / 2
Transform failed with error code 525: Cannot read property 'x' of undefined

https://app.circleci.com/pipelines/github/plotly/plotly.js/9547/workflows/fbf28531-254a-4bb8-9722-924609ce9de4

ayjayt avatar Jan 17 '24 22:01 ayjayt

I've just made a pull request on this repo:branch w/ about a million commits that makes us go green, but there are issues, let's see:

1) The closed caption div. By default it doesn't generate automatically.

  • Tests now create a closed caption div in their createGraphDiv() in assets/
  • Dev dashboard now comes with a nice one stock.

If it's going to generate automatically, and INSIDE the graph div, someone needs to quiet down all the tests and functions that are surprised to see a new neighbor. Or let's just keep it elsewhere? But how.

@julianna-langston Does it (closed-caption div) even need to be visible to work? :-)

2) Where to rerender C2M?

Plotly has several possible renders eg. transitionFromReact(), newPlot(). However, accessibility.js does everything in _doPlot(). I think accessibility.js should split into two:

  1. Set up basic config
  2. (Re)Count data

Or what? @archmoj @alexcjohnson I have no idea who this question is for.

3) Large Accessibility Concerns

I think probably that when someone key-tabs into plotly, it should focus first to a button on the mode bar (accompanied by a temporary closed caption?) that can activate accessibility mode. I don't think closed captions should just appear anytime someone clicks the graph with the mouse. I'm assuming users will generally being key-tabbing into it and so that makes a good entry point for enabling accessibility mode. Of course, other uses can click the mode button and turn it on and off. Opinions? @julianna-langston... I'm tagging you because I have no experience with screen readers and I'm not sure what the user experience should be like.

4) Technical Side Note:

Probably need to fully ensure that we copy all _context items from gd to c2m() and not pass any references, and do in "1. Set up basic config". That said, c2m probably should cause the gd to redraw completely (ie no transition) if the c2m config changes, the same as gd does.

ayjayt avatar Jan 26 '24 02:01 ayjayt

Does it (closed-caption div) even need to be visible to work? :-)

You can position the closed caption off screen, or you can use visibility:hidden, but you can't use display: none.

There was a few months there where either Firefox or NVDA wasn't recognizing closed captions that were positioned off screen, but I haven't seen that problem in over a year. I would feel comfortable with simply moving the box off screen with something like this:

#cc {
  position: absolute;
  top: -1000px;
  left: -1000px;
}

I don't think closed captions should just appear anytime someone clicks the graph with the mouse

If you position the closed caption off screen, you can support screen reader users, without sighted users ever knowing.

I'm assuming users will generally being key-tabbing into it and so that makes a good entry point for enabling accessibility mode.

This is a fair assumption for sighted, keyboard-only users.

However, screen reader users frequently navigate a page with shortcut keys that come with the screen reader. Many of these shortcuts will allow them to jump directly to a type of element. For example, you can navigate by headings, links, buttons, or images, among other options. Notably, if the poltly graphs have role="img" associated with them, a screen reader user could get that graph in their list of images, and then be able to jump directly to it.

While it's obviously best for screen reader support to always be on, it's still perfectly reasonable to put accessibility behind a button, as long as that button is discoverable. The button can be considered discoverable if:

  1. It's located nearby in the DOM - being visually nearby doesn't help.
  2. Clearly labeled, such as saying "Turn on accessibility mode" or "Turn on screen reader support".

I hope that's helpful. Please let me know if you have any other questions!

julianna-langston avatar Jan 26 '24 16:01 julianna-langston

thanks @julianna-langston, I take it the <div> must have id=cc, or would other id work?

Todo list + structure moved to new comment

~Proposed Structure~ (obsolete):

  • Two c2m functions: accessibility.fullRedraw() accessibility.reprocessData()

    • accessibility.fullRedraw() will create a c2m instance in gd._context._c2m

    • accessibility.reprocessData() if traces are added or removed, I suppose (what if user zooms? can we get visible?)

  • Look into how accessibility configs (stored in _context) are changed in run time- if through a c2m api, c2m must communicate config change to gd to avoid transitionFromReact() and prompt whole-redraw. But if c2m is using its options object for example as short term memory, it shouldn't trigger a configFlag change. So c2m/accessibility.js need to understand when they should trigger a full redraw of the plot. Likewise, gd will seq.push() the appropriate accessibility. function during its many render functions but have to decide which is called and where.

Future This will play into my thoughts about adding optional down-sampling later this year, since listening to over-renderer data would be worse than seeing it. Interestingly, a competitor to dash who uses plotly implemented some algorithms and published about it, but didn't push it into plotly. Forget who off the top of my head.

ayjayt avatar Jan 26 '24 17:01 ayjayt

I take it the

must have id=cc, or would other id work?

You can pass any kind of DIV you want to the c2mChart's cc option. The DIV can have any value for ID, or even not define an ID, and it will still work.

julianna-langston avatar Jan 26 '24 18:01 julianna-langston

Thank you for working on this project, everyone. We're trying to figure out a way to support ES6 in our code base. We should have an update about this soon, which will allow us to make more progress implementing Chart2Music.

Coding-with-Adam avatar Feb 09 '24 17:02 Coding-with-Adam

yeah rollup can do it ~out of box~ pretty easily tbh.

edit: in fact, the push i submitted to chart2music creates a CJS using babel, but since they already use rollup i'm going to migrate my solution to rollup so we can eliminate that dependency (babel).

ayjayt avatar Feb 09 '24 17:02 ayjayt

@ayjayt FYI we're working on transitioning this project from webpack to esbuild. webpack can of course be made to handle modern syntax and output to any feature set we want, but this project has some odd quirks particularly around the WebGL code that have made it tricky for us to get right, and anyway we're provisionally seeing some great benefits to switching to esbuild. So hang on just a bit, hopefully next week we'll have this sorted out and be able to move forward 🙏

alexcjohnson avatar Feb 09 '24 18:02 alexcjohnson

ESM/CJS preferences aside, here's new todo:

  • [x] merge with main and make go green again

Little Stuff

  • [ ] sensible defaults for CC generation (off screen)
  • [ ] use Lib.getGraphDiv()
  • [ ] Add role=img?
  • [ ] test user-flow with actual screen reader (always on? toggleable via modebar? modebar is accessible?)
  • [ ] get rid of "Test" cc text in dev dashboard

Big Stuff

  • [x] c2m should copy default config
  • [x] c2m should store its handler in a hidden _context variable (relook at how config spurs redraw)
  • [x] c2m should also store it's copied config in hidden _context variables
  • [x] ~When c2m is reinitialized in the same plot (for whatever reason), use the stored, not the default, so that any config changed at runtime is maintained~ (not sure where the use for this is)
  • [ ] Add function to only redraw data
  • [x] c2m will never provoke plotly to redraw, all data flows from plotly to c2m
  • [ ] c2m data can be redrawn on the zoom/pan callback (i think there is a callback), although I'm not quite sure how a visually impaired person would use zoom/pan yet. make sure we get current selected data point and make that same current selected data point after change? unless out of frame? hmm
  • [x] ~follow instructions in c2m documentation about supporting hover~ (had already been done)
  • [ ] see if maybe c2m left/right hotkeys can cause panning when we get to end of data
  • [ ] add ability to skip notes
  • [ ] add modebar button
  • [ ] all conversions from plotly-->c2m data structures will be modularized as codecs
  • [ ] failed conversions will have a debug option to dump information so new codecs can be written
  • [ ] add support to manually switch between discrete/continuous mode in c2m, perhaps as precursor to intelligently deciding.
  • [ ] support for locale and language

ayjayt avatar Feb 24 '24 15:02 ayjayt

I'm happy to help with the Chart2Music side! Please let me know what I can do to facilitate knowledge transfer, or if there are specific features you need from C2M.

julianna-langston avatar Feb 24 '24 15:02 julianna-langston

Hi @julianna-langston ! I think that the c2m library is fine, when I say "c2m side" I'm just referring to the plotly module that implements it.

ayjayt avatar Feb 24 '24 15:02 ayjayt

I've refactored this branch to give accessibility.js a proper API.

Since accessibility.js is set up to support multiple accessibility libraries, accessibility.js no longer contains c2m-specific logic, but instead calls a c2m library internal to plotly (a wrapper for the actual chart2music library).

This to-do list builds on the above:

  • [ ] The code is now littered with TODO's that need to be addressed, so these two lists are not comprehensive!
  • [ ] Need to create *_codec.model files which will serve as guides for future codecs
  • [ ] Need to reread C2M docs about hiding/showing traces
  • [ ] Need to consider live data
  • [ ] Need to get plotly expert to tell where plotly should call full-scale reinit, or just reinit data.
  • [ ] Need to consider data width
  • [ ] https://github.com/plotly/plotly.js/pull/6680#discussion_r1272787262 If you're interested in the development, consider the development docs.

ayjayt avatar Feb 28 '24 04:02 ayjayt

hi @ayjayt Thank you for all your work on this. Can I please have your email or linkedin? I'd like to share more side thoughts with you. [email protected]

Thanks,

Coding-with-Adam avatar Feb 28 '24 15:02 Coding-with-Adam

hi @ayjayt Thank you for all your work on this. Can I please have your email or linkedin? I'd like to share more side thoughts with you. [email protected]

Thanks,

Sure,

https://www.linkedin.com/in/ajpikul/ [email protected]

Although this thread is a great place to have the discussion as well!

ayjayt avatar Feb 28 '24 15:02 ayjayt

Although this thread is a great place to have the discussing as well!

Of course, I plan to discuss issues related to this topic only here. Just wanted to share a personal note via email. Thanks for sharing.

Coding-with-Adam avatar Feb 28 '24 15:02 Coding-with-Adam

Alright, I opened #6909 and merge it into chart2music_dev branch. All the tests are green there. So let's pull/merge that here.

archmoj avatar Feb 28 '24 19:02 archmoj

thanks @archmoj!

ayjayt avatar Feb 29 '24 00:02 ayjayt

Todo 1: https://github.com/plotly/plotly.js/pull/6680#issuecomment-1962404214 Todo 2: https://github.com/plotly/plotly.js/pull/6680#issuecomment-1968198454

just copying this here because it all got lost with the big merge

ayjayt avatar Mar 10 '24 03:03 ayjayt

https://github.com/julianna-langston/chart2music/pull/416

ayjayt avatar Mar 11 '24 03:03 ayjayt

This is on track (it's not obvious here because my last two blocks I've been focused on the c2m repository)! When the infrastructure is done (probably next weekend), it will be ready for systematic integration with all of plotly's plot types. I will get us started with 2 or 3 (aliwelchoo already did a lot of linear), but I'm not as familiar with all of plotly's plot types as some of you so I will write documentation to make it easy, organized, and systematic to:

  1. discover which of plotly's graph types aren't supported yet
  2. write the pursuant support

I promise it will be very straightforward.

ayjayt avatar Mar 18 '24 01:03 ayjayt

Thank you so much with your support on this, @ayjayt

Coding-with-Adam avatar Mar 18 '24 14:03 Coding-with-Adam

We fixed a problem on CircleCI (#6935). Please pull upstream/master and merge it into this branch.

archmoj avatar Mar 20 '24 20:03 archmoj

I'm back 😎, lets see what we can´t finish today :-)

ayjayt avatar Apr 01 '24 17:04 ayjayt