Bug 100865 – Warn in the inspector console when using dpi and dpcm units outside of media="print"

• Component: CSS
• Status: RESOLVED FIXED

[John Mellor]

The dpi unit as specified[2] is incredibly confusing.

The iPhone 3G and iPhone 4 are widely known to have 163 ppi and 326 ppi screens respectively. So people are going to write media queries like:

@media (min-resolution: 300dpi) { /* 2x graphics */ }

And this is going to fail. Why? Because the spec[1] defined dpi and dpcm in terms of CSS inches and CSS cm, which are a fixed number of px, rather than based on the physical units. Hence the iPhone 3G and iPhone 4 are actually 96 CSS dpi and 192 CSS dpi respectively[1]!

While authors have just about gotten used to CSS's fixed-size inches and cm, and adapted by simply avoiding all use of those units, it will take a long time for them to realise that dpi and dpcm are similarly broken. This is especially dangerous since the effects of resolution media queries are usually fairly subtle - e.g. images gets slightly sharper/blurrier or more bandwidth-intensive when they are set wrongly, but it's rare to get elements appearing the wrong size or in the wrong place.

To prevent authors shooting themselves in the foot with this counter-intuitive "feature", we should drop support for the dpi and dpcm units before shipping resolution mq/image-resolution, and only support dppx (which does behave sensibly across different resolution devices).

[1]: (in case it's not clear why that is, it's because iPhone 3G and iPhone 4 are 1 dppx and 2 dppx respectively, and the spec[2] hardcodes 1 dppx == 96 CSS dpi, so you have to multiply those values by 96, even though on phones 1 dppx usually means 160 physical ppi, since phones are held closer to the user's eyes, so you need more physical pixels per inch, and for the same reason the CSS px unit represents a smaller physical length on phones than on desktops[3])

[2]: http://dev.w3.org/csswg/css3-values/#resolution

[3]: http://dev.w3.org/csswg/css3-values/#reference-pixel

[Tony Chang]

You should bring this up on the www-style mailing list.  We should argue to change the spec if we think it's not going to be helpful/useful.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> You should bring this up on the www-style mailing list.  We should argue to change the spec if we think it's not going to be helpful/useful.

I agree with that. Also I would like to point out that dpi is a lot more useful for printers etc than using dppx.

Another option would be to warn the user then using dpi etc which are not a factor of 96.

[Rick Byers]

(In reply to comment #2) 
> Another option would be to warn the user then using dpi etc which are not a factor of 96.

I don't think that's an option - devices can and will have non-integer device scale factors.  Android already uses non-integer device pixel ratios via fixed layout, and we'll probably need 1.4 and 1.8 for windows metro eventually.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (In reply to comment #2) 
> > Another option would be to warn the user then using dpi etc which are not a factor of 96.
> 
> I don't think that's an option - devices can and will have non-integer device scale factors.  Android already uses non-integer device pixel ratios via fixed layout, and we'll probably need 1.4 and 1.8 for windows metro eventually.

Yeah you are totally right about that, but we can warn about avoid using dpi or dpcm together with 'screen'.

[Tab Atkins]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2) 
> > > Another option would be to warn the user then using dpi etc which are not a factor of 96.
> > 
> > I don't think that's an option - devices can and will have non-integer device scale factors.  Android already uses non-integer device pixel ratios via fixed layout, and we'll probably need 1.4 and 1.8 for windows metro eventually.
> 
> Yeah you are totally right about that, but we can warn about avoid using dpi or dpcm together with 'screen'.

I don't understand. Rick pointed out that android devices - which match the 'screen' media type - have and will continue to have non-integer ratios.

Or are you just arguing that, outside of printing, people should simply never use dpi/dpcm, and just use dppx, with non-integer values when appropriate?

I have an example in the Images 4 spec (<http://dev.w3.org/csswg/css4-images/#image-set-notation> scroll down to example 9) of using both x and dpi in a single image-set() value, where the dpi one is intended for printing.

[Kenneth Rohde Christiansen]

> Or are you just arguing that, outside of printing, people should simply never use dpi/dpcm, and just use dppx, with non-integer values when appropriate?

John argues that the dpi/dpcm values have a good chance for being used incorrectly by web authors. So I was suggesting that we could warn in the Web Inspector console, like we do for viewport meta width = 320, at least when used in combination with 'screen'.

I don't want to remove the support as it is quite useful for printing.

[John Mellor]

Printers are interesting. They could use dppx (300dpi would be 3.125 dppx), but I appreciate that it's more clumsy.

Warning when dpi/dpcm are used in combination with screen would definitely be a good start. I worry that a lot of sites might use them but not specify screen, so would never see the warning.

Could we warn whenever they are used outside of media="print"? This would show a warning for Tab's mixed units example (which combines 1x, 2x and 600dpi in a non-print stylesheet). But on some level that warning seems valid, as that usage is only safe because no devices yet have 600 ppi screens; if the example used 300dpi instead of 600dpi it would behave unpredictably, as the 300dpi would fail to match iPhone 4 etc.

[Kenneth Rohde Christiansen]

> Could we warn whenever they are used outside of media="print"? This would show a warning for Tab's mixed units example (which combines 1x, 2x and 600dpi in a non-print stylesheet). But on some level that warning seems valid, as that usage is only safe because no devices yet have 600 ppi screens; if the example used 300dpi instead of 600dpi it would behave unpredictably, as the 300dpi would fail to match iPhone 4 etc.

I am all for that.

[Alexander Shalamov]

I could take a look at this bug. I think I know good place where we could throw error to inspector console.

[Alexander Shalamov]

Created attachment 172748 [details]
Patch 1

- patch for initial review
- Added function that prints warning to inspector console
- Warning is printed when:
  * StyleSheet is parsed
  * HTMLStyleElement is updated or created
  * Media Query listener interface is used

[John Mellor]

Comment on attachment 172748 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=172748&action=review

Thanks for the patch! Some quick comments:

> Source/WebCore/css/MediaList.cpp:306
> +static void addWarningMessageToConsole(Document* document, const String& serializedExpression, const CSSPrimitiveValue* value)

Perhaps rename this to addResolutionWarningMessageToConsole to clarify that it has a specialized purpose?

> Source/WebCore/css/MediaList.cpp:308
> +    DEFINE_STATIC_LOCAL(String, mediaQueryMessage, (ASCIILiteral("Consider using 'dppx' units for 'screen' media type, as the '%replacementUnits%' units are in CSS units and not actual device units. Media query expression: ")));

Two slightly confusing things about this message:
1. It mentions 'screen' media type, even if you didn't explicitly target screen (e.g. in CSS rules for all media types).
2. It's not obvious what the implications are of dpi being in CSS units.

How about the following text instead:

"Consider using 'dppx' units, as 'dpi' is dots-per-CSS-in, not dots-per-physical-inch, so will not match physical pixel density of mobile device screens. Media query expression: "
and
"Consider using 'dppx' units, as 'dpcm' is dots-per-CSS-cm, not dots-per-physical-cm, so will not match physical pixel density of mobile device screens. Media query expression: "

> Source/WebCore/css/MediaList.cpp:341
> +        if (!query->ignored() && (equalIgnoringCase(mediaType, "screen") || equalIgnoringCase(mediaType, "all") || mediaType.isEmpty())) {

What about "handheld", "projection" or "tv" media types? I expect would using dpi in such media types to suffer from the same issues...

> Source/WebCore/css/MediaList.h:126
> +// FIXME: should be removed when dpi / dpcm values would be deprecated for resolution media feature.

I'm not quite sure what this FIXME means?

[Kenneth Rohde Christiansen]

Comment on attachment 172748 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=172748&action=review

>> Source/WebCore/css/MediaList.cpp:308
>> +    DEFINE_STATIC_LOCAL(String, mediaQueryMessage, (ASCIILiteral("Consider using 'dppx' units for 'screen' media type, as the '%replacementUnits%' units are in CSS units and not actual device units. Media query expression: ")));
> 
> Two slightly confusing things about this message:
> 1. It mentions 'screen' media type, even if you didn't explicitly target screen (e.g. in CSS rules for all media types).
> 2. It's not obvious what the implications are of dpi being in CSS units.
> 
> How about the following text instead:
> 
> "Consider using 'dppx' units, as 'dpi' is dots-per-CSS-in, not dots-per-physical-inch, so will not match physical pixel density of mobile device screens. Media query expression: "
> and
> "Consider using 'dppx' units, as 'dpcm' is dots-per-CSS-cm, not dots-per-physical-cm, so will not match physical pixel density of mobile device screens. Media query expression: "

s/dots-per-CSS-in/dots-per-CSS-inch/ :-)
I will change mobile to "all" (mac book pro's have the same issue)

>> Source/WebCore/css/MediaList.cpp:341
>> +        if (!query->ignored() && (equalIgnoringCase(mediaType, "screen") || equalIgnoringCase(mediaType, "all") || mediaType.isEmpty())) {
> 
> What about "handheld", "projection" or "tv" media types? I expect would using dpi in such media types to suffer from the same issues...

I would do it the other way around, just not complain when using 'print'

>> Source/WebCore/css/MediaList.h:126
>> +// FIXME: should be removed when dpi / dpcm values would be deprecated for resolution media feature.
> 
> I'm not quite sure what this FIXME means?

Just remove it

[John Mellor]

Comment on attachment 172748 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=172748&action=review

>>> Source/WebCore/css/MediaList.cpp:308
>>> +    DEFINE_STATIC_LOCAL(String, mediaQueryMessage, (ASCIILiteral("Consider using 'dppx' units for 'screen' media type, as the '%replacementUnits%' units are in CSS units and not actual device units. Media query expression: ")));
>> 
>> Two slightly confusing things about this message:
>> 1. It mentions 'screen' media type, even if you didn't explicitly target screen (e.g. in CSS rules for all media types).
>> 2. It's not obvious what the implications are of dpi being in CSS units.
>> 
>> How about the following text instead:
>> 
>> "Consider using 'dppx' units, as 'dpi' is dots-per-CSS-in, not dots-per-physical-inch, so will not match physical pixel density of mobile device screens. Media query expression: "
>> and
>> "Consider using 'dppx' units, as 'dpcm' is dots-per-CSS-cm, not dots-per-physical-cm, so will not match physical pixel density of mobile device screens. Media query expression: "
> 
> s/dots-per-CSS-in/dots-per-CSS-inch/ :-)
> I will change mobile to "all" (mac book pro's have the same issue)

> s/dots-per-CSS-in/dots-per-CSS-inch/ :-)

"CSS-in" was meant emphasize that it was the CSS unit, but agree that it's probably clearer to just put "CSS-inch".

> I will change mobile to "all" (mac book pro's have the same issue)

Good point. How about:

"Consider using 'dppx' units instead of 'dpi', as in CSS 'dpi' means dots-per-CSS-inch, not dots-per-physical-inch, so does not correspond to the actual dpi of a screen. In media query expression: "

[Kenneth Rohde Christiansen]

Sounds good to me

[Alexander Shalamov]

Created attachment 172947 [details]
Patch 2

- rebased to master
- removed FIXME
- modified warning message according to review comments

[Kenneth Rohde Christiansen]

Comment on attachment 172947 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=172947&action=review

> Source/WebCore/css/MediaList.cpp:306
> +static void addWarningMessageToConsole(Document* document, const String& serializedExpression, const CSSPrimitiveValue* value)

addResolutionWarningMessageToConsole... it is not just any warning

> Source/WebCore/css/MediaList.cpp:316
> +
> +    String message;
> +    if (value->isDotsPerInch())
> +        message = String(mediaQueryMessage).replace("%replacementUnits%", mediaValueDPI).replace("%lengthUnit%", lengthUnitInch);

Why no assert for calling this method when it shouldnt warn?

> Source/WebCore/css/MediaList.cpp:329
> +void reportMediaQueryWarning(Document* document, const MediaQuerySet* mediaQuerySet)

IfNeeded ?

[Alexander Shalamov]

Created attachment 172963 [details]
Patch 3

- reportMediaQueryWarning => reportMediaQueryWarningIfNeeded
- addWarningMessageToConsole => addResolutionWarningMessageToConsole
- add asserts for null document, null css value or css value that is not dpcm / dpi

[John Mellor]

Comment on attachment 172963 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=172963&action=review

> Source/WebCore/css/MediaList.cpp:349
> +        if (!query->ignored() && (equalIgnoringCase(mediaType, "screen") || equalIgnoringCase(mediaType, "all") || mediaType.isEmpty())) {

This patch doesn't seem to address these two comments:

kenneth wrote:
> johnme wrote:
> > What about "handheld", "projection" or "tv" media types? I expect would using dpi in such media types to suffer from the same issues...
> I would do it the other way around, just not complain when using 'print'

[Alexander Shalamov]

Created attachment 172979 [details]
Patch 4

-        Reviewed by NOBODY (OOPS!).
+        Reviewed by Kenneth Rohde Christiansen.

-        if (!query->ignored() && (equalIgnoringCase(mediaType, "screen") || equalIgnoringCase(mediaType, "all") || mediaType.isEmpty())) {
+        if (!query->ignored() && !equalIgnoringCase(mediaType, "print")) {

[WebKit Review Bot]

Comment on attachment 172979 [details]
Patch 4

Clearing flags on attachment: 172979

Committed r133884: <http://trac.webkit.org/changeset/133884>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

As far as I know, this is the first time we show warnings in the Inspector for a CSS issue. Before we do this, I'd like to hear more of a consensus on webkit-dev that this is a desired behavior.