Skip to content

/ incubator-pagespeed-mod Public

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

Jump to bottom

Don't relocate scoped style tags #965

Open
GoogleCodeExporter opened this issue Apr 6, 2015 · 14 comments
Open

Don't relocate scoped style tags #965

GoogleCodeExporter opened this issue Apr 6, 2015 · 14 comments
Labels
auto-migrated Milestone-r32 Priority-High release-note Type-Defect

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

1. Open a page that is using <style scoped="scoped">..</style> with 
modpagespeed enabled (link below)
2. Open a page that is using <style scoped="scoped">..</style> with 
modpagespeed disabled (link below)
3. Compare. 

Due to the fact that style scoped applies its styles to and only it's children, 
the layout of the page is broken as modpagespeed is moving all style tags to 
the <head>..</head> section.

Solution: Do not relocate style scoped tags

Version: X-Mod-Pagespeed: 1.8.31.4-4009

Fedora 20
Server: Apache/2.4.9 (Fedora) OpenSSL/1.0.1e-fips mod_nss/2.4.6 NSS/3.15.3 
Basic ECC PHP/5.5.14 mod_wsgi/3.5 Python/2.7.5 mod_perl/2.0.9-dev Perl/v5.18.2

Compare these urls:
https://www.highdefinition.ch/kendo/listview/index.php
https://www.highdefinition.ch/kendo/listview/index.php?ModPagespeed=off

Original issue reported on code.google.com by frederic...@gmail.com on 3 Aug 2014 at 12:31
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Original comment by jmaes...@google.com on 3 Aug 2014 at 6:10

  • Changed title: Don't relocate scoped style tags
  • Changed state: Accepted
  • Added labels: Priority-High
  • Removed labels: Priority-Medium

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

This looks like the work of "move_css_to_head", which in general is considered 
a best practice for performance, but is not guaranteed to be free of side 
effects.

However, the page doesn't look broken to me in either of your links.  In any 
case you can turn that filter off in your pagespeed.conf and still use the 
other filters.

   ModPagespeedDisableFilters move_css_to_head

or just remove the line that enables that filter.  You can also preview this by 
viewing:

    https://www.highdefinition.ch/kendo/listview/index.php?PageSpeedFilters=-move_css_to_head

Can you describe in more detail what layout problem you noticed?

Original comment by jmara...@google.com on 3 Aug 2014 at 9:27

  • Changed state: RequestClarification

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

Right now scoped css is only supported in firefox: 
http://caniuse.com/style-scoped

If scoped css is going to be widely supported we need to not break sites that 
are using it.  Is it enough for move_css_to_head to skip any style=scoped 
blocks?  Are these pages the same in a browser that supports style=scoped?

Original:

  <html>
    <head>
      <style>* {color: red}</style>
    </head>
    <body>
      <div>
        Test
        <style scoped>* {color: green}</style>
      </div>
      More testing
      <style>* {color: purple}</style>
    </body>
  </html>

  (Displays "Test" in green and "More testing" in purple.)

After running move_css_to_head with it set to ignore <style scoped> blocks:

  <html>
    <head>
      <style>* {color: red}</style>
      <style>* {color: purple}</style>
    </head>
    <body>
      <div>
        Test
        <style scoped>* {color: green}</style>
      </div>
      More testing
      <style>* {color: purple}</style>
    </body>
  </html>

  (Should also display "Test" in green and "More testing" in purple.)

Original comment by jefftk@google.com on 4 Aug 2014 at 3:15

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

Here's an example page with that code: 
http://www.jefftk.com/scoped-css?PageSpeed=off

In Chrome it shows both "Test" and "More testing" in purple because scoped 
styles aren't enabled yet.  In FF it does the expected green/purple thing.

Original comment by jefftk@google.com on 4 Aug 2014 at 3:17

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Original comment by jefftk@google.com on 4 Aug 2014 at 3:17

  • Changed state: Accepted

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

Issue 625 has been merged into this issue.

Original comment by sligocki@google.com on 4 Aug 2014 at 6:46

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

Fixed for move_css_to_head and css_outline_filter in r4142 (which should appear 
in the next beta).  A fix for prioritize_critical_css will take longer as it 
requires a more invasive refactoring.  For the moment you should assume that 
scoped style blocks and prioritize_critical_css are incompatible with one 
another.

Original comment by jmaes...@google.com on 7 Aug 2014 at 3:21

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

[deleted comment]

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015 

Opened Issue 967 to track prioritize_critical_css specifically.  Closing this 
so we remember to document the fix to the other filters in the release notes.

Original comment by jmaes...@google.com on 7 Aug 2014 at 3:36

  • Added labels: Milestone-r32, release-note

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Aug 2, 2016

I think this is coming up again with WebComponents. They use scoped css, but without setting the scoped attribute: http://www.html5rocks.com/en/tutorials/webcomponents/shadowdom/

It looks like maybe <style> inside <template> should be treated like scoped?

@jeffkaufman jeffkaufman reopened this Aug 2, 2016
@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Aug 2, 2016

Talking to Polymer people, I think the problem is that our parser isn't <template>-aware at all. We should probably skip over <template> blocks entirely? I should read the spec for it and see what's ok for us to do.

@jmaessen
Copy link
Contributor

@jmaessen jmaessen commented Aug 2, 2016

My feeling is we should parse it, but be wary of it in much the same way as
we are wary of noscript blocks in the current system.

On Tue, Aug 2, 2016 at 3:24 PM, Jeff Kaufman notifications@github.com
wrote:

Talking to Polymer people, I think the problem is that our parser isn't
-aware at all. We should probably skip over blocks
entirely? I should read the spec for it and see what's ok for us to do.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#965 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAmZSNrs2TWuSBW8OLUSEtP1v52tibkuks5qb5l2gaJpZM4Ja7By
.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Aug 3, 2016

One problem with templates from our perspective is that they don't do anything in the form that pagespeed will see them, and then they are arbitrarily modified by js before they are actually on the page. So we may resolve urls incorrectly, or a url may be present that's intended to be rewritten to point somewhere else. Minification should be safe, though.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Aug 3, 2016

Template spec: https://html.spec.whatwg.org/multipage/scripting.html#the-template-element

@GoogleCodeExporter GoogleCodeExporter mentioned this issue Apr 6, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
auto-migrated Milestone-r32 Priority-High release-note Type-Defect
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@jeffkaufman @jmaessen @GoogleCodeExporter