Summary: | Invalid values for media query features are not handled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Shalamov <alexander.shalamov> | ||||||||
Component: | WebCore Misc. | Assignee: | Alexander Shalamov <alexander.shalamov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, eric.carlson, feature-media-reviews, kenneth, macpherson, menard, simon.fraser, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://www.w3.org/Style/CSS/Test/MediaQueries/20100726/media-queries-test.html | ||||||||||
Bug Depends on: | 96752 | ||||||||||
Bug Blocks: | 45017 | ||||||||||
Attachments: |
|
Description
Alexander Shalamov
2012-09-18 06:09:16 PDT
Created attachment 167979 [details]
Patch 1
- Imported w3c media query test suite (without parsing tests, would be added in another patch)
- Made media query expression stricter and compliant with the w3c specification
- Fixed few performance issues in media query evaluator, string comparison were used in few places instead of CSSValueIDs
- Fixed media query serialization for invalid queries
Comment on attachment 167979 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=167979&action=review > LayoutTests/fast/media/media-query-invalid-value.html:6 > <head> > <style type="text/css"> > - @media (min-width) { > + @media (min-width: 100px) { > div { background-color: green; } > } > @media (min-width: blah) { I don't get this. The test it supposed to fail right, as it is testing invalid values! @media (min-width: 100px) is valid! not invalid which @media (min-width) is! > LayoutTests/fast/media/media-query-serialization.html:5 > -<style type="text/css" media="NOT braille, tv and (orientation: 'landscape') AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)"> > +<style type="text/css" media="NOT braille, tv and (orientation: landscape) AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)"> Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please > Source/WebCore/ChangeLog:8 > + > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). OOPS! > Source/WebCore/css/CSSGrammar.y:538 > + // if restrictor is specified, ignore expression Comments should be proper sentences. ie. // If ... expression. > Source/WebCore/css/MediaQueryEvaluator.cpp:252 > + if (width > height) // Square viewport is portrait Dot at end > Source/WebCore/css/MediaQueryEvaluator.cpp:257 > + // orientation could be calculated // Orientation could be calculated. Is that a FIXME? I dont understand the comment > Source/WebCore/css/MediaQueryExp.cpp:70 > +static inline bool featureWithPositiveInteger(const AtomicString& mediaFeature) > +{ > + return mediaFeature == MediaFeatureNames::colorMediaFeature > + || mediaFeature == MediaFeatureNames::max_colorMediaFeature > + || mediaFeature == MediaFeatureNames::min_colorMediaFeature > + || mediaFeature == MediaFeatureNames::min_monochromeMediaFeature > + || mediaFeature == MediaFeatureNames::max_monochromeMediaFeature; > +} Why not static inline bool featureWithValidPositiveInteger(const AtomicString& mediaFeature) { if (!value->isInt || value->fValue < 0) return false; return mediaFeature == MediaFeatureNames::colorMediaFeature || mediaFeature == MediaFeatureNames::max_colorMediaFeature || mediaFeature == MediaFeatureNames::min_colorMediaFeature || mediaFeature == MediaFeatureNames::min_monochromeMediaFeature || mediaFeature == MediaFeatureNames::max_monochromeMediaFeature; } Also I guess it is cheaper to check the value first > Source/WebCore/css/MediaQueryExp.cpp:131 > + // Media features that use CSSValueIDs Punctuation mark at end > Source/WebCore/css/MediaQueryExp.cpp:136 > + else if (featureWithPositiveLenghtOrNumber(mediaFeature) && (((value->unit >= CSSPrimitiveValue::CSS_EMS && value->unit <= CSSPrimitiveValue::CSS_PC) || value->unit == CSSPrimitiveValue::CSS_REMS) || value->unit == CSSPrimitiveValue::CSS_NUMBER) && value->fValue >= 0) Why not move that checking to the other method, and rename it like featureWithValidPositiveLengthOrNumber ? (In reply to comment #2) > (From update of attachment 167979 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167979&action=review > > > LayoutTests/fast/media/media-query-invalid-value.html:6 > > <head> > > <style type="text/css"> > > - @media (min-width) { > > + @media (min-width: 100px) { > > div { background-color: green; } > > } > > @media (min-width: blah) { > > I don't get this. The test it supposed to fail right, as it is testing invalid values! @media (min-width: 100px) is valid! not invalid which @media (min-width) is! Test checks that div has green color. min-* media features MUST have value specified, therefore, old test had two invalid media expressions that do not match. > > LayoutTests/fast/media/media-query-serialization.html:5 > > -<style type="text/css" media="NOT braille, tv and (orientation: 'landscape') AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)"> > > +<style type="text/css" media="NOT braille, tv and (orientation: landscape) AND (min-WIDTH:100px) and (max-width: 200px ), all and (color) and (color)"> > > Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please http://www.w3.org/TR/css3-mediaqueries/#orientation (landscape | portrait) > > Source/WebCore/css/MediaQueryEvaluator.cpp:257 > > + // orientation could be calculated > > // Orientation could be calculated. > > Is that a FIXME? I dont understand the comment It is not. If no value is specified for media expression, e.g. (orientation) and frame has widht and height that are > 0, => there is orientation :) and it can be calculated if needed. > > Source/WebCore/css/MediaQueryExp.cpp:70 > > +static inline bool featureWithPositiveInteger(const AtomicString& mediaFeature) > > +{ > > + return mediaFeature == MediaFeatureNames::colorMediaFeature > > + || mediaFeature == MediaFeatureNames::max_colorMediaFeature > > + || mediaFeature == MediaFeatureNames::min_colorMediaFeature > > + || mediaFeature == MediaFeatureNames::min_monochromeMediaFeature > > + || mediaFeature == MediaFeatureNames::max_monochromeMediaFeature; > > +} > > Why not > > static inline bool featureWithValidPositiveInteger(const AtomicString& mediaFeature) > { > if (!value->isInt || value->fValue < 0) > return false; > > return mediaFeature == MediaFeatureNames::colorMediaFeature > || mediaFeature == MediaFeatureNames::max_colorMediaFeature > || mediaFeature == MediaFeatureNames::min_colorMediaFeature > || mediaFeature == MediaFeatureNames::min_monochromeMediaFeature > || mediaFeature == MediaFeatureNames::max_monochromeMediaFeature; > } > > Also I guess it is cheaper to check the value first Thanks. Will fix it. > > Source/WebCore/css/MediaQueryExp.cpp:131 > > + // Media features that use CSSValueIDs > > Punctuation mark at end > > > Source/WebCore/css/MediaQueryExp.cpp:136 > > + else if (featureWithPositiveLenghtOrNumber(mediaFeature) && (((value->unit >= CSSPrimitiveValue::CSS_EMS && value->unit <= CSSPrimitiveValue::CSS_PC) || value->unit == CSSPrimitiveValue::CSS_REMS) || value->unit == CSSPrimitiveValue::CSS_NUMBER) && value->fValue >= 0) > > Why not move that checking to the other method, and rename it like featureWithValidPositiveLengthOrNumber ? Good point, thanks! (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 167979 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=167979&action=review > > > > > LayoutTests/fast/media/media-query-invalid-value.html:6 > > > <head> > > > <style type="text/css"> > > > - @media (min-width) { > > > + @media (min-width: 100px) { > > > div { background-color: green; } > > > } > > > @media (min-width: blah) { > > > > I don't get this. The test it supposed to fail right, as it is testing invalid values! @media (min-width: 100px) is valid! not invalid which @media (min-width) is! > > Test checks that div has green color. min-* media features MUST have value specified, therefore, old test had two invalid media expressions that do not > match. I think you need to explain this better in the changelog, like maybe add a comment to the first // Valid media query, background color should remain green. @media (min-width: 0px) { div { bac.... } > > Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please > > http://www.w3.org/TR/css3-mediaqueries/#orientation (landscape | portrait) Yes, but what does the spec say to say "(width: 'blah')" That is just invalid right and ignored no? So I think (orientation: "blahhh") should be fine to serialize no? It just serializes to something without it, no? > > > > Source/WebCore/css/MediaQueryEvaluator.cpp:257 > > > + // orientation could be calculated > > > > // Orientation could be calculated. > > > > Is that a FIXME? I dont understand the comment > > It is not. If no value is specified for media expression, e.g. (orientation) > and frame has widht and height that are > 0, => there is orientation :) and it > can be calculated if needed. // Orientation can be calculated. ?? > > > Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please > > > > http://www.w3.org/TR/css3-mediaqueries/#orientation (landscape | portrait) > > Yes, but what does the spec say to say "(width: 'blah')" That is just invalid right and ignored no? So I think (orientation: "blahhh") should be fine to serialize no? It just serializes to something without it, no? width must have length value, 'blah' is not length and would be parsed as string, this makes media query expression invalid => media query is invalid. Invalid media query evaluates to false and serialized to "not all". http://dev.w3.org/csswg/cssom/#serialize-a-media-query > > > > Source/WebCore/css/MediaQueryEvaluator.cpp:257 > > > > + // orientation could be calculated > > > > > > // Orientation could be calculated. > > > > > > Is that a FIXME? I dont understand the comment > > > > It is not. If no value is specified for media expression, e.g. (orientation) > > and frame has widht and height that are > 0, => there is orientation :) and it > > can be calculated if needed. > > // Orientation can be calculated. ?? Sorry, I used wrong term. What I meant is that (orientation) should evaluate to true if frame's widht and height > 0. (In reply to comment #5) > > > > Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please > > > > > > http://www.w3.org/TR/css3-mediaqueries/#orientation (landscape | portrait) > > > > Yes, but what does the spec say to say "(width: 'blah')" That is just invalid right and ignored no? So I think (orientation: "blahhh") should be fine to serialize no? It just serializes to something without it, no? > > width must have length value, 'blah' is not length and would be parsed as string, this makes media query expression invalid => media query is invalid. Invalid media query evaluates to false and serialized to "not all". I am not sure it is a string as strings are only represented by using " according to the spec. I seem to remember something like ' should just be ignored or so. (In reply to comment #6) > (In reply to comment #5) > > > > > Are you sure 'landscape' is invalid? shouldn't it just not be false? spec link please > > > > > > > > http://www.w3.org/TR/css3-mediaqueries/#orientation (landscape | portrait) > > > > > > Yes, but what does the spec say to say "(width: 'blah')" That is just invalid right and ignored no? So I think (orientation: "blahhh") should be fine to serialize no? It just serializes to something without it, no? > > > > width must have length value, 'blah' is not length and would be parsed as string, this makes media query expression invalid => media query is invalid. Invalid media query evaluates to false and serialized to "not all". > > I am not sure it is a string as strings are only represented by using " according to the spec. I seem to remember something like ' should just be ignored or so. I cannot find this anywhere. Maybe it changed in the spec. Can we at least make a test with (orientation: 'landscape') to make sure we enforce this behavior, maybe even submit it as a W3C test case? (In reply to comment #6) > I am not sure it is a string as strings are only represented by using " according to the spec. I seem to remember something like ' should just be ignored or so. In CSSParser.cpp:9648 "case CharacterQuote:" If ' or " found, tokenizer would tell to parser that next token is string. I think that is correct behaviour. (In reply to comment #8) > (In reply to comment #6) > > I am not sure it is a string as strings are only represented by using " according to the spec. I seem to remember something like ' should just be ignored or so. > > In CSSParser.cpp:9648 "case CharacterQuote:" > > If ' or " found, tokenizer would tell to parser that next token is string. > I think that is correct behaviour. Yes, that is what I saw, so lets just make a test for it; or a couple. Created attachment 168041 [details] Patch 2 - Fixed Source/WebCore/ChangeLog. - Added comment to LayoutTests/fast/media/media-query-invalid-value.html - Fixed comment in Source/WebCore/css/CSSGrammar.y:538 - Fixed comment in Source/WebCore/css/MediaQueryEvaluator.cpp:252 - Fixed comment in Source/WebCore/css/MediaQueryEvaluator.cpp:257 - Refactored Source/WebCore/css/MediaQueryExp.cpp by moving checks from if() statements. - Added tests to fast/media/w3c/test_media_queries.html which verify that when string value is set to orientation or media feature that accepts length or number, media query becomes invalid and serialized to "not all" http://dev.w3.org/csswg/cssom/#serialize-a-media-query Comment on attachment 168041 [details] Patch 2 Attachment 168041 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14254258 New failing tests: fast/media/media-query-invalid-value.html Created attachment 168064 [details]
Patch 3
- Fixed comments in fast/media/media-query-invalid-value.html
Comment on attachment 168064 [details] Patch 3 Clearing flags on attachment: 168064 Committed r130995: <http://trac.webkit.org/changeset/130995> All reviewed patches have been landed. Closing bug. |