Bug 97006

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:

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	Flags
Patch 1	kenneth: review-, kenneth: commit-queue-
Patch 2	kenneth: review+, buildbot: commit-queue-
Patch 3	none

Alexander Shalamov

Reported 2012-09-18 06:09:16 PDT

Invalid values for media query features are not handled correctly. For example, negative values for width, height, color, min-*, max-* features, treated as valid. Same applies for missing attributes for orientation and other features that must have value.

Attachments
Patch 1 (53.97 KB, patch) 2012-10-10 04:40 PDT, Alexander Shalamov	kenneth: review- kenneth: commit-queue-	Details Formatted Diff Diff
Patch 2 (56.03 KB, patch) 2012-10-10 11:17 PDT, Alexander Shalamov	kenneth: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Patch 3 (56.06 KB, patch) 2012-10-10 13:31 PDT, Alexander Shalamov	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alexander Shalamov

Comment 1 2012-10-10 04:40:41 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

Kenneth Rohde Christiansen

Comment 2 2012-10-10 05:19:44 PDT

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 ?

Alexander Shalamov

Comment 3 2012-10-10 05:45:23 PDT

(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!

Kenneth Rohde Christiansen

Comment 4 2012-10-10 05:54:29 PDT

(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. ??

Alexander Shalamov

Comment 5 2012-10-10 06:11:09 PDT

> > > 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.

Kenneth Rohde Christiansen

Comment 6 2012-10-10 06:17:06 PDT

(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.

Kenneth Rohde Christiansen

Comment 7 2012-10-10 06:28:27 PDT

(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?

Alexander Shalamov

Comment 8 2012-10-10 06:44:28 PDT

(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.

Kenneth Rohde Christiansen

Comment 9 2012-10-10 07:09:16 PDT

(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.

Alexander Shalamov

Comment 10 2012-10-10 11:17:58 PDT

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

Build Bot

Comment 11 2012-10-10 11:56:25 PDT

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

Alexander Shalamov

Comment 12 2012-10-10 13:31:44 PDT

Created attachment 168064 [details] Patch 3 - Fixed comments in fast/media/media-query-invalid-value.html

WebKit Review Bot

Comment 13 2012-10-10 19:07:26 PDT

Comment on attachment 168064 [details] Patch 3 Clearing flags on attachment: 168064 Committed r130995: <http://trac.webkit.org/changeset/130995>

WebKit Review Bot

Comment 14 2012-10-10 19:07:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.