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

Skip keyboard navigation for series that are not visibile #6838

Closed
primerano opened this Issue Jun 14, 2017 · 17 comments

Comments

Projects
None yet
5 participants
@primerano

primerano commented Jun 14, 2017

Expected behaviour

I thought this was a feature request but it might be a bug.

If I have 3 lines and one is hidden, i don't expect keyboard navigation to navigate the hidden series.

Actual behaviour

Keyboard navigation walks the visible and hidden series

Live demo with steps to reproduce

See https://forum.highcharts.com/post135758.html for background.

I made updates to H.Chart.prototype.highlightAdjacentPoint that mostly worked here
http://jsfiddle.net/primerano/44pum7oq/1/

But after looking at the code I think this line might be wrong.

if ( newPoint.isNull && this.options.accessibility.keyboardNavigation.skipNullPoints || newPoint.series.options.skipKeyboardNavigation // docs ) {

I think this code should actually skip if null and skip null is set OR skipKeyboardNavigation is set as below.

if ( (newPoint.isNull && this.options.accessibility.keyboardNavigation.skipNullPoints) || newPoint.series.options.skipKeyboardNavigation // docs ) {

I guess the question is. Should HighCharts skip hidden series or should HighCharts skip series where skipKeyboardNavigation is true?

Neither works at the moment but I would like to know if this is a feature that is planned or that is broken.

Thanks!!!
Tony

Affected browser(s)

All. Not browser specific.

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Jun 15, 2017

Collaborator

@primerano Thanks for reporting! The two code lines you mention are functionally identical due to operator precedence in JS, so the skipKeyboardNavigation option should work with the current code. Do you have an example where it doesn't work?

We should obviously also skip hidden series, so I'll quickly add in a check for that.

Collaborator

oysteinmoseng commented Jun 15, 2017

@primerano Thanks for reporting! The two code lines you mention are functionally identical due to operator precedence in JS, so the skipKeyboardNavigation option should work with the current code. Do you have an example where it doesn't work?

We should obviously also skip hidden series, so I'll quickly add in a check for that.

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jun 15, 2017

I guess the operator precedence works. my bad.

I do notice that the 1st point in the hidden series is available to the keyboard navigation when using skipKeyboardNavigation.

See http://jsfiddle.net/primerano/x0d7325h/1/

click "Hide Series". shift tab to the chart, space to enter chart and then start tabbing. 1st point selected is in the hidden series.

Also, at the end it tabs through the hidden "Series 1" legend item.

I see you already made a commit to allow skipping of hidden series. How can I test that code out?

Thanks
Tony

primerano commented Jun 15, 2017

I guess the operator precedence works. my bad.

I do notice that the 1st point in the hidden series is available to the keyboard navigation when using skipKeyboardNavigation.

See http://jsfiddle.net/primerano/x0d7325h/1/

click "Hide Series". shift tab to the chart, space to enter chart and then start tabbing. 1st point selected is in the hidden series.

Also, at the end it tabs through the hidden "Series 1" legend item.

I see you already made a commit to allow skipping of hidden series. How can I test that code out?

Thanks
Tony

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Jun 16, 2017

Collaborator

@primerano There were definitely some bugs in the existing code, so it's great that you reported. With current code it works as it should, I believe: http://jsfiddle.net/x0d7325h/2/. You can load the latest accessibility module from github.highcharts.com/hc5-fixes/modules/accessibility.js.

Tabbing through hidden legend items is on purpose, as it allows keyboard-only users to hide/show series using the legend.

Collaborator

oysteinmoseng commented Jun 16, 2017

@primerano There were definitely some bugs in the existing code, so it's great that you reported. With current code it works as it should, I believe: http://jsfiddle.net/x0d7325h/2/. You can load the latest accessibility module from github.highcharts.com/hc5-fixes/modules/accessibility.js.

Tabbing through hidden legend items is on purpose, as it allows keyboard-only users to hide/show series using the legend.

@oysteinmoseng oysteinmoseng added this to the 5.0.13 milestone Jun 16, 2017

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jun 16, 2017

Is there a way to disable legend navigation? In our case we have a line with all 50 states. by default we show 1 and let folks hide or show the series via a select2 control as having 50 items in the legend is a bit much,

At the moment we stop the legend clicks via

line: {
events: {
legendItemClick: function () {
return false;
}
}

but this doesn't affect keyboard navigation.

Maybe this is a better option?

legend: {navigation: {enabled: false}}

This didn't seem to do anything for me. maybe it is the wrong option?

I'll try out your fix for the series navigation next week and let you know if i find issues.

Thanks

primerano commented Jun 16, 2017

Is there a way to disable legend navigation? In our case we have a line with all 50 states. by default we show 1 and let folks hide or show the series via a select2 control as having 50 items in the legend is a bit much,

At the moment we stop the legend clicks via

line: {
events: {
legendItemClick: function () {
return false;
}
}

but this doesn't affect keyboard navigation.

Maybe this is a better option?

legend: {navigation: {enabled: false}}

This didn't seem to do anything for me. maybe it is the wrong option?

I'll try out your fix for the series navigation next week and let you know if i find issues.

Thanks

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Jun 19, 2017

Collaborator

@primerano I added a new option legend.keyboardNavigation.enabled to allow this. Example: http://jsfiddle.net/oysteinmoseng/ugkgt4dj/

Collaborator

oysteinmoseng commented Jun 19, 2017

@primerano I added a new option legend.keyboardNavigation.enabled to allow this. Example: http://jsfiddle.net/oysteinmoseng/ugkgt4dj/

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jun 20, 2017

Thanks for the quick feedback and feature add. I'm not sure if the changes are in the hc5-fixes anymore because your example is navigating the series now

http://jsfiddle.net/x0d7325h/2/

And I made one that uses the new legend.keyboardNavigation setting and it doesn't work either

http://jsfiddle.net/x0d7325h/7/

How often do you release patch releases? Woudl 5.0.13 be out this month?

primerano commented Jun 20, 2017

Thanks for the quick feedback and feature add. I'm not sure if the changes are in the hc5-fixes anymore because your example is navigating the series now

http://jsfiddle.net/x0d7325h/2/

And I made one that uses the new legend.keyboardNavigation setting and it doesn't work either

http://jsfiddle.net/x0d7325h/7/

How often do you release patch releases? Woudl 5.0.13 be out this month?

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Jun 20, 2017

Collaborator

@primerano The changes are still there, but there seems to be an issue with the github.highcharts.com service returning the wrong file somehow.

Regardless, the changes are also now in master so you can use github.highcharts.com/modules/accessibility.js which seems to work.

We do release patch versions roughly every month or so, and do expect 5.0.13 to be out in a few weeks max.

Collaborator

oysteinmoseng commented Jun 20, 2017

@primerano The changes are still there, but there seems to be an issue with the github.highcharts.com service returning the wrong file somehow.

Regardless, the changes are also now in master so you can use github.highcharts.com/modules/accessibility.js which seems to work.

We do release patch versions roughly every month or so, and do expect 5.0.13 to be out in a few weeks max.

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jun 21, 2017

I just realized that you can still keyboard navigate to the hidden series with the up/down arrows. Can we get a fix for that with this issue too?

primerano commented Jun 21, 2017

I just realized that you can still keyboard navigate to the hidden series with the up/down arrows. Can we get a fix for that with this issue too?

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jul 18, 2017

Should we open a separate issue against the up.down arrow keys navigating into hidden series?

primerano commented Jul 18, 2017

Should we open a separate issue against the up.down arrow keys navigating into hidden series?

@sebastianbochan

This comment has been minimized.

Show comment
Hide comment
@sebastianbochan

sebastianbochan Jul 19, 2017

Contributor

Hi @primerano
It looks like keyboardNavigation parameter only enables / disables navigation, but does not "consider" hidden series.

Thank you for the reporting and you do not create new case at this moment.

Contributor

sebastianbochan commented Jul 19, 2017

Hi @primerano
It looks like keyboardNavigation parameter only enables / disables navigation, but does not "consider" hidden series.

Thank you for the reporting and you do not create new case at this moment.

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Jul 19, 2017

@sebastianbochan - will a fix for the arrow navigation be done under this issue #6838 or is there another issue i should follow?

primerano commented Jul 19, 2017

@sebastianbochan - will a fix for the arrow navigation be done under this issue #6838 or is there another issue i should follow?

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Jul 20, 2017

Contributor

Let's reopen this issue.

@oysteinmoseng

Contributor

pawelfus commented Jul 20, 2017

Let's reopen this issue.

@oysteinmoseng

@pawelfus pawelfus reopened this Jul 20, 2017

@TorsteinHonsi TorsteinHonsi removed this from the 5.0.13 milestone Jul 27, 2017

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Sep 20, 2017

@pawelfus is there a chance we can get the up-down arrow into the hidden series fixed in the next release?

http://jsfiddle.net/primerano/x0d7325h/8/

primerano commented Sep 20, 2017

@pawelfus is there a chance we can get the up-down arrow into the hidden series fixed in the next release?

http://jsfiddle.net/primerano/x0d7325h/8/

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 20, 2017

Contributor

I'm not sure, @oysteinmoseng what do you think?

Contributor

pawelfus commented Sep 20, 2017

I'm not sure, @oysteinmoseng what do you think?

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Sep 21, 2017

Collaborator

@primerano Should be doable, I'll add it as a milestone

Collaborator

oysteinmoseng commented Sep 21, 2017

@primerano Should be doable, I'll add it as a milestone

@primerano

This comment has been minimized.

Show comment
Hide comment
@primerano

primerano Oct 5, 2017

@oysteinmoseng was tab navigation of a line removed? Before I could tab through the series. now only up/down arrow are available to skip ahead and back in the series.

primerano commented Oct 5, 2017

@oysteinmoseng was tab navigation of a line removed? Before I could tab through the series. now only up/down arrow are available to skip ahead and back in the series.

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Oct 6, 2017

Collaborator

@primerano Yes, tab navigation is now used to navigate between the major chart sections (points, exporting menu, legend, etc). To move between points/series, use arrow keys (up/down/left/right). Our testing so far indicates that this is fairly intuitive to users, and has the added benefit of making chart navigation through charts with lots of points much easier. A major issue with the previous navigation model was that users were unable to tab past charts with lots of points without going through all the points of the chart.

Collaborator

oysteinmoseng commented Oct 6, 2017

@primerano Yes, tab navigation is now used to navigate between the major chart sections (points, exporting menu, legend, etc). To move between points/series, use arrow keys (up/down/left/right). Our testing so far indicates that this is fairly intuitive to users, and has the added benefit of making chart navigation through charts with lots of points much easier. A major issue with the previous navigation model was that users were unable to tab past charts with lots of points without going through all the points of the chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment