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

Data table is rendered incorrectly for charts with bullet and line series #7400

Closed
AleksueiR opened this Issue Nov 14, 2017 · 18 comments

Comments

Projects
None yet
4 participants
@AleksueiR

AleksueiR commented Nov 14, 2017

Expected behaviour

Headers for bullet and line series rendered on a single line; bullet series subheaders underneath the corresponding header.

Actual behaviour

Headers for bullet series are rendered as the top line and the line series header is rendered underneath one of the bullet header.

Live demo with steps to reproduce

http://jsfiddle.net/ouroborosDragon/aga1k7nd/7/

Affected browser(s)

Chrome, IE11, likely others.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Nov 15, 2017

Contributor

Hi @AleksueiR

Thank you for reporting this bug. It looks like issue comes from accessibility module, without this module it works better: http://jsfiddle.net/aga1k7nd/9/

I tried to workaround the issue by setting columnHeaderFormatter, but unfortunately it's wrapped and top row of series names can not be accessed. That's how far I got: http://jsfiddle.net/twkvLhd0/ - still we are missing some cell for the first category/year.

@oysteinmoseng

Contributor

pawelfus commented Nov 15, 2017

Hi @AleksueiR

Thank you for reporting this bug. It looks like issue comes from accessibility module, without this module it works better: http://jsfiddle.net/aga1k7nd/9/

I tried to workaround the issue by setting columnHeaderFormatter, but unfortunately it's wrapped and top row of series names can not be accessed. That's how far I got: http://jsfiddle.net/twkvLhd0/ - still we are missing some cell for the first category/year.

@oysteinmoseng

@pawelfus pawelfus added the Bug label Nov 15, 2017

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Nov 16, 2017

Collaborator

@oysteinmoseng I think it would be cleaner if we just do what we have to do in the export-data module instead of wrapping it from the accessibility module.

Collaborator

TorsteinHonsi commented Nov 16, 2017

@oysteinmoseng I think it would be cleaner if we just do what we have to do in the export-data module instead of wrapping it from the accessibility module.

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Nov 16, 2017

Collaborator

Yes, it would for sure be a lot cleaner to build the accessibility functionality into export-data. We are just talking about simple markup after all.

Collaborator

oysteinmoseng commented Nov 16, 2017

Yes, it would for sure be a lot cleaner to build the accessibility functionality into export-data. We are just talking about simple markup after all.

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Nov 16, 2017

@pawelfus Thank you for the workaround with the formatter since I do need to keep the accessibility module enabled.

Missing cells in the first row can be fixed by adding a null data point ({ x: 6, y: null }) to the "Bullet Series 4": https://jsfiddle.net/ouroborosDragon/twkvLhd0/2/

AleksueiR commented Nov 16, 2017

@pawelfus Thank you for the workaround with the formatter since I do need to keep the accessibility module enabled.

Missing cells in the first row can be fixed by adding a null data point ({ x: 6, y: null }) to the "Bullet Series 4": https://jsfiddle.net/ouroborosDragon/twkvLhd0/2/

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Nov 21, 2017

@TorsteinHonsi I think this will be the best solution as I ran into another problem.

I need the table to be rendered in a different node, not immediately after the chart, so I specify a custom viewData.onclick implementation under menuItemDefinitions.

viewData: function() { customDiv.innerHTML = this.getTable(); }

This works well enough and the table is rendered where specified. However, none of the accessibility attributes are applied to this table anymore since the accessibility module wraps the original viewData prototype function to add these attributes. The original function is no longer called and no attributes are applied.

Any suggestions on how to deal with this?

AleksueiR commented Nov 21, 2017

@TorsteinHonsi I think this will be the best solution as I ran into another problem.

I need the table to be rendered in a different node, not immediately after the chart, so I specify a custom viewData.onclick implementation under menuItemDefinitions.

viewData: function() { customDiv.innerHTML = this.getTable(); }

This works well enough and the table is rendered where specified. However, none of the accessibility attributes are applied to this table anymore since the accessibility module wraps the original viewData prototype function to add these attributes. The original function is no longer called and no attributes are applied.

Any suggestions on how to deal with this?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Nov 21, 2017

Collaborator

You can create your own chart.dataTableDiv to hijack the logic in the viewData function: http://jsfiddle.net/quL1ajuc/

Collaborator

TorsteinHonsi commented Nov 21, 2017

You can create your own chart.dataTableDiv to hijack the logic in the viewData function: http://jsfiddle.net/quL1ajuc/

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Nov 21, 2017

@TorsteinHonsi thanks, that will do.

AleksueiR commented Nov 21, 2017

@TorsteinHonsi thanks, that will do.

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Dec 4, 2017

Err, this doesn't work together with the accessibility module, since the function adding accessibility attributes is skipped if the dataTableDiv is already defined:

if (!this.dataTableDiv) {

AleksueiR commented Dec 4, 2017

Err, this doesn't work together with the accessibility module, since the function adding accessibility attributes is skipped if the dataTableDiv is already defined:

if (!this.dataTableDiv) {

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Dec 4, 2017

Collaborator

@AleksueiR I think you would want to add these attributes manually here, the accessibility module will not really make intelligent guesses with your custom table regardless.

Collaborator

oysteinmoseng commented Dec 4, 2017

@AleksueiR I think you would want to add these attributes manually here, the accessibility module will not really make intelligent guesses with your custom table regardless.

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Dec 4, 2017

@oysteinmoseng I'll probably do that, but I'm not using a custom table here - I'm just trying to render it in a custom location.

Works fine: http://jsfiddle.net/ouroborosDragon/quL1ajuc/3/
Stops working after adding the accessibility module: http://jsfiddle.net/ouroborosDragon/quL1ajuc/2/

AleksueiR commented Dec 4, 2017

@oysteinmoseng I'll probably do that, but I'm not using a custom table here - I'm just trying to render it in a custom location.

Works fine: http://jsfiddle.net/ouroborosDragon/quL1ajuc/3/
Stops working after adding the accessibility module: http://jsfiddle.net/ouroborosDragon/quL1ajuc/2/

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Dec 4, 2017

Collaborator

@AleksueiR Ouch, yes, that is obviously not intentional. I'll push a quick fix.

Collaborator

oysteinmoseng commented Dec 4, 2017

@AleksueiR Ouch, yes, that is obviously not intentional. I'll push a quick fix.

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR commented Dec 4, 2017

@oysteinmoseng Thanks!

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng
Collaborator

oysteinmoseng commented Dec 4, 2017

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Dec 4, 2017

Yeap, the table renders now, but no accessibility attributes are applied.
Is this the intended behaviour - if I'm rendering the table outside of the default location, I should be responsible for its accessibility attributes?

AleksueiR commented Dec 4, 2017

Yeap, the table renders now, but no accessibility attributes are applied.
Is this the intended behaviour - if I'm rendering the table outside of the default location, I should be responsible for its accessibility attributes?

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Dec 4, 2017

Collaborator

@AleksueiR Intended for now, but we will be rewriting this logic and moving the accessibility attributes to the export-data module eventually. Looking at the code, however, I am not sure why the accessibility logic is bypassed in this case. I think it may have been meant as a safeguard against adding the table multiple times, but that does not seem to be an issue regardless now.

Collaborator

oysteinmoseng commented Dec 4, 2017

@AleksueiR Intended for now, but we will be rewriting this logic and moving the accessibility attributes to the export-data module eventually. Looking at the code, however, I am not sure why the accessibility logic is bypassed in this case. I think it may have been meant as a safeguard against adding the table multiple times, but that does not seem to be an issue regardless now.

@AleksueiR

This comment has been minimized.

Show comment
Hide comment
@AleksueiR

AleksueiR Dec 4, 2017

@oysteinmoseng Fair enough. I'll write my own accessibility logic for now. Thank you for the help!

I wonder if it makes sense to modify the viewData prototype function so it accepts an optional node reference for the table to be rendered on top of. If not specified, it falls back to the default location.

AleksueiR commented Dec 4, 2017

@oysteinmoseng Fair enough. I'll write my own accessibility logic for now. Thank you for the help!

I wonder if it makes sense to modify the viewData prototype function so it accepts an optional node reference for the table to be rendered on top of. If not specified, it falls back to the default location.

@oysteinmoseng

This comment has been minimized.

Show comment
Hide comment
@oysteinmoseng

oysteinmoseng Dec 4, 2017

Collaborator

@AleksueiR You can of course do that, although that is kind of what chart.dataTableDiv does

Collaborator

oysteinmoseng commented Dec 4, 2017

@AleksueiR You can of course do that, although that is kind of what chart.dataTableDiv does

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 8, 2017

Collaborator

@oysteinmoseng Will you do the change we agreed on? Make the export-data.js table aria-compatible out of the box, so that we can remove the override in accessibility.js.

Collaborator

TorsteinHonsi commented Dec 8, 2017

@oysteinmoseng Will you do the change we agreed on? Make the export-data.js table aria-compatible out of the box, so that we can remove the override in accessibility.js.

AleksueiR added a commit to AleksueiR/dqvue that referenced this issue Dec 21, 2017

update cesi 03.2 sample
Highchart fixed bug with multi-level headers

Related highcharts/highcharts#7400

AleksueiR added a commit to AleksueiR/dqvue that referenced this issue Dec 21, 2017

update cesi 03.2 sample
Highchart fixed bug with multi-level headers

Related highcharts/highcharts#7400
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment