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

Test pass conditions wrong #1

Open
zcorpan opened this issue Dec 9, 2014 · 7 comments
Open

Test pass conditions wrong #1

zcorpan opened this issue Dec 9, 2014 · 7 comments

Comments

@zcorpan
Copy link

zcorpan commented Dec 9, 2014

Some tests don't have the same pass condition as the original test. e.g. the CSS comment test. Is that intentional? How did you decide on the expected value?

@albell
Copy link
Owner

albell commented Dec 9, 2014

TBH a handful of your tests I could just not wrap my head around, and that's probably my own shortcoming. I've also added a bunch of new tests, reorganized them into readable thematic groups, etc.

Just for starters, in the fifth reference test, I don't understand why <img srcset='data:,a 50w, data:,b 51w' sizes='1px'> would result in data:;b being selected. If the image is only going to be displayed a pixel wide, why would the wider of the two sources be selected?

Are CSS-style comments supposed to be ignored/stripped? Before or after splitting on commas?

Also, zero (and +0 and -0) are valid non-negative css lengths, correct? I could go on and on. There's a lot that doesn't make sense to me in these tests.

I'm trying to avoid the code weight of a full-blown CSS parser while still doing a decent job at this. If you could point out the what I've bungled and explain a bit about what I'm misunderstanding I would be extremely grateful.

@zcorpan
Copy link
Author

zcorpan commented Dec 10, 2014

The imgs on lines 20 to 119 are all supposed to result in the same selection as the img on line 19, i.e. expected result of parsing sizes should be something close to 1px.

Similarly, the imgs on lines 123 onwards are supposed to result in the same selection as the img on line 122, i.e. expected result of parsing sizes should be 100vw.

Just for starters, in the fifth reference test,

That one would result in data:,a being selected. (Unless the UA opts to always selecting the biggest candidate, which it is allowed to do technically but unlikely to with default configuration at least.)

Are CSS-style comments supposed to be ignored/stripped? Before or after splitting on commas?

Yes. Before. You're also supposed to tokenize into component values before splitting on commas, but I understand you don't want to do that. Given that, I would not bother doing anything about CSS comments either, and just say in documentation that you don't support things like CSS comments or CSS escapes, and have wrong behavior for invalid constructs like matching (), [] and {} per CSS rules. If you change your mind about using a CSS parser, I recommend https://github.com/tabatkins/parse-css

Also, zero (and +0 and -0) are valid non-negative css lengths, correct?

Yes.

HTH,

@albell
Copy link
Owner

albell commented Dec 11, 2014

Thank you. Weirdly, in Chrome Canary, line 19 is actually selecting data:,b. This was a stumbling block for me. I imagine there's some strange heuristics going on here.

I've refactored to handle CSS-style comments correctly, which on reflection I think is important because handwritten markup might include helpful comments on breakpoints.

Can you explain a bit about how compound media conditions are supposed to be handled? I'm having trouble unpacking this. E.g. is this test correct:

{sizes: '(min-width:0) or (min-width:0) 1px', expect: '100vw'} // ???

Or does a compound construct needs outer parens to pass?

{sizes: '((min-width:0) or (min-width:0)) 1px', expect: '1px'} // ???

@zcorpan
Copy link
Author

zcorpan commented Dec 12, 2014

Ah, the behavior in Canary is explained by it selecting the biggest image that is already available in cache (or as a data: URL). I guess I need to use a regular URL for these tests and use unique URLs for each one to avoid cache affecting the selection. Thanks for catching this.

(min-width:0) or (min-width:0) is a valid media condition. Wrapping in parens is also valid.

http://dev.w3.org/csswg/mediaqueries-4/#typedef-media-condition

@albell
Copy link
Owner

albell commented Dec 13, 2014

Thanks, yes sounds good.

Regarding compound queries, in both FF Aurora and Chrome Canary:

window.matchMedia("(min-width:0)").matches; // returns true, as expected.
window.matchMedia("(min-width:1px)").matches; // returns true, as expected.
window.matchMedia("(min-width:0) or (min-width:0)").matches; // returns false ?!
window.matchMedia("(min-width:1px) or (min-width:1px)").matches; // returns false ?!

See: http://jsfiddle.net/9hqwmtdy/1/

Note that for the last two the media property on the object is "not all", meaning there was a parse error:

http://dev.w3.org/csswg/mediaqueries4/#error-handling

But it seems to be valid based on the spec. I don't see any error. Why does this work in stylesheets but not in matchMedia? Am I just doing this the wrong way? Should I be testing the component conditions atomically with my own logic? Or is it that browsers haven't implemented MQ level 4 yet, and this is a bug?

@zcorpan
Copy link
Author

zcorpan commented Dec 15, 2014

Or is it that browsers haven't implemented MQ level 4 yet, and this is a bug?

Yeah. :( I think Blink is somewhere in-between L3 and L4. @yoavweiss

@yoavweiss
Copy link

Blink's MQ parser, which is used by sizes, is based on L4 grammar (so, e.g. not (max-width: 600px) is a valid MQ), but doesn't include any L4 features (or, <=, etc).

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

No branches or pull requests

3 participants