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

Don't emit type=text/javascript on script tags #1207

Closed
GuillaumeRossolini opened this Issue Dec 9, 2015 · 12 comments

Comments

Projects
None yet
4 participants
@GuillaumeRossolini
Copy link

GuillaumeRossolini commented Dec 9, 2015

Hi,
I noticed mod_pagespeed includes a new "script" tag inside "head". Which is normal, but in this case the head already had a "script" tag:

<!DOCTYPE html><html xmlns="http://www.w3.org/1999/xhtml" xmlns:g="http://base.google.com/ns/1.0" xmlns:og="http://ogp.me/ns#" xmlns:fb="http://ogp.me/ns/fb#" xml:lang=en lang=en>
<head>
<script type='text/javascript'>window.mod_pagespeed_start = Number(new Date());</script><script>var FileAPI={

Maybe there could be a way to merge these tags? Although their declaration is not strictly identical, I have heard that for certain doctypes, the "type" attribute of the "script" tag is unnecessary. Maybe stripping this attribute could be another "less bytes" optimization for pagespeed, and in this case merging two tags as well?
Thanks,

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Dec 9, 2015

My 2p on merging script tags: when you merge script tags, you have to make sure that an exception that happens in code originating from one script block does not interfere with execution of merged code from another script block.
So I think you would have to add js code for that, which I suspect might offset the benefit of saving the script-open and script-close tags in the first place.

As for eliminating type attributes for script tags: the Elide Attributes filter will do that for you if you serve html 5. You can maybe try it on your site with ?PageSpeedFilters=+elide_attributes
See https://developers.google.com/speed/pagespeed/module/filter-attribute-elide

@GuillaumeRossolini

This comment has been minimized.

Copy link

GuillaumeRossolini commented Dec 10, 2015

Oh, you might be right about the exceptions.
As for the Elide filter, it does not seem to have much effect on pagespeed-issued tags: the output I gave you had Elide activated, as it is part of standard config in my case (see both lang="en" attributes from the declaration: they were shortened as lang=en).

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Dec 10, 2015

Perhaps the filters that inject those script tags run after the elide-attributes filter. I'm not sure if that is by design.. When I paste that html and run the elide_attributes filter on it, the type attribute does get elided. @jeffkaufman ?

@GuillaumeRossolini

This comment has been minimized.

Copy link

GuillaumeRossolini commented Dec 10, 2015

I tried enabling elide_attributes last to see what happens, but no luck.
Maybe you could make the order in which filters are added relevant?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Dec 10, 2015

merging tags: it doesn't save much in terms of bytes and it's pretty dangerous

removing the text/javascript tag on pagespeed scripts: why don't we just stop emitting this tag unless the pedantic filter is on?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Dec 10, 2015

Not emitting the type attribute unless pedantic is turned on (and we are serving a html 5 doc?) sounds good to me

@jeffkaufman jeffkaufman changed the title Additional "script" tag in head? Don't emit type=text/javascript on script tags Dec 10, 2015

@jeffkaufman

This comment has been minimized.

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Dec 10, 2015

Can we make that have a 100% accurate view of whether browsers will
interpret a doc as HTML5. I think this has come into question in the past.

On Thu, Dec 10, 2015 at 10:03 AM, Otto van der Schaaf <
notifications@github.com> wrote:

Not emitting the type attribute unless pedantic is turned on (and we are
serving a html 5 doc?) sounds good to me


Reply to this email directly or view it on GitHub
#1207 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Dec 10, 2015

We don't need to know if it's html5; leaving off type=text/javascript doesn't cause problems in browsers even with html4. It's just not (pedantically) spec until html5.

@GuillaumeRossolini

This comment has been minimized.

Copy link

GuillaumeRossolini commented Dec 17, 2015

Hi,
While you are at it, maybe you could also do away with the // counterpart?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Dec 17, 2015

We need that (only in Apache) because we don't know if the page is xhtml. If it is, then failing to include CDATA would break the page.

On the other hand, extremely few pages are xhtml these days, so it's possible that with an upcoming release we could require you to explicitly set something in your config if you wanted us to treat the page as xhtml.

@GuillaumeRossolini

This comment has been minimized.

Copy link

GuillaumeRossolini commented Dec 18, 2015

Yes, I meant with HTML5, and with the same exception rules you defined for type=text/javascript (nonpedantic). Or a config option would work too.

ashishk-1 added a commit to ashishk-1/mod_pagespeed that referenced this issue Jun 27, 2017

@oschaaf oschaaf closed this in bc43e45 Jun 30, 2017

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