Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Consider a non-comment way to detect media query #12

Closed
edwardoriordan opened this Issue · 65 comments

9 participants

@edwardoriordan

It would be great to have a non-comment way to detect media queries because

  • They will get stripped out when minifying.
  • It is difficult to correctly place them after the query with no line break when using compass.
@scottjehl
Owner

I agree that'd be ideal; the comments sure save weight in the JS logic though.

I'm currently discussing potential scenarios where matching two }'s in a row (allowing for whitespace and comments in between them) will fail. I've got a regexp locally that'll match my examples just fine without the comments, but I'd want to build a larger test suite before rolling something like this out as the current test page is pretty limited. The question is, what sort of {'s can appear within a media query? @keyframes, @font-face, etc.

I'll give it some thought.

Your second bullet seems easy enough to fix though! I'll make a new bug for it

@edwardoriordan

I figured that there was a good reason why it was implemented that way. Thanks for letting me know the status. Good luck with a great project.

@pyrsmk

But, just a matter of interest, why should you put comments to find @media? Can't you just find @media?

@scottjehl
Owner

The comments allow for a lightweight implementation. Without the comments, the script would need to implement more parsing logic to detect the end of a media query. Simply detecting two }'s in a row won't be enough, as several CSS features use braces.

@pyrsmk

You're right, I didn't thought about that ^^

Good luck with this new implementation, if you need testers I could help you ;)

@gillesv

Could you perhaps adapt the regular expression used to select the comment to this one:

/@media ([^{]+){([\S\s]+?)(?=}\s\/*\/mediaquery*\/)/gmi

This will also work if the mediaquery is written as:

@media screen{
...
}
//mediaquery/

... which is what LESS (http://lesscss.org/) compiles to when you disable minifying. The extra linebreak between the closing bracket and the comment breaks your existing implementation. This regex should solve that.

@chendrix

I'm also pushing for @gillesv said, as I use LESS and was hoping for Respond support.

@pyrsmk

Just to explain a bit, the LESS compiled code is the same as Sass. And right, to rectify @gillesv, the compiled comment is /*/mediaquery*/.

@gillesv @chendrix: the only problem with that is that the comment will be dropped when minifying, so we can't keep this regexp as is, but it can be a nice ephemeral fix...

@scottjehl
Owner

yeah you'll need to ensure certain comments aren't stripped. Most minifiers have ways to specify comments to exclude from minification, but I'm not sure about LESS/Sass.

@pyrsmk

It doesn't exists for Sass, we can just remove all the comments or none :s

@chendrix

I just opened an issue in less asking to preserve the no-line-break between closing braces and comments, as well as selective non-minify, but I doubt much will come out of it.

@scottjehl
Owner

@chendrix: the line break shouldn't cause a problem now at least... if you grab latest. Selective non-min is critical though...

@rupl

I would be in favor of a non-comment approach. Minifying CSS is a standard practice for production sites, and no one will stop doing that. Instead of altering each minify process to make an exception (LESS, Sass, Drupal etc), a more globally effective solution is to make respond.js marginally smarter. In my opinion, the performance loss is worth the broader application.

A possible solution is to create a Respond "plugin" that could implement the non-comment approach. That would keep the core Respond code clean/fast, and Respond would only use the plugin if it's required for a particular situation.

@pyrsmk

I'm agree with you but I'm not sure that will be faster. Using a plugin, so write more code, will, generally, slow down performances.

On the other side, a logical implementation of the non-comment approach would be a parser which cut up the brackets to create a token tree. And, with the right functions, we can greatly divide the plugin integration performance cost. I don't know what about js, but I have developed a template engine, in php, with the aim of rapidity. I noticed the preg_split function has nearly the same benchmark as the "strpos implementation". Perhaps there are other possible optimizations.

@scottjehl
Owner

Good feedback, thanks.

I'm certainly not advocating that anyone stop minifying their CSS. :) Minifiers like YUI allow you to pass exceptions for comments that should be preserved, so that's what I was referring to. This sort of thing is often used in JS too, for preserving portions of scripts that use conditional compilation (however improper that may be).

Anyway, I will experiment with a couple approaches to this problem when I get a chance.

Thx again!

@pyrsmk

@scottjehl: But sass doesn't support excludes, as less maybe.

Anyway, good luck :)

@scottjehl
Owner

Quick update: @rupl and @pifantastic are working on a a plugin for respond.js that parses without comments. I believe it's a bit heavier, naturally, but please check it out and see if it's working for you.

I'm interested in pulling something in eventually, at least as a plugin.
https://github.com/rupl/Respond

Thanks!

@shichuan

To use /*/mediaquery*/ is indeed an issue, currently, it gets stripped out by the boilerplate build which is based on YUI CSS Compressor that uses ! as an option to prevent stripping. An alternative option is to allow user to define the match option when init respond.js or something?

@scottjehl
Owner

@shichuan that sounds like a nice intermediate step. Good idea.

@pyrsmk

@scottjehl @rupl

Works nice in IE8 and not at all in IE7. Since I saw "Fuck IE7" in the commit comment, I think it was expected :D

@pifantastic

@pyrsmk The "Fuck IE7" comment was when I discovered that you can't address specific indexes of a string in IE <= 7 (d'oh!). I replaced all usages of str[] with str.charAt(). This is to say, it should work in IE7. At least it does in my testing. I would be interested to see what case in which it is failing for you.

@pyrsmk

@pifantastic Hu ok... So bad for the str[], it's very useful... To let you see, that's the site which fails with IE7: http://home.dreamysource.fr/dreamysource.fr

@pifantastic

Sweet, thanks, I'll check it out.

@pifantastic

@pyrsmk One point I will make is that you should load the parser before respond. That way respond knows about the parser and can use it for the initial page load. Aside from that, I'm still looking into why it isn't working in IE7.

@pyrsmk

Indeed, invert both lines make all works ;)

@pifantastic

@pyrsmk Awesome! Thanks for taking the time to test it out.

@pyrsmk

That was a pleasure, I've many interests in Respond :)

@scottjehl
Owner

so, @pifantasic: I'd like to see about officially integrating this into Respond.js, perhaps even doing away with the comment-based approach if we can keep things lightweight.

Want to work with me on this idea?

@scottjehl
Owner

I did some more testing on the parser branch on a local project and unfortunately, I'm seeing some not-great performance. It works okay in the simple unit tests, but on a more real-world site, I'm seeing up to 4 seconds delay before the layout snaps in. I'm not sure walking the file will be the fastest approach, but it's great to have a working option for those who need it :)

Would anyone here have time to test the src from this branch in their own project? I'm seeing all but one unit test fail, so it seems viable (though it'll need a bit more logic before it's ready to handle all bracket scenarios).

I'm working on modifying the regexp so that it doesn't need comment tokens. It'd be nice to get a gauge for whether it's working yet in live projects. Thanks!
https://github.com/scottjehl/Respond/tree/comment-free

@pyrsmk

Walking could be greedy... Maybe a String.split(regexp) could be faster, much than a regexp match (at least in PHP, but I think it would be the same in js). I could make a benchmark to see what is fastest if you want.

I've tested the comment-free Respond.js and IE6/7/8 just hang and crash :s

@pifantastic

A string.split to boostrap the parsing is a really good idea. I've been planning to implement something like that when I have time. The only complication that arises is the edge case of ending up in the middle of a comment or string that contains "@media".

@scottjehl
Owner

@pyrsmk: it'd be useful to see the css you're using. I'm not seeing crashes when running the unit tests or a local client project (which is quite complex). Maybe you're using a combination of CSS rules I haven't accounted for.
Thanks

@scottjehl
Owner

btw - you may not have time for this one, but if you check out the comment-free branch and run the unit tests on a web server, are you seeing crashing there too, or just in your project?

@pyrsmk

@pifantastic: It will be very rare to have content:"blabla@mediablabla"; and we can advise users that don't use @media in a comment. It's restrictive but it will work as fast as possible. Or else, you can build your regexp to match @media type[,type[,...]](params){, but it will be slower.

@scottjehl: Also, I made my tests with IETester (to have IE5.5 to IE10 directly). It's not really stable. But it worked with the previous parser version. Here's the file: http://pastie.org/1968274 . It's quite big, but there's also a base.css ^^

I runned tests with test.html but not the unit testing since it needs to run in a popup and IETester just opens me tabs ^^ With test.html, media queries don't work at all with IE6/7/8.

@thulstrup

Using @font-face makes IE crash for me every time.
@pyrsmk: Could you try testing without the @font-face rules in your CSS?

@scottjehl
Owner

Good idea, @thulstrup.

@pyrsmk: Does your page crash without the @font-face rules? That'd be good to know...

@thulstrup

Apparently IE doesn't like having a @font-face declaration inside a @media selector. Once I moved it outside the selector, IE started behaving again.

So it doesn't look like it's the same problem as @pyrsmk after all...

@pyrsmk

Bingo! Without @font-face rules, my website works again ;)

@scottjehl
Owner

hmm ok. So to clarify, you're using the comment-free branch, yes?

Interesting: "At-rules inside @media are invalid in CSS2.1."
http://www.w3.org/TR/CSS21/media.html#at-media-rule

Not sure if css3 is so strict.

@pyrsmk

Yes, I am :)

@scottjehl
Owner

Good to hear.
Well, if anyone's interested in helping, I'd really like to work out some of the edge cases for the code in the "comment-free" branch.

The code in question will be line 76 https://github.com/scottjehl/Respond/blob/comment-free/respond.src.js#L76

@pyrsmk

In fact, you can't extract media queries with a js regexp. You'll need regex recursion and it's not supported in javacsript. For comment-free support you must match with regexes and walk a bit to find the good closing bracket, like:

var i=actual_position;
var nests=0;
var position;
do{
    if(contents[i]=='{'){
        ++nests;
    }
    if(contents[i]=='}'){
        if(--nests==0){
            // found!
            position=i;
            break;
        }
    }
}
while(++i);
if(position!==undefined){
    // it's right, it's found!
}

It's just an example but you'll see the matter.

Note: do-while loops and pre-(in)(de)crementation are known as faster :)

@pyrsmk

This, coupled with a contents.split(/@media[^{]+\{/gmi) will do the perfect stuff.

@scottjehl
Owner

Anxious to see an attempt at this. If anyone is able to work it out as a first pass, starting from the current code in master would be ideal.

Thanks everyone!

@pyrsmk

I currently work on the parsing implementation (but not with the Respond src, to keep it simple). I'll give you some news later ;)

@scottjehl
Owner

thanks!

@pyrsmk

Done.

Here's the wide test: http://home.dreamysource.fr/dreamysource.fr

The script is at the bottom of the page. Sheethub is the CSS API I developed (if you remember). Consolelog is a wrapper to have a log() function through browsers. All the results are in the console :)

@scottjehl
Owner

Thanks pyrsmk. So, I played with this a little bit, and while it does seem to match the correct bracket, I'm not quite understanding the difference between this result and the one provided by the following regexp:

styles.match( /@media ([^\{]+)\{((?!@media)[\s\S])*(?=\}(?![^\{]*\}))/gmi )

Seems to me that this does meet our needs: match all text after each @media statement that doesn't contain "@media" and is followed by a } that isn't followed by another } (forgiving whitespace in between).

If we were to take the following css:
@media only all { body { background: red; } }

The regexp above would match this:
@media only all { body { background: red; }

Which matches the comment-based match in master (the logic that follows in respond.js expects the final closing bracket to be excluded).

You may be seeing something that I'm missing here, though. Can you point out for me some situations where this would not meet our needs (aside from other bracket nesting scenarios we have not yet covered in either approach)?

Thanks!

@pyrsmk

I'm not certain that a regexp is really strong for that stuff. Indeed, your regexp matched @font-face :) But you're right, matching a closing bracket after a closing bracket should be good.

Also, we could benchmark the two methods to watch what is faster. Logically, it's the split/walk method because there are much less code to compile.

@scottjehl
Owner

Okay, so I've improved the comment-free regexp. It's now passing all unit tests in IE 6-8 and it renders a client site of mine flawlessly as well, which happens to have rather uncommon media query complexity.

That said, I'll need some testing help. Please grab the minified src and see how it fairs in your codebases. https://github.com/scottjehl/Respond/blob/comment-free/respond.min.js

Thanks so much!

@scottjehl
Owner

@pyrsmk: I believe the font-face issue is some thing different (however important). A couple sandboxed tests below show no conflicts with my regexp and embedded @font-face. In other words, it's not matching font-face, but there does seem to be another issue you're running into. Examples:

Regular media query:

var styles = "@media only all { body { background: red; } }";
styles.match( /@media ([^\{]+)\{((?!@media)[\s\S])*(?=\}(?![^\{]*\}))/gmi );
--> ["@media only all { body { background: red; } "]

Same media query with @font-face inside:

var styles = '@media only all { body { background: red; @font-face { font-family: "test"; src: url("test.eot") format("eot"); } }';
styles.match( /@media ([^\{]+)\{((?!@media)[\s\S])*(?=\}(?![^\{]*\}))/gmi );
--> ["@media only all { body { background: red; @font-face { font-family: "test"; src: url("test.eot") format("eot"); } "]
@pyrsmk

Drop what I said :D I'd taken the bad regexp, so it matched my @font-faces... Normal ^^

So, your regexp works well. But, as you said, it's not efficient with nested @ rules. Moreover, I've made a benchmark on your actual regexp and my split/walk code. The last one is more than twice faster (3800ops/s against 1700ops/s). So there will no loss to implement that solution, and it works with infinite nests.

That said, the actual Respond version works with IE6/7 but IE8 still crashes with me (I point out that my tests are done with IETester, which is quite chilly).

@scottjehl
Owner

thanks @pyrsmk,

Okay, cool. I'd love to see your parser implemented into the Respond.js codebase for proper testing. Would you have time to put that together?

I think a good starting point would be to checkout the parser-integration branch and replace the internals of the parseMQs function with your own logic. If it's faster, that's great! It seems to be a small enough increase in file size that it wouldn't be a problem on that front.

I'm not sure I follow the nested @ rules part. What are you nesting exactly? It seems this regexp version would ignore any @rules that aren't @media - and @media rules aren't meant to be nested anyhow (afaik anyway!).

The IE8 crash is interesting still. I'm not seeing it in my tests so I'll need to check out your CSS again and see what's in there that could be throwing it.

Thanks again!

@pyrsmk

Currently, I've time for it, I'm going to try to put my code into the parser-integration branch.

I used "nested" term to mean "imbrication" or "node": an @font-face rule into a @media one is a child of it (I didn't mean an @media into another one :p).

@scottjehl
Owner

Hi all,
I have put together a jsperf test this morning to compare the regexp and loop parsing implementations and see which makes more sense to use for the Respond.js comment-free implementation.

The main goal is to simply test the speed of each method of media query isolation, so the actual queries being used are somewhat irrelevant as far as I can tell. The CSS passed to the parser is intentionally large to expose differences in performance. Some of the queries include a large amount of CSS to see how the number of enclosed rules affect the performance. There are more notes on this at the top.

I'd very much appreciate feedback on whether the test itself should be edited to cover situations I hadn't thought of.

It should be noted that the only browsers that really matter in this test are non-media-query-supporting browsers, such as IE6-8. Other browser results are in there purely for curiosity's sake.

Here's the test suite:
http://jsperf.com/respond-js-parser-regexp-vs-split-loop

@pyrsmk

I'm really surprised about the results. How can I have the opposite of yours Oo

Edit: I think it can't be relevant with IETester, but I've 2.5 for the first and 5.5 for the second in IE6/7.

@scottjehl
Owner

Hey Pyrsmk: thanks for checking.

The differences you see may be due to this http://jsperf.com/faq#browserscope

By the way, I want to emphasize that I really don't care which method is faster here. I just want to pick one that makes the most sense for a broad use case. I also think favoring IE 7 and 8 results over IE6 results may make sense, given their greater marketshare. I'm not sure if there will be a difference between those anyway.

Update: I just changed the CSS to use fewer rules per query, and added a few more queries. This aims to get IE 7 and 8 to run more iterations when testing.

Thanks for the help!

@pyrsmk

I'm agree with you Scott. Maybe the right question is: are imbricated rules really needed? I think the most revealing case would be @font-face rules inside media queries.

I've got an idea... Can't you test with split(/@media/gmi) instead of split('@media')? I'd seen a performance loss when I switch the regexp match for a string.

@scottjehl
Owner

Sure, I can test it with regexp vs string.

As for @font-face within queries, isn't that a separate issue? Either way, I've added a @font-face declaration within one of the queries to test if it affects anything.

@scottjehl
Owner

including suggested edits, new test is up http://jsperf.com/respond-js-parser-regexp-vs-split-loop/2

@scottjehl
Owner

@pyrsmk: I made an isolated test and it appears that IE7 and 8 will crash simply by dynamically appending a CSS rule containing @font-face. If you're able to confirm this, that'd be very helpful.

If this is the case, I'll have to include some info about this in the docs so people know not to embed font-face in media queries when using respond. (or any parser, I'd imagine)

Test page here:
http://jsfiddle.net/scottjehl/Ejyj5/1/

@scottjehl
Owner

btw - to clarify, all branches that parsed media queries as expected caused a crash in IE8. After some debugging, I found that it was during applyMedia where the crash occurs - right when the CSS is appended to the page.

@scottjehl
Owner

Confirmed by a few people: dynamically appending a style element that contains @font-face in its CSS will cause IE 7 & 8 to crash. Rules like @page are okay.

Seems the @font-face crash is unrelated to Respond.js, but something to warn about, and watch out for!

@pyrsmk

As I saw, the regexp version is faster than the string in IE... I think I will revert this :)

Does jsfiddle running automatically himself when loading the page? Because IE6/7/8 in IETester, crash directly :p

By the way, on my side, I've tested the @font-face rule insertion with Sheethub: there's no crash. Also, it doesn't use CSSStyleSheet's cssText method but the node's text property (for centralization). You can see the unit testing here: http://home.dreamysource.fr/sheethub/unit/

@scottjehl
Owner

Thanks pyrsmk, that's great info. I'll have to give that a shot for the @font-face thing.

@scottjehl scottjehl closed this issue from a commit
scottjehl Merging in the comment-free parser. Respond.js no longer needs CSS co…
…mment tokens to mark the end of a media query statement. Fixes #12.
77ac03c
@scottjehl scottjehl closed this in 77ac03c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.