[tkranenburgrivm]

No description provided.

[tkranenburgrivm]

@hypebright Hi Veerle,
Annette approved one echarts graph on accessibility on SPIN. This is one of the keyboard features to make the legend okay for accessibility.
At the moment, I think this is a draft-PR, because I think I should still integrate this new function with your ro_e_build function (and perhaps depricate ro_e_keyboard_nav_legend_js?). We can discuss it after your holidays!

[hypebright]

@tkranenburgrivm , nice work!

Some remarks (specifically about integration):

• ro_e_theme() hides the built-in ECharts legend (legend: { show: false }) and draws its own HTML legend out of <span class="ro-legend-item"> elements (R/ro_e_build.R). The code you propose looks for the built-in legend's SVG paths. So on any chart that uses ro_e_theme() it finds nothing and quietly does nothing. So I would add the same behaviour to the HTML legend that's already there, which might be simpler as well.

How you could integrate it:

• ro_e_build_onrender_js(): in the seriesNames.forEach loop that builds each .ro-legend-item, make the item focusable. I think if you would use , you get Tab, Enter and Space for free, which is nice. Or you can add tabindex="0", role="button" and an Enter/Space keydown handler.
• ro_e_keyboard_nav(): the letter-key shortcuts (series_keys → ro_e_keyboard_nav_legend_js()) can be deprecated. Those listen on the whole window anyway, so they also fire while typing in inputs and hit every chart on the page. Wasn't ideal. And since we haven't released anything yet we can easily deprecate it now.

Other stuff I noticed (but didn't check in depth yet since this is still a draft):

• Mismatched argument names. The docs say @param fig, but the function takes fig2. This gives an R CMD check warning.
• Remove console.log calls.

[tkranenburgrivm]

@tkranenburgrivm , nice work!

Some remarks (specifically about integration):

• ro_e_theme() hides the built-in ECharts legend (legend: { show: false }) and draws its own HTML legend out of <span class="ro-legend-item"> elements (R/ro_e_build.R). The code you propose looks for the built-in legend's SVG paths. So on any chart that uses ro_e_theme() it finds nothing and quietly does nothing. So I would add the same behaviour to the HTML legend that's already there, which might be simpler as well.

How you could integrate it:

• ro_e_build_onrender_js(): in the seriesNames.forEach loop that builds each .ro-legend-item, make the item focusable. I think if you would use , you get Tab, Enter and Space for free, which is nice. Or you can add tabindex="0", role="button" and an Enter/Space keydown handler.
• ro_e_keyboard_nav(): the letter-key shortcuts (series_keys → ro_e_keyboard_nav_legend_js()) can be deprecated. Those listen on the whole window anyway, so they also fire while typing in inputs and hit every chart on the page. Wasn't ideal. And since we haven't released anything yet we can easily deprecate it now.

Other stuff I noticed (but didn't check in depth yet since this is still a draft):

• Mismatched argument names. The docs say @param fig, but the function takes fig2. This gives an R CMD check warning.
• Remove console.log calls.

@hypebright Hi Veerle, thanks a lot for the feedback!
I removed the logs and fixed the argument naming.
Good to point out that ro_e_theme removed the legend and added a new one. I add some functionality that it can handle with or without ro_e_theme.
I removed the letter-key-series now.
And the new function is still a stand-alone thing. Do we want to integrate the accessibility-functions in the ro_e_build, which is then used in ro_e_theme? Or keep accesibility and theming seperate?

[hypebright]

Ok, since there's a lot of related work. My proposal (also for internal documentation and overview):

Convention: *_theme() + *_standard()

We recently decided on the way to go for plotly, with the *_standard() function that serves as a high-level function and we said we wanted it in other packages too.

So, each package (now: ROvis.echarts, ROvis.plotly) exports two functions:

Function	Does	Who uses it
ro_e_standard() / ro_ply_standard()	Theme + all accessibility features	Default, recommended for everyone
ro_e_theme() / ro_ply_theme()	Styling only	Charts *_standard() doesn't cover, or styling-only needs

Everything else (keyboard navigation, screen reader fixes, cursor, toolbar, etc.) is internal and wired into *_standard().

Principles

1. Accessible by default. Users get all accessibility features without thinking about it, by calling *_standard(). Keeps it simple. As said, should be the default.
2. One feature = one switch (future work) We need an option to turn off some stuff. Features can be turned off with a logical argument, default TRUE:

   ro_e_standard(e,
     palette = "categorical",    # styling args are passed through to the theme
     keyboard_nav = TRUE,        # axis + legend keyboard navigation
     screenreader_order = TRUE   # title first + label order (#24, #25)
   )

3. Same argument names across packages wherever a feature exists in both.
4. Every *_theme() takes the chart and applies the styling to it directly, like ro_gg_theme(), ro_gt_theme() and ro_e_theme() already do.
5. Docs lead with *_standard(). The *_theme() help page says it does not include accessibility features and points to *_standard().
6. Combinations that don't work get an explicit warning. For example, renderer = "canvas" + screenreader_order = TRUE → cli_warn().

Overview per PR

ROvis.echarts #14 (this PR)

• Add ro_e_standard(): calls ro_e_theme() + keyboard navigation (a potential switch to turn off/on comes later)
• Make ro_e_keyboard_nav() internal (remove @export, add @Keywords internal)
• Keep ro_e_theme() exported, styling only
• Update the vignette to use ro_e_standard()
• NEWS entry

ROvis.echarts #24 / #25

• Keep ro_e_announce_title_first() and ro_e_sort_xaxis_labels() internal
• Call them from ro_e_standard() (in the future with a potential screenreader_order switch)
• Rename the fig argument to e (package convention)

ROvis.plotly #26

• Keep ro_ply_theme() exported, but as ro_ply_theme(fig, ...) that applies the styling directly, and drop ro_ply_add_theme()
• Unexporting the rest (cursor, dim, title, toolbar, legend, keyboard_nav_*) is good 👍
• Keep one vignette, built around ro_ply_standard(), instead of removing both

Future work: make keyboard navigation on by default (currently type = "none") and use the shared switch names

I think we should keep ro_*_theme exported, mainly because every ROvis package has a *_theme() (ro_gg_theme, ro_gt_theme, ro_dt_theme, ro_e_theme), it's good we can make a distinction between styling and accessibility feats, and not every chart will fit *_standard() (subplots, maps, ...), so styling-only still makes sense.

Does all of that makes sense? If so, I will open issues for the future work (on/off switch)

[hypebright]

See above ⏫

[tkranenburgrivm]

• Add ro_e_standard(): calls ro_e_theme() + keyboard navigation (a potential switch to turn off/on comes later)

Totally agree!

Do you think we should add the e_standard in this PR, or should it be a separate thing?

[hypebright]

• Add ro_e_standard(): calls ro_e_theme() + keyboard navigation (a potential switch to turn off/on comes later)

Totally agree!

Do you think we should add the e_standard in this PR, or should it be a separate thing?

Can be separate if you want to keep it smaller, but for my review it doesn’t matter since I reviewed the standalone functionality already. But yeah technically speaking the cleanest way is separate..🤓