Bug 143262 - Invalid media feature evaluates to true
Summary: Invalid media feature evaluates to true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 143266 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-03-31 06:21 PDT by Simon Pieters
Modified: 2016-03-11 03:13 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.84 KB, patch)
2015-03-31 09:54 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (254.59 KB, application/zip)
2015-03-31 10:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-mavericks (233.66 KB, application/zip)
2015-03-31 10:42 PDT, Build Bot
no flags Details
Patch (5.08 KB, patch)
2015-03-31 14:19 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (648.94 KB, application/zip)
2015-03-31 14:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (537.73 KB, application/zip)
2015-03-31 15:07 PDT, Build Bot
no flags Details
Patch (6.77 KB, patch)
2015-04-01 13:36 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (6.80 KB, patch)
2016-03-11 00:23 PST, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pieters 2015-03-31 06:21:16 PDT
See http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3473

<!DOCTYPE html>
<link rel=stylesheet href="data:text/css,p { color:red }" media="(width invalid)">
<p>There should be no red

The stylesheet is incorrectly applied.

Blink/Gecko/IE11 do not have this issue.

This is an issue for introducing comparison operators in media features, since WebKit will just evaluate them to true. e.g. (width < 500px)

https://drafts.csswg.org/mediaqueries/#mq-range-context

https://drafts.csswg.org/mediaqueries/#error-handling
Comment 1 Simon Pieters 2015-03-31 07:26:43 PDT
This does not happen for @media or matchMedia.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3474 passes
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3475 passes
Comment 2 Yoav Weiss 2015-03-31 09:51:31 PDT
*** Bug 143266 has been marked as a duplicate of this bug. ***
Comment 3 Yoav Weiss 2015-03-31 09:54:54 PDT
Created attachment 249827 [details]
Patch
Comment 4 Yoav Weiss 2015-03-31 09:56:03 PDT
When <link rel=stylesheet> with an invalid media attribute was part of the HTML, I saw that it resulted in a non-empty MediaQuerySet, containing a single query with no expressions. (which didn't have its m_ignored flag on)

Therefore I added an extra check that makes sure that the queries handled by MediaQueryEvaluator::eval have expressions, and if not, they are treated as ignored.
Comment 5 Build Bot 2015-03-31 10:23:10 PDT
Comment on attachment 249827 [details]
Patch

Attachment 249827 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6568812564447232

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2015-03-31 10:23:14 PDT
Created attachment 249829 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-03-31 10:42:42 PDT
Comment on attachment 249827 [details]
Patch

Attachment 249827 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6645182988550144

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2015-03-31 10:42:45 PDT
Created attachment 249833 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 9 Yoav Weiss 2015-03-31 14:19:53 PDT
Created attachment 249850 [details]
Patch
Comment 10 Build Bot 2015-03-31 14:44:03 PDT
Comment on attachment 249850 [details]
Patch

Attachment 249850 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6519012418650112

New failing tests:
fast/media/media-descriptor-syntax-05.html
Comment 11 Build Bot 2015-03-31 14:44:06 PDT
Created attachment 249855 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 12 Build Bot 2015-03-31 15:07:17 PDT
Comment on attachment 249850 [details]
Patch

Attachment 249850 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5765064195833856

New failing tests:
fast/media/media-descriptor-syntax-05.html
Comment 13 Build Bot 2015-03-31 15:07:22 PDT
Created attachment 249857 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 14 Yoav Weiss 2015-04-01 08:25:46 PDT
Hmm, the failing test is https://github.com/WebKit/webkit/blob/master/LayoutTests/fast/media/media-descriptor-syntax-05.html which is testing the opposite of this fix (basically making sure that this issue remains).

Simon - http://www.w3.org/TR/REC-html40/types.html#h-6.13 basically tells UAs to ignore values they don't understand, which would mean that invalid MQs match by default. Did that change at a later revision of the spec?
Comment 15 Simon Pieters 2015-04-01 09:54:58 PDT
I suppose the requirement in HTML4 explains WebKit's behavior here, but that is long obsolete.

https://html.spec.whatwg.org/multipage/semantics.html#attr-link-media
[[
However, if the link is an external resource link, then the media attribute is prescriptive. The user agent must apply the external resource when the media attribute's value matches the environment and the other relevant conditions apply, and must not apply it otherwise.
]]

->

https://html.spec.whatwg.org/multipage/infrastructure.html#matches-the-environment
[[
A string matches the environment of the user if it is the empty string, a string consisting of only space characters, or is a media query list that matches the user's environment according to the definitions given in the Media Queries specification. [MQ]
]]

->

https://drafts.csswg.org/mediaqueries/#error-handling
[[
A media query that does not match the grammar in the previous section must be replaced by not all during parsing.
]]
Comment 16 Yoav Weiss 2015-04-01 13:36:56 PDT
Created attachment 249939 [details]
Patch
Comment 17 Yoav Weiss 2015-04-15 23:34:19 PDT
ping
Comment 18 Brent Fulgham 2016-03-10 10:37:46 PST
Comment on attachment 249939 [details]
Patch

r=me. Please confirm tests pass before landing.
Comment 19 Yoav Weiss 2016-03-11 00:23:25 PST
Created attachment 273696 [details]
Patch
Comment 20 Yoav Weiss 2016-03-11 02:23:45 PST
Comment on attachment 273696 [details]
Patch

Thanks for reviewing!
Comment 21 WebKit Commit Bot 2016-03-11 03:13:15 PST
Comment on attachment 273696 [details]
Patch

Clearing flags on attachment: 273696

Committed r198004: <http://trac.webkit.org/changeset/198004>
Comment 22 WebKit Commit Bot 2016-03-11 03:13:22 PST
All reviewed patches have been landed.  Closing bug.