@da2x opened this Pull Request on January 28th 2014 Contributor

Just a small optimization/modernization to move the web forward.

The <script> elements only need an explicit type when it is something
other than JavaScript.

This is fully backwards compatible and compliant with HTML5.

@halfdan commented on January 28th 2014 Member

:+1:

@da2x commented on January 28th 2014 Contributor

Pardon the sloppy work. Embarrassing. Thank you, Travis.

@da2x commented on January 28th 2014 Contributor

Travis failed do to an unrelated server error. Thought I believe it should be fine now.

@halfdan commented on January 30th 2014 Member

@Aeyoun I restarted the build - let's see how it turns out.

@da2x commented on January 30th 2014 Contributor

@halfdan, it failed again. I do not think it is related to these changes, though. https://travis-ci.org/piwik/piwik/builds/17798758

@robocoder commented on January 30th 2014 Contributor

Please revert the change to piwik.js and js/piwik.js. Removing type="text/javascript" is ok for the UI because it's designed for html5. But the tracker aims for cross-browser compatibility, including older browser versions.

@da2x commented on January 30th 2014 Contributor

TL;DR; if the type attribute is absent, the script is treated as JavaScript by every legacy browser. type is no longer required so no reason to keep it around.

@robocoder, this change will not create any compatibility issues. Every browser who ever supported JavaScript will support <script> without a type. All browsers expect this to be JavaScript and will treat it as such.

Including such marvelous legacy browsers as Firefox 2 and IE 5 (could not get a hold of even older IEs to test). This modernization really does not have any compatibility issues. It just makes the script more light-weight.

Including it is an old habit, however, it is time for it to die.

It is easy to test this. I checked this myself by running through browsershots.org. Every single legacy and modern browser showed up with an alert dialog.

http://www.w3.org/html/wg/drafts/html/master/scripting-1.html#the-script-element

@da2x commented on February 1st 2014 Contributor

Any unanswered concerns?

@mattab commented on February 1st 2014 Owner

I'll try to find time next week to try it and will merge, as long as there is no other concern by anybody?

@mattab commented on February 3rd 2014 Owner

Thanks for the PR!

Feedback

  • In general, i'd rather we don't change the piwik.js tracking code. GA is still using text/javascript so maybe they have a good reason for it? I'd like to take your word for granted that it just works without it, but i'm lacking experience with this.
    • revert chagne in js/piwik.js and piwik.js
    • also in misc/proxy-hide-piwik-url/README.md
    • and in plugins/CoreAdminHome/javascripts/jsTrackingGenerator.js
    • and plugins/Overlay/client/client.js
    • and plugins/Zeitgeist/templates/_piwikTag.twig plugins/Zeitgeist/templates/javascriptCode.tpl
  • Other changes look good!

Hopefully you can update the pR with these changes and then I'll merge it!

@alexlehm commented on February 5th 2014

If google analytics and google tag manager are anything to go by, they have removed text/javascript from the tags, even though it may be correct to do so, most some will still complain about that. Most major sites still use text/javascript at least in some parts (e.g. yahoo.com is quite inconsistent about that).

I'm not sure if this can be easily done, but the best way to deprecate the old version would be to have checkbox that turns off the text/javascript attribute in the tag and in the contruction of the script element. (Assume that not everybody will agree on the topic supporting a choice is probably best)

@da2x commented on February 5th 2014 Contributor

I’ll leave this open for discussion for a little longer before acting on @mattab‘s suggestion.

If anyone want to do independent compatibility testing, that would be very welcome.

@alexlehm commented on February 5th 2014

Reading up on the topic, your change is not compliant with XHTML and html 4, where the type attribute is a mandatory element. While I am certainly with you when you want to modernize the web, changing this and breaking validation of pages that are not strictly html5, is not a good idea.

@mattab commented on February 5th 2014 Owner

If google analytics and google tag manager are anything to go by, they have removed text/javascript from the tags,

I specifically checked and I can see the google analytics tag given in the "tracking code" section still has the type= attribute... However the new Universal analytics code does not have the type= attribute. I guess therefore that it's OK to not put it, but as I said I'm not keen to change this without testing more... Btw we have javascript tests at piwik/tests/javascript/ so these could maybe run in browsershots to check the tests still pass

@alexlehm commented on February 5th 2014

Sorry, I checked my analytics account, but apparently I upgraded all sites to universal already, the ga code likely uses type="text/javascript"

w3c validation differs between html4 and 5:

valid: http://validator.w3.org/check?uri=http%3A%2F%2Fwww.lehmann.cx%2Fpiwik-test%2Fhtml5.html
not valid: http://validator.w3.org/check?uri=http%3A%2F%2Fwww.lehmann.cx%2Fpiwik-test%2Fhtml4.html

@da2x commented on February 5th 2014 Contributor

Validation is a means not a goal. Compatibility should be the goal.
My goals were modernization and shrinking the tracking code (performance).

@mattab commented on March 26th 2014 Owner

What is our decision regarding this PR? is anyone against it? if not let's just do it... :+1:

@halfdan commented on March 26th 2014 Member

:+1:

@mattab commented on April 7th 2014 Owner

@Aeyoun could you please update the pull request so I can merge it with latest git? thanks in advance.

@da2x commented on April 8th 2014 Contributor

@mattab, I can do that in two weeks time unless someone wants to do it first. (I do not have time for it before then.)

@mattab commented on May 9th 2014 Owner

@Aeyoun sounds good! please re-base + re-open the pull request if/when you can.

This Pull Request was closed on May 9th 2014
Powered by GitHub Issue Mirror