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

type="text/javascript" is redundant #214

Closed
wants to merge 6 commits into from
Closed

type="text/javascript" is redundant #214

wants to merge 6 commits into from

Conversation

da2x
Copy link
Contributor

@da2x da2x commented Jan 28, 2014

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.

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

This is fully backwards compatible and compliant with HTML5.
@halfdan
Copy link
Member

halfdan commented Jan 28, 2014

👍

@da2x
Copy link
Contributor Author

da2x commented Jan 28, 2014

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

@da2x
Copy link
Contributor Author

da2x commented Jan 28, 2014

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

@halfdan
Copy link
Member

halfdan commented Jan 30, 2014

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

@da2x
Copy link
Contributor Author

da2x commented Jan 30, 2014

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

@robocoder
Copy link
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
Copy link
Contributor Author

da2x commented Jan 30, 2014

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 <script>window.alert(1)</script> 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
Copy link
Contributor Author

da2x commented Feb 1, 2014

Any unanswered concerns?

@mattab
Copy link
Member

mattab commented Feb 1, 2014

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
Copy link
Member

mattab commented Feb 3, 2014

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
Copy link

alexlehm commented Feb 5, 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
Copy link
Contributor Author

da2x commented Feb 5, 2014

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
Copy link

alexlehm commented Feb 5, 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
Copy link
Member

mattab commented Feb 5, 2014

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
Copy link

alexlehm commented Feb 5, 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
Copy link
Contributor Author

da2x commented Feb 5, 2014

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

@mattab
Copy link
Member

mattab commented Mar 26, 2014

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

@halfdan
Copy link
Member

halfdan commented Mar 26, 2014

👍

@mattab
Copy link
Member

mattab commented Apr 7, 2014

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

@da2x
Copy link
Contributor Author

da2x commented Apr 8, 2014

@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
Copy link
Member

mattab commented May 9, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants