plotly.js
plotly.js copied to clipboard
add Chart2Music keyboard and sound accessibility features
Use data and layout to call the chart2music API inside makePlot.
This pull request is for scatter plots only
- 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
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
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):
(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:
Back to all this on friday-ish.
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.
Alright, I fixed the obvious stuff (including plot-schema) from those errors, but non-obvious stuff is:
- 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)
- 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
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()
inassets/
- 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:
- Set up basic config
- (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.
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:
- It's located nearby in the DOM - being visually nearby doesn't help.
- 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!
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 ac2m
instance ingd._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 ac2m
api,c2m
must communicate config change togd
to avoidtransitionFromReact()
and prompt whole-redraw. But ifc2m
is using itsoptions
object for example as short term memory, it shouldn't trigger aconfigFlag
change. Soc2m
/accessibility.js
need to understand when they should trigger a full redraw of the plot. Likewise,gd
willseq.push()
the appropriateaccessibility.
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.
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.
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.
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 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 🙏
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
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.
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.
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.
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,
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!
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.
Alright, I opened #6909 and merge it into chart2music_dev branch. All the tests are green there. So let's pull/merge that here.
thanks @archmoj!
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
https://github.com/julianna-langston/chart2music/pull/416
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:
- discover which of plotly's graph types aren't supported yet
- write the pursuant support
I promise it will be very straightforward.
Thank you so much with your support on this, @ayjayt
We fixed a problem on CircleCI (#6935).
Please pull upstream/master
and merge it into this branch.
I'm back 😎, lets see what we can´t finish today :-)