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

JS Tracking code should validate with the W3C validator #17281

Closed
Chardonneaur opened this issue Feb 27, 2021 · 9 comments · Fixed by #17685
Closed

JS Tracking code should validate with the W3C validator #17281

Chardonneaur opened this issue Feb 27, 2021 · 9 comments · Fixed by #17685
Assignees
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone

Comments

@Chardonneaur
Copy link

When using https://validator.w3.org the Matomo Tag Manager code snippet is flagged as a warning.
Solution: please change <script type="text/javascript"> into <script>

@Chardonneaur Chardonneaur added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label Feb 27, 2021
@Findus23 Findus23 added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels Feb 27, 2021
@Findus23
Copy link
Member

This is a fun "issue": In HTML5 javascript is the default for the <script> tag and therefore adding the type is redundand. The standard even says:

Authors should omit the attribute, instead of redundantly giving a JavaScript MIME type.

But Matomo can't know that the website you paste the tracking (or tag manager) code into is a HTML5 page accessed with a modern browser.
It could also be an ancient website only used by IE8 users and in this case I think adding the type is required for the browser to know what scripting language to use for it.

So it might be worth it to add redundand information in 99% of cases to avoid breaking things in very rare cases.

You can of course feel free to remove the type in your website as it isn't needed when you use HTML5.

@Findus23 Findus23 removed the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Feb 27, 2021
@tsteur tsteur added this to the Backlog (Help wanted) milestone Feb 28, 2021
@tsteur tsteur added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Feb 28, 2021
@tsteur
Copy link
Member

tsteur commented Feb 28, 2021

Thanks for this @Chardonneaur . @Findus23 @Chardonneaur not sure if we should keep the issue open or close it? We wouldn't remove it for now while we support older browsers so tempted to close the issue and re-open when we drop support for older browsers maybe (which is unlikely to happen in the next 2-3 years).

@oldrup
Copy link

oldrup commented May 15, 2021

My comment from in the matomo forums might actually belong here...

Internet Explorer 8 was released twelve years ago, and had end of life six years ago. Purely an opinion - but is it wise to keep pampering users of an obsolete browser no longer supported by its creator, at the cost of a (slightly, admittedly) less optimal experience for the 99% rest? I don’t think so.

Also, if using IE8, not supporting HTML5 or CSS3 properly, you have way more severe UX problems than missing out on getting your visit recorded by matomo.

Maybe IE browser support could be optional in matomo, just like do-not-track and IP anonymization?

As a site owner, using “modern” web techniques like CSS grid and custom properties, my sites support a vide range of modern browsers on many devices - but not IE - so it’s not that flipping that switch in matomo, serving recommended html, would not be the dealbreaker anyway. I’ve already “lost” my IE visitors.

Regarding "You can of course feel free to remove the type in your website as it isn't needed when you use HTML5." ... As far as I can see, that's not possible if fetching the tracking code via api - like the matomo integration plugin for WordPress, is it?

@tsteur
Copy link
Member

tsteur commented May 16, 2021

@oldrup do you mind letting us know why it's a problem when it's there? If I see this right in the initial issue it triggers a warning in w3c validator (not an error). Is that a problem? why?

FYI it's always possible to write a custom plugin to remove the type if that's absolutely necessary. Most users don't have an issue with the attribute being there so we'd like to prevent having too many options or settings when it works for the majority without issues. In the future this will change for sure and be great to understand why it's a problem.

@oldrup
Copy link

oldrup commented May 17, 2021

@tsteur

Thank you so much for your interest in this. I'll try to explain. It's more like a political issue, than a hard technical error, I'm totally aware of that, so please excuse me if this gets lengthy:

I aim to make sustainable, accessible, solid websites, that lives up to best practices and respects privacy. The latter is where matomo comes into play. Now, project owners, clients, customers, are not always easy to convince, that efforts should be made to live up to certain standards. They are often unaware of the concept of "code quality". They decide on a pretty design and settle with the tools they are used to (GA) and that's it.

Unless you can show them, not tell, that there is a better way, and it's neither hard nor expensive.

Take a look, at these two screenshots, showing the difference in test reports (in this case using wattspeed) before and after removing the redundant JavaScript MIME type:

Before: https://snipboard.io/mYdLTf.jpg
After: https://snipboard.io/xzAelZ.jpg

You and I both know that the redundant JavaScript MIME type, isn't doing any harm whatsoever. Still, there are good reasons that the latter result is preferred:

  • Project owners are much more likely to accept any ethic requirement (like WCAG and GDPR-compliance), when shown a case or prototype, testing green across the board. Having anything but zero errors, is not convincing to non-tech decision makers.
  • Telling clients or co-workers to ignore some "unimportant warnings", is a steep slippery slope, where they immediately decide to ignore every warning or error on the site.

Say what you want about these tests, but in my experience, with the warnings eliminated, I can tell decision makers "Yes - we will respect privacy. Yes - matomo is a viable alternative to GA. Yes - we this site will be accessible. No - you don't just have to take my word for it. It IS a better solution. Looks at these 3rd party test-results.".

And changing peoples habits, like using GA, is not easy. We need every good argument we can get.

We also both know, that there is so much more to creating great websites, than just to pass some synthetic tests. But it is definitely a good starting point.

A constructive afterthought. I appreciate that you want to keep the options in the settings dashboard as simple as possible. And that 99% of the users don't care about HTML validation. But how about making it an option in the matomo config file then? I have no problem setting a flag there, just like I did with the enforce-SSL flag until the official plugin was made.

All the best!
Bjarne

@tsteur tsteur modified the milestones: Backlog (Help wanted), 4.3.0 May 18, 2021
@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label May 18, 2021
@tsteur tsteur modified the milestones: 4.3.0, 4.4.0 May 18, 2021
@tsteur
Copy link
Member

tsteur commented May 18, 2021

@Findus23 @oldrup

I have just tested it without the attribute in IE8, IE7, IE6 and FF3 and it worked without any issues without the type attribute.

Note in IE7, IE6 and FF3 the tracking doesn't work because the JSON API is not defined and it needs to be polyfilled but the JS executes otherwise nicely. We removed out of the box tracking support for these.

I cannot think of any issue to remove this attribute right now and I reckon it's ok to go ahead with this. In worst case we could always release a patch release with the type attribute again should there be any specific reason why this is needed. We would ideally not have a config option for this.

We will need to mention this in the developer docs. Removing the attribute itself is easy but more of a task is updating all the test as also some tests in the plugins like A/B testing will break tests and needs to be updated later too. It shouldn't be big issue though.

@oldrup I have scheduled this work now and we're planning to make this change and thanks for explaining the background, very appreciated.

If there are any objections by anyone please comment.

@Findus23
Copy link
Member

If it works without, then I guess there is nothing wrong with removing the attribute.
I also saw that this discussion was already had a long time ago: #214

@tsteur
Copy link
Member

tsteur commented May 20, 2021

FYI already updating this in Matomo for WordPress in next release as the tracking code is generated differently there (and it won't break any other automated tests)

@oldrup
Copy link

oldrup commented May 20, 2021

That's great news. I will give a spin on a test site :) Thank you for the update!

@justinvelluppillai justinvelluppillai self-assigned this Jun 16, 2021
sgiehl added a commit that referenced this issue Jun 25, 2021
* Remove redundant type="text/javascript" from matomo tracking code, see #17281

* Fix system test expected output

* Fix more system tests' expected output

* Fix integration tests

* Fixed UI tests failure caused by removing type attribute from script tags

* Fix UI tests

* Fix lfs screenshot

* plugins/SegmentEditor/tests/UI/expected-screenshots/SegmentSelectorEditorTest_deleted.png: convert to Git LFS

* plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_permissions_bulk_access_set.png: convert to Git LFS

* tests/UI/expected-screenshots/UIIntegrationTest_invalid_idsite_superuser.png: convert to Git LFS

* tests/UI/expected-screenshots/EmptySite_emptySiteDashboard.png: convert to Git LFS

* tests/UI/expected-screenshots/UIIntegrationTest_admin_manage_tracking_code.png: convert to Git LFS

Co-authored-by: sgiehl <stefan@matomo.org>
@mattab mattab changed the title W3C validator code snippet JS Tracking code should validate the W3C validator Jul 26, 2021
@mattab mattab changed the title JS Tracking code should validate the W3C validator JS Tracking code should validate with the W3C validator Jul 26, 2021
@mattab mattab removed this from the 4.6.0 milestone Aug 25, 2021
@mattab mattab added this to the 4.5.0 milestone Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants