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 Chart2Music keyboard and sound accessibility features #6680

Open
wants to merge 81 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
81 commits
Select commit Hold shift + click to select a range
6fb9c65
initial commit, single scatter plot
Jul 21, 2023
87b4c5c
single scatter plot
Jul 21, 2023
64ae505
options and info
Jul 21, 2023
d37d24f
data and trace labels
Jul 21, 2023
d0467bb
false default
Jul 21, 2023
3a73776
remove debugging prints
Jul 21, 2023
b4df959
use index if no x provided
Jul 21, 2023
97b608f
Lib requirement
Jul 22, 2023
6205d62
layout destructuring
Jul 22, 2023
7e40cde
layout defaults
Jul 22, 2023
6d9185b
destructure trace
Jul 22, 2023
935cf54
test sourcetype module
Jul 22, 2023
44deae2
remove sourcetype
Jul 22, 2023
c38ab67
into es5
Jul 28, 2023
c6a27be
include mjs files
Jul 28, 2023
cac174c
transpile accessibility
Jul 30, 2023
159ca1b
accessibility in config
Jul 30, 2023
7467a47
Merge branch 'master' into chart2music
ayjayt Jan 12, 2024
08b820b
Remove special rules from webpack.config:
ayjayt Jan 12, 2024
41b0fb9
Reorder declarations to make a bit more readable
ayjayt Jan 12, 2024
94e25c3
Lint
ayjayt Jan 13, 2024
61ea8ce
Change import/export scheme to promise
ayjayt Jan 17, 2024
c9a0b91
Remove a console.log
ayjayt Jan 17, 2024
cc92639
Limit chunks to 1
ayjayt Jan 17, 2024
7baa0d3
Upgrade chart2music to support commonjs
ayjayt Jan 17, 2024
3127dee
Rewrite accessibility in es5
ayjayt Jan 17, 2024
2429e55
Merge pull request #1 from geopozo/pikul-music-import-promise
aliwelchoo Jan 17, 2024
00572e8
Fix some undef errors in accessibility
ayjayt Jan 17, 2024
30f4e11
update plot-schema diff
ayjayt Jan 17, 2024
247b301
Merge branch 'master' into pikul-music-import-promise
ayjayt Jan 25, 2024
d08adfb
Add config so dev can place closed captions:
ayjayt Jan 25, 2024
dbce5da
Merge branch 'chart2music' of https://github.com/aliwelchoo/plotly.js…
ayjayt Jan 25, 2024
e1f9563
Add more default assurance in accessibility
ayjayt Jan 25, 2024
3f4ab54
Lint
ayjayt Jan 25, 2024
92d97b7
Skip empty trace data in accessibility.enable:
ayjayt Jan 25, 2024
6b2dcc9
update plot-schema diff
ayjayt Jan 25, 2024
b5bb517
Modify how accessibility treats config vars:
ayjayt Jan 26, 2024
25ca44a
Remove jasmine ftest focus
ayjayt Jan 26, 2024
7328324
Run schema plot diff
ayjayt Jan 26, 2024
00b2750
Test if previous commits deprecate hack-fix
ayjayt Jan 26, 2024
b04bb17
Set c2music default to not make its caption div:
ayjayt Jan 26, 2024
d4bd71b
update plot-scheme diff
ayjayt Jan 26, 2024
af2ff80
Add closed captions to dev dashboard
ayjayt Jan 26, 2024
80b4cf3
Merge pull request #2 from geopozo/pikul-music-import-promise
aliwelchoo Jan 28, 2024
ee99f35
Merge branch 'master' into chart2music
ayjayt Jan 29, 2024
3841640
Merge branch 'master' into chart2music
ayjayt Feb 9, 2024
d84d108
Merge branch 'master' into chart2music
ayjayt Feb 28, 2024
0c43407
Refactor accessibility/c2m:
ayjayt Feb 28, 2024
4c75035
Lint
ayjayt Feb 28, 2024
3c49ed6
Fix import path
ayjayt Feb 28, 2024
f7be666
Merge remote-tracking branch 'origin/build-with-esbuild' into chart2m…
archmoj Feb 28, 2024
4a260af
Merge branch 'master' into chart2music
ayjayt Mar 10, 2024
f67c513
Merge branch 'master' into chart2music
ayjayt Mar 20, 2024
41baab9
Merge branch 'master' into chart2music
ayjayt Mar 27, 2024
26754b7
Merge with master
marthacryan Sep 16, 2024
ed18907
Change name of config to sonification, clean up directory
marthacryan Sep 23, 2024
1e4bcef
Spelling
marthacryan Sep 23, 2024
2c1abf4
fix directory name
marthacryan Sep 23, 2024
614fb8b
Reset package-lock
marthacryan Sep 23, 2024
5f5e599
Update package-lock.json
marthacryan Sep 23, 2024
fe9736e
Reset tests
marthacryan Sep 23, 2024
59ad5c6
Update auto-generated schema
marthacryan Sep 23, 2024
d18c584
Rename more stuff to sonification and clean up contributing
marthacryan Sep 24, 2024
043d408
Update filepath for new location of enable_sonification.js
marthacryan Sep 24, 2024
129eb3b
Code readability improvements
marthacryan Sep 24, 2024
acc8517
Add baseline for new mock
marthacryan Sep 24, 2024
c5ad240
Remove changes in existing mock
marthacryan Sep 24, 2024
0113d90
Update baseline
marthacryan Sep 24, 2024
771029e
Remove function that was introduced through incorrect merge
marthacryan Sep 24, 2024
bfbbbf1
Undo changes to jasmine tests
marthacryan Sep 25, 2024
cadfd7f
Revert "Code readability improvements"
marthacryan Sep 25, 2024
56e582d
Fix syntax
marthacryan Sep 25, 2024
8e55f98
remove comment
marthacryan Sep 25, 2024
86d75d8
Remove todo
marthacryan Sep 25, 2024
15671b0
Schema update
marthacryan Oct 1, 2024
057e067
Merge branch 'master' of github.com:plotly/plotly.js into chart2music
marthacryan Oct 1, 2024
44f7ad3
Remove accessibility wrapper directory
marthacryan Oct 1, 2024
8e3b62b
Fix filepaths
marthacryan Oct 1, 2024
aed1e02
Code cleanup
marthacryan Oct 1, 2024
1777fba
Code cleanup
marthacryan Oct 1, 2024
3a0384f
Code cleanup
marthacryan Oct 1, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"@turf/bbox": "^6.4.0",
"@turf/centroid": "^6.0.2",
"canvas-fit": "^1.5.0",
"chart2music": "^1.11.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice MIT library.
To transpile it to es5, please add it to

include: /node_modules[\\\/](buffer|d3-color|d3-interpolate|is-mobile)[\\\/]/,

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! Knew there'd be something simple I was missing so I could get this into the same style

"color-alpha": "1.0.4",
"color-normalize": "1.5.0",
"color-parse": "1.3.8",
Expand Down
71 changes: 71 additions & 0 deletions src/plot_api/accessibility.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
"use strict";
import c2mChart from "chart2music";
Copy link
Contributor

Choose a reason for hiding this comment

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

To pass current es5 build tests, this file should be written in es5 style similar other files to src.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need to be writing in es5? Won't webpack take that and give us the desired output? I'd love it if we can start accepting (and eventually converting existing code to) more modern syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

For es modules, we have a config rule in place.
I did try to add a rule to transpile this file during build processes; but npm run build wasn't successful in the past hour.
If one could help with setting up the webpack config, it would be great 🙏

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for these tips again! I've made some attempts at this (putting chart2music and/or accessibility.js into the webpack config but struggling with "Cannot find module 'chart2music'" whenever I use require instead of import. I think I'm still missing something


export function enable (gd) {

const c2mData = {};
const labels = [];

const fullData = gd._fullData;

for(var i = 0; i < fullData.length; i++) {
var trace = fullData[i] ?? {};
var {type, x = [], y = [], name = i, text = []} = trace;
if(type === 'scatter') {
var traceData = [];
if ('y' in trace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In scatter traces with orientation: 'h', we should use trace.x instead of trace.y.

for(var p = 0; p < y.length; p++) {
traceData.push(
{
x: x.length > 0 ? x[p] : p,
y: y[p],
label: text[p] ?? p
})
}
c2mData[name] = traceData;
labels.push(name);
}
}
else {
// 'Accessibility not implemented for trace type: ' + trace.type
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should use continue instead of return here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think so. There's some overview text c2m reads out when you first focus on the chart, right? Perhaps we can have any skipped traces briefly mentioned there?

Copy link
Author

Choose a reason for hiding this comment

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

I can look into this. Need to look into where that closed captions text element goes in general, not sure about outputting our own messages into it, it's more than just the overview at the start, also reads off the data as you navigate it.

But this general point is a good one, ideally want to be able to support more than just scatter traces and want to support figures containing unsupported traces alongside supported ones.

Then there's also the opportunity for a config option to say which traces should be in accessibility mode

Choose a reason for hiding this comment

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

This is an interesting use case. I've created a feature request for chart2music to add config so that you can specify skipped traces (aka unsupported layers). If you think that would work for you, I'll go ahead and get started on a PR for chart2music.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@julianna-langston that would be great! I think that's going to be really important for this project, so that plotly.js and Dash users who care about this can turn on chart2music in all of their charts, knowing that the result will not always be complete but will always be as useful as it can be and will tell them what they're missing. And then over time as both the plotly.js and the chart2music sides add more features, existing applications will automatically improve.

Choose a reason for hiding this comment

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

Chart2Music v1.12 now has an unsupported chart type. Examples and documentation

}
}

var closed_captions = document.createElement('div');
closed_captions.id = 'cc';
closed_captions.className = 'closed_captions';
gd.appendChild(closed_captions);

const {
title: {text: title_text = ''} = {},
xaxis: {title: {text: xaxis_text = ''} = {}} = {},
yaxis: {title: {text: yaxis_text = ''} = {}} = {},
} = gd._fullLayout;

c2mChart({
title: title_text,
type: "line",
Copy link

@julianna-langston julianna-langston Jul 25, 2023

Choose a reason for hiding this comment

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

If this PR is for scatter plots, you probably want to have type: "scatter".

In the future, if someone wants to support more chart types, chart2music supports an array. So, if you have 2 traces, 1 scatter and 1 line, you could convert that to chart2music's type: ["line", "scatter"]. Just fyi.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you. I think I'll end up extending the scope to as many types as we can

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for supporting our project, @julianna-langston

axes: {
x: {
label: xaxis_text
},
y: {
label: yaxis_text
}
},
element: gd,
cc: closed_captions,
data: c2mData,
options: {
onFocusCallback: ({slice, index}) => {
Plotly.Fx.hover(gd, [{
curveNumber: labels.indexOf(slice),
pointNumber: index
}])
},
...gd._context.chart2musicOptions
},
info: gd._context.chart2musicInfo
aliwelchoo marked this conversation as resolved.
Show resolved Hide resolved
});
};
8 changes: 6 additions & 2 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var manageArrays = require('./manage_arrays');
var helpers = require('./helpers');
var subroutines = require('./subroutines');
var editTypes = require('./edit_types');
var accessibility = require('./accessibility');

var AX_NAME_PATTERN = require('../plots/cartesian/constants').AX_NAME_PATTERN;

Expand Down Expand Up @@ -376,10 +377,13 @@ function _doPlot(gd, data, layout, config) {
// calculated. Would be much better to separate margin calculations from
// component drawing - see https://github.com/plotly/plotly.js/issues/2704
Plots.doAutoMargin,
saveRangeInitialForInsideTickLabels,
Plots.previousPromises
saveRangeInitialForInsideTickLabels
);

if(gd._context.chart2music) seq.push(accessibility.enable);

seq.push(Plots.previousPromises);

function saveRangeInitialForInsideTickLabels(gd) {
marthacryan marked this conversation as resolved.
Show resolved Hide resolved
if(gd._fullLayout._insideTickLabelsAutorange) {
if(graphWasEmpty) Axes.saveRangeInitial(gd, true);
Expand Down
20 changes: 19 additions & 1 deletion src/plot_api/plot_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,25 @@ var configAttributes = {
'instead of MM/DD/YYYY). Currently `grouping` and `currency` are ignored',
'for our automatic number formatting, but can be used in custom formats.'
].join(' ')
}
},

chart2music: {
valType: 'boolean',
dflt: true,
description: 'Whether or not to enable chart2music keyboard and sound accessibility',
},

chart2musicOptions: {
valType: 'any',
dflt: {},
description: 'Additional options to pass to the chart2music API. See https://www.chart2music.com/docs/API/Config',
},

chart2musicInfo: {
valType: 'any',
dflt: {},
description: 'Info to pass to the chart2music API. See https://www.chart2music.com/docs/API/Config',
},
};

var dfltConfig = {};
Expand Down