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

Description Alexander Shalamov 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.
Comment 1 Alexander Shalamov 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
Comment 2 Kenneth Rohde Christiansen 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 ?
Comment 3 Alexander Shalamov 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!
Comment 4 Kenneth Rohde Christiansen 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. ??
Comment 5 Alexander Shalamov 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.
Comment 6 Kenneth Rohde Christiansen 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.
Comment 7 Kenneth Rohde Christiansen 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?
Comment 8 Alexander Shalamov 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.
Comment 9 Kenneth Rohde Christiansen 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.
Comment 10 Alexander Shalamov 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
Comment 11 Build Bot 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
Comment 12 Alexander Shalamov 2012-10-10 13:31:44 PDT 
Created attachment 168064 [details]
Patch 3

- Fixed comments in fast/media/media-query-invalid-value.html
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2012-10-10 19:07:30 PDT 
All reviewed patches have been landed.  Closing bug.