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

[mediaqueries] Non-positive value should be invalid for 'resolution' feature #1454

Closed
upsuper opened this issue May 23, 2017 · 16 comments
Closed
Assignees
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. mediaqueries-4 Current Work Tested Memory aid - issue has WPT tests Tracked in DoC

Comments

@upsuper
Copy link
Member

upsuper commented May 23, 2017

Zero and negative values for 'resolution' feature don't make any sense, and it seems all of Firefox, Chrome, and Edge already reject non-positive values for it. The spec should follow the impl and say so.

A simple testcase:

<!DOCTYPE html>
<style>
div {
  width: 100px; height: 100px;
  background: red;
  display: inline-block;
  margin: 10px;
}
@media (min-resolution: 1dpi) {
  #a { background: green; }
}
@media (min-resolution: 0dpi) {
  #b { background: green; }
}
@media (min-resolution: -1dpi) {
  #c { background: green; }
}
</style>
<div id="a"></div>
<div id="b"></div>
<div id="c"></div>
<pre>
<script>
let rules = document.styleSheets[0].cssRules;
for (var i = 1; i < rules.length; i++) {
  document.write(rules[i].media.mediaText + '\n');
}
</script>
</pre>
@upsuper upsuper added the mediaqueries-4 Current Work label May 23, 2017
@frivoal
Copy link
Collaborator

frivoal commented May 23, 2017

Banning negative numbers at parse time makes sense to me. Excluding 0 seems more problematic, as I think we're trying to avoid open ranges in CSS. In your sample above, if you put @media (resolution: 0.0000000000000000000000000000000000000000000001dpi) firefox will output not all while Chrome outputs (min-resolution: 1e-46dpi) (at least on my machine it does, not sure about other OSes, version...).

If Firefox had rounded to 0 when it reached its maximum precision instead of failing, the two queries would have been effectively equivalent (since they're a min- query).

@upsuper
Copy link
Member Author

upsuper commented May 23, 2017

I don't have strong opinion for zero. We have something elsewhere that we treat zero specifically (e.g. perspective) so it isn't really something new... but yeah, in general we probably want to avoid this kind of thing.

@tabatkins
Copy link
Member

As a general rule, banning zero from a <number> or <dimension> is disallowed; it's an open range https://wiki.csswg.org/spec/limited-ranges.

perspective()'s special zero behavior is a worst-case thing; the behavior impls settled on is nonsensical from a numerical perspective, not to mention totally discontinuous, and we're sticking with it only because content depends on it. If its behavior had been properly described originally, and someone had noticed it, it would have been changed in the design phase (almost certainly by specifying that there's a UA-specific minimum value). It is never an example to look to, and cannot be used to justify more bad behavior.

The correct behavior for this case is fairly similar, in that as the value approaches 0 the size of the pixel approaches infinity. The correct solution is thus to define that negative values are invalid, and that there's a UA-specific minimum value.

@frivoal
Copy link
Collaborator

frivoal commented May 24, 2017

and that there's a UA-specific minimum value.

Depending on what you mean by that, do we even need that part? (resolution: 0.000001dpi) will never match any actual device, any more than (resolution: 0dpi). On the other hand, every visual device will match both (resolution > 0dpi) and (resolution > 0.00001dpi).

We should:

  • forbid negative numbers
  • define that (resolution:0) does not matches on non visual devices (to avoid the discontinuity between actual 0, rounded to 0, and very-small-but-not-rounded-to-0 numbers on non visual devices)
  • (optionally) note that css-values allows for limited precision, and that rounding small numbers to 0 is ok.

bors-servo pushed a commit to servo/servo that referenced this issue May 25, 2017
style: Reject non-positive resolution values in media queries.

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17006)
<!-- Reviewable:end -->
@tabatkins
Copy link
Member

By "there's a UA specific minimum value", I mean that values between 0 and that minimum are rounded to the minimum, not that values in that range are invalid.

@upsuper
Copy link
Member Author

upsuper commented May 25, 2017

I... don't think this really needs a UA-specific minimum value, because I don't think anyone is going to use this number as a divisor. Impls just get a resolution from the system and compare it against the provided number, no?

I think directly making zero valid should be enough, unless we use these units for computation somewhere.

@frivoal
Copy link
Collaborator

frivoal commented May 25, 2017

By "there's a UA specific minimum value", I mean that values between 0 and that minimum are rounded to the minimum

I don't see how that's better than rounding to 0, so let's round to 0 when it's too small.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 25, 2017
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : ecf258a50bd00b636e4a21bad17e072db383c449
@tabatkins
Copy link
Member

A resolution of 0dpi is meaningless; it corresponds to an infinite dot size, which isn't physically possible. That's why I'm talking about a minimum value; it limits you to physically possible resolutions.

@tabatkins
Copy link
Member

Again, this implies that zero is a valid value for parsing purposes, it's just treated as some particular non-zero positive value.

@frivoal
Copy link
Collaborator

frivoal commented May 26, 2017

A resolution of 0dpi is meaningless; it corresponds to an infinite dot size, which isn't physically possible.

Neither is a device with pixels that are 375 miles across.

I don't see the practical benefit / difference. No device will have infinitely large pixels. No device will have 375 miles large pixels either. So none of resolution:0, resolution: something-tiny, max-resolution:0, or max-resolution: something-tiny will ever match anything. However, both min-resolution: 0 and min-resolution: something-tiny will match the same things (all the visual devices, for which a concept of resolution is meaningful).

So if the practical results are the same, why introduce extra logic in processing, when old fashioned rounding will do just fine?

@frivoal
Copy link
Collaborator

frivoal commented May 26, 2017

As far as I can tell, without any normative prose, this seems to be all we need on this topic of rounding / smallest values / 0.

Note: Since resolution:0 would imply infinitely large pixels, it will never match on any device (nor will max-resolution: 0). However, min-resolution:0 does match on all visual devices. Small values that are rounded to 0 due to limited precision in the implementation behave identically.

@frivoal
Copy link
Collaborator

frivoal commented May 26, 2017

On the other side of the discussion, I wonder whether we should actually let negative values parse. (resolution < -3dpi) should always be false. But if we made it not parse, it would be unknown instead of being false, as per the error handling section:

An unknown or , or disallowed , results in the value “unknown”.

Either way

@media (resolution: -1dpi) { body { background: red; } }

would fail to give a red background on any device, but

@media not (resolution: -1dpi) { body { background: green; } }

could very reasonably give you a green body on all visual devices. And while we're at it, so could

@media (resolution > -2dpi ) { body { background: green; } }

I don't think there's much of a use case for specifying such nonesense resolutions manually, but they could fall out of calc() or some server side math, and still be usable in range contexts.

If we clarify that this works, we should apply the same logic to other range properties with nonsensical negative values, like width

aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue May 26, 2017
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue May 27, 2017
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue May 30, 2017
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512
@frivoal frivoal added the Agenda+ label Jun 6, 2017
@frivoal frivoal self-assigned this Jun 6, 2017
@dbaron
Copy link
Member

dbaron commented Jun 7, 2017

For the record, the existing 2012 RECcomendation already requires that the value be positive, exactly matching what all the implementations do. Its Values section says:

The <resolution> value is a positive <number> immediately followed by a unit identifier (‘dpi’ or ‘dpcm’).

This probably explains why we have such good interoperability.

The text that led to that interoperability just appears to have been dropped from all current specification drafts.

This text, however, didn't get ported to the relevant section of values-3 when the type was copied there (in late 2011 or early 2012).

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Non-positive value should be invalid for 'resolution' feature, and agreed to the following resolutions:

  • RESOLVED: Spec should say resolution used to disallow negative numbers. It makes sense to allow negative with the boolean logic with some exmaples and that's for all MQ with a numberic value.
The full IRC log of that discussion <dael> Topic: Non-positive value should be invalid for 'resolution' feature
<dael> github topic: https://github.com//issues/1454
<dael> Florian: We found this about resolution, but it's more than that. There are a number of media features that take a range and for which negative value makes no sense. Resolution of -300 dpi makes no sense.
<dael> Florian: Old MQ it didn't really matter if we declared this as invalud don't parse or value evaluates to false. The difference it makes is in the OM. This has changed a bit in MQ4 because we have unknown values.
<dael> Florian: If we say it parses and is false there width -300 is false NOT width -300 is true.
<dael> Florian: I think it would be more useful to parse it and say it evaluates to false. If it doesn't parse we don't know if it is -300 or not.
<dael> Florian: We have, from the OM Po we have interop on the opposite. But the boolean logic isn't impl anywhere yet so it's not observable.
<dael> Florian: I don't feel like breaking the interop is a major issue, but maybe I'm wrong. What do you all think?
<dael> dino: I thinkw e should do as you said and break the interop.
<dael> Florian: If they're relying on media resolution -300dpi this will be fine. What this would break is very uncommon.
<dael> dbaron: Does the effect of the change...is it different on if the browser impl the unknown thing?
<dael> Florian: Yes. It's part of impl the logic for nested and/or/not. If you don't have that then it's undefined what this does, I guess.
<dael> dbaron: Maybe the spec should have a note saying that the previous version forbade negative values and that shouldn't be removed until you impl this stuff.
<dael> Florian: That's fair
<dael> astearns: Prop: have the spec say you should parrse - values if you implement this boolean stuff. Obj?
<dael> Florian: This is all media features with a range. width, heigth, aspect ratio, color I suppose.
<dael> astearns: Spec should say resolution used to disallow - numbers. It makes sense to allow - witht he boolean logic with some exmaples and that's for all MQ with a numberic value.
<dael> dbaron: There should be a note that it's a change and we're looking for feedback.
<dael> Florian: Fair.
<dbaron> s/change and we're looking for feedback/change from the previous version and we're looking for compat feedback/
<AmeliaBR> [Tangent] A reminder that this and many aspects of CSS parsing would have been easier define if there were CSS Values data types specifically for non-negative or strictly-positive values (https://github.com//issues/355).
<dael> RESOLVED: Spec should say resolution used to disallow negative numbers. It makes sense to allow negative with the boolean logic with some exmaples and that's for all MQ with a numberic value.

@frivoal
Copy link
Collaborator

frivoal commented Jun 8, 2017

@upsuper We did agree to a change based on the issue you raised, but the conclusion we reached is not the one you initially suggested. Can you confirm that you are OK with that change?

@upsuper
Copy link
Member Author

upsuper commented Jun 8, 2017

I'm OK with that change.

@frivoal frivoal added Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Commenter Response Pending labels Jun 8, 2017
@frivoal frivoal closed this as completed in e922b42 Jun 8, 2017
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512

UltraBlame original commit: 71de3b0f414bc494c7efaf7a9172bd28f86db940
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512

UltraBlame original commit: 71de3b0f414bc494c7efaf7a9172bd28f86db940
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
… media queries (from emilio:negative-resolution); r=upsuper

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1366961
See: w3c/csswg-drafts#1454
Source-Repo: https://github.com/servo/servo
Source-Revision: 8d950bd62036cebeb6596e74f412444061e59512

UltraBlame original commit: 71de3b0f414bc494c7efaf7a9172bd28f86db940
frivoal added a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2020
@frivoal frivoal added Tested Memory aid - issue has WPT tests and removed Needs Testcase (WPT) labels Jul 4, 2020
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 10, 2020
…e negative range, a=testonly

Automatic update from web-platform-tests
Test media features that are false in the negative range (#24433)

See w3c/csswg-drafts#1454
--

wpt-commits: 6b786f18589c7407b7b804c883b5d5cbf254c086
wpt-pr: 24433
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jul 10, 2020
…e negative range, a=testonly

Automatic update from web-platform-tests
Test media features that are false in the negative range (#24433)

See w3c/csswg-drafts#1454
--

wpt-commits: 6b786f18589c7407b7b804c883b5d5cbf254c086
wpt-pr: 24433
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. mediaqueries-4 Current Work Tested Memory aid - issue has WPT tests Tracked in DoC
Projects
None yet
Development

No branches or pull requests

5 participants