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

HTML validator does fail #5835

Closed
kaljak opened this Issue Oct 15, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@kaljak

kaljak commented Oct 15, 2016

Expected behaviour

Highcharts should have no errors while HTML validation

Actual behaviour

There are three errors in line charts:

Error: Attribute width not allowed on element g at this point.
From line 587, column 5919; to line 587, column 6147
5"></path><g class="highcharts-series highcharts-series-0 highcharts-line-series highcharts-color-0 " transfor…)" clip-path="url(http://www.highcharts.com/demo/line-basic#highcharts-1)" width="288" height="663"><path 
Error: Attribute height not allowed on element g at this point.
From line 587, column 10449; to line 587, column 10677
/path></g><g class="highcharts-series highcharts-series-1 highcharts-line-series highcharts-color-1 " transfor…)" clip-path="url(http://www.highcharts.com/demo/line-basic#highcharts-1)" width="288" height="663"><path 
Error: Attribute isshadow not allowed on element path at this point.
From line 587, column 28516; to line 587, column 28897
)"></path><path fill="none" class="highcharts-label-box highcharts-tooltip-box" d="M 3.5 0.5 L 109.5 0.5 C 112…isShadow="true" stroke="#000000" stroke-opacity="0.15" stroke-width="1" transform="translate(1, 1)"></path

Live demo with steps to reproduce

Validate the chart from http://www.highcharts.com/demo/line-basic with the W3C HTML validator (https://validator.w3.org/nu/)

Affected browser(s)

All

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 17, 2016

Collaborator

Thanks for reporting!

For some reason, I don't see any errors related to the charts when validating that page in the nu validator. I see some other errors though. But I find it a little strange that you get this errors, because the SVG content is dynamically generated. If you view the source of the web page, you won't see any SVG code (like <path) at all.

Collaborator

TorsteinHonsi commented Oct 17, 2016

Thanks for reporting!

For some reason, I don't see any errors related to the charts when validating that page in the nu validator. I see some other errors though. But I find it a little strange that you get this errors, because the SVG content is dynamically generated. If you view the source of the web page, you won't see any SVG code (like <path) at all.

@kaljak

This comment has been minimized.

Show comment
Hide comment
@kaljak

kaljak Oct 17, 2016

Right, you need to use the direct input: https://validator.w3.org/#validate_by_input

kaljak commented Oct 17, 2016

Right, you need to use the direct input: https://validator.w3.org/#validate_by_input

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 17, 2016

Collaborator

It doesn't change anything, because the SVG is dynamically rendered by Highcharts, it is not part of the page source... You if you do View source, copy and paste the markup into the validator, you won't see any errors connected to the SVG. You probably pasted the generated source code?

Collaborator

TorsteinHonsi commented Oct 17, 2016

It doesn't change anything, because the SVG is dynamically rendered by Highcharts, it is not part of the page source... You if you do View source, copy and paste the markup into the validator, you won't see any errors connected to the SVG. You probably pasted the generated source code?

@kaljak

This comment has been minimized.

Show comment
Hide comment
@kaljak

kaljak Oct 17, 2016

Sure, also generated source code should be valid, right?

kaljak commented Oct 17, 2016

Sure, also generated source code should be valid, right?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Oct 17, 2016

Collaborator

Yes, arguably it should. Currently we set some expando properties directly on the elements, that should probably be stored in other places. Like isShadow and g.width in this case.

Collaborator

TorsteinHonsi commented Oct 17, 2016

Yes, arguably it should. Currently we set some expando properties directly on the elements, that should probably be stored in other places. Like isShadow and g.width in this case.

@ThomasOwens

This comment has been minimized.

Show comment
Hide comment
@ThomasOwens

ThomasOwens Dec 6, 2016

This may work with the Highcharts client-side exporter, but this invalid SVG doesn't appear to work with Batik, which is required for full support of exporting in older browsers. I'm experimenting with Highstock 5.0.5 and Batik 1.8 running on JRE 8u112 on CentOS 7.

First, I still need to use the patch from #3095 to handle changing rgba to rgb.

Second, g tags cannot have height and width and it appears that not only do they appear in the Highcharts examples, but I'm seeing at least two or three instances of them in my experimentation.

Is there a way I can perhaps modify the patch identified in 3095 to fix this? Either way, if Batik is the standard server for handling exporting, the data produced by Highcharts should be consumed by it.

ThomasOwens commented Dec 6, 2016

This may work with the Highcharts client-side exporter, but this invalid SVG doesn't appear to work with Batik, which is required for full support of exporting in older browsers. I'm experimenting with Highstock 5.0.5 and Batik 1.8 running on JRE 8u112 on CentOS 7.

First, I still need to use the patch from #3095 to handle changing rgba to rgb.

Second, g tags cannot have height and width and it appears that not only do they appear in the Highcharts examples, but I'm seeing at least two or three instances of them in my experimentation.

Is there a way I can perhaps modify the patch identified in 3095 to fix this? Either way, if Batik is the standard server for handling exporting, the data produced by Highcharts should be consumed by it.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 6, 2016

Collaborator

which is required for full support of exporting in older browsers

That shouldn't be the case... Our own server runs PhantomJS, and works with older browsers.

Either way, if Batik is the standard server for handling exporting

But it isn't. @gert our documentation gives the false impression that Batik is the preferred server: http://www.highcharts.com/docs/export-module/setting-up-the-server.

@ThomasOwens The node-export-server is still in Beta, but we're running it in production now. I recommend that you invest your time in this. We will update our docs as soon as we have verified its stability.

Collaborator

TorsteinHonsi commented Dec 6, 2016

which is required for full support of exporting in older browsers

That shouldn't be the case... Our own server runs PhantomJS, and works with older browsers.

Either way, if Batik is the standard server for handling exporting

But it isn't. @gert our documentation gives the false impression that Batik is the preferred server: http://www.highcharts.com/docs/export-module/setting-up-the-server.

@ThomasOwens The node-export-server is still in Beta, but we're running it in production now. I recommend that you invest your time in this. We will update our docs as soon as we have verified its stability.

@ThomasOwens

This comment has been minimized.

Show comment
Hide comment
@ThomasOwens

ThomasOwens Dec 6, 2016

That shouldn't be the case... Our own server runs PhantomJS, and works with older browsers.

I meant that the client-side exporter doesn't fully support older browsers. The documentation gives the impression that the preferred export server is the PHP script + Batik. Apparently that is incorrect.

I'll have to evaluate options at this point.

Going back to the original issue identified here, I believe that fixing these SVG compliance issues will make PHP+Batik work. It appears that it's no longer a supported option, but I don't see a reason why it shouldn't work. From what I can tell, it looks like the export servers that are tested against aren't fully compliant to SVG.

If the output isn't compliant SVG, this should be clearly documented somewhere. It also means that Batik will probably not work, so that should be removed from the docs as well.

Ideally, I'd like to see this issue resolved. If the client-side Highcharts/Highstock produce fully valid SVGs, then any tool to convert SVGs should be able to consume them.

ThomasOwens commented Dec 6, 2016

That shouldn't be the case... Our own server runs PhantomJS, and works with older browsers.

I meant that the client-side exporter doesn't fully support older browsers. The documentation gives the impression that the preferred export server is the PHP script + Batik. Apparently that is incorrect.

I'll have to evaluate options at this point.

Going back to the original issue identified here, I believe that fixing these SVG compliance issues will make PHP+Batik work. It appears that it's no longer a supported option, but I don't see a reason why it shouldn't work. From what I can tell, it looks like the export servers that are tested against aren't fully compliant to SVG.

If the output isn't compliant SVG, this should be clearly documented somewhere. It also means that Batik will probably not work, so that should be removed from the docs as well.

Ideally, I'd like to see this issue resolved. If the client-side Highcharts/Highstock produce fully valid SVGs, then any tool to convert SVGs should be able to consume them.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 7, 2016

Collaborator

Thanks for your patience!

Yes, we will update the docs.

And yes, we appreciate that this is a bug. We'll work on it immediately.

Collaborator

TorsteinHonsi commented Dec 7, 2016

Thanks for your patience!

Yes, we will update the docs.

And yes, we appreciate that this is a bug. We'll work on it immediately.

@TorsteinHonsi TorsteinHonsi added Bug and removed Undecided labels Dec 7, 2016

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 7, 2016

Collaborator

Demo with the fix applied:

Collaborator

TorsteinHonsi commented Dec 7, 2016

Demo with the fix applied:

@ThomasOwens

This comment has been minimized.

Show comment
Hide comment
@ThomasOwens

ThomasOwens Dec 7, 2016

Is this fix in 5.0.6? I don't see it in the change log. If not, is it slated for a versioned release coming up in the near future?

ThomasOwens commented Dec 7, 2016

Is this fix in 5.0.6? I don't see it in the change log. If not, is it slated for a versioned release coming up in the near future?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Dec 12, 2016

Collaborator

It will be part of the next maintenance release, typically released within a few weeks.

Collaborator

TorsteinHonsi commented Dec 12, 2016

It will be part of the next maintenance release, typically released within a few weeks.

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