Bug 87407

Created attachment 143854 [details] work in progress

Created attachment 143859 [details] Work in progres

Apparently this matches what iOS does more closely.

Comment on attachment 143859 [details] Work in progres This version looks great. Only question is whether all ports actually set defaultDeviceScaleFactor; I get the feeling it's currently somewhat optional.

> Only question is whether all ports actually set defaultDeviceScaleFactor; I get the feeling it's currently somewhat optional. My next step is to write a test, which will hopefully help answer this question.

Note to folks looking at this bug: I'm still very much learning about how we do scaling and/or pixel ratios. :)

Comment on attachment 143859 [details] Work in progres I'm confused by this change. Why is the old way wrong?

> I'm confused by this change. Why is the old way wrong? Here is what johnme wrote when he made this in the chromium-android branch: ---8<--- Tweak device-pixel-ratio & device-width/height metrics. 1. Always return the default devicePixelRatio (e.g. 2 on Galaxy Nexus) instead of changing it based on the viewport tag. This matches both iOS and Android Browser behavior, as well as being more useful. 2. Use device-independent pixels for @media query & CSSOM View widths/heights. This matches iOS, and arguably better matches specs, as well as being more useful. --->8--- My understanding is that these locations shouldn't be affected by the viewport tag, which is why we need to use the defaultDeviceScaleFactor rather than the current deviceScaleFactor. As I wrote above, I'm just learning about these mechanisms, so please be gentle. :)

Note: http://trac.webkit.org/browser/trunk/LayoutTests/fast/viewport/viewport-133.html seems to imply that devicePixelRatio should be affected by the viewport tag.

I see, but I think there's a misunderstanding about frame->page()->deviceScaleFactor(). This doesn't change with scaling; it's the screen pixel ratio (1 or 2), and nothing else. That test looks wrong.

http://trac.webkit.org/browser/trunk/Source/WebCore/page/Page.cpp#L674 seems to have a bunch of machinery for handling dynamic updates, which doesn't seem that useful if the value is a constant. I'm tracking down who calls that function now.

> I'm tracking down who calls that function now. It looks like at least Qt and Chromium call setDeviceScaleFactor when processing the viewport. How does PLATFORM(MAC) handle scaling things for the viewport?

(In reply to comment #12) > > I'm tracking down who calls that function now. > > It looks like at least Qt and Chromium call setDeviceScaleFactor when processing the viewport. > > How does PLATFORM(MAC) handle scaling things for the viewport? BlackBerry does so as well http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/WebPage.cpp#L3338

> How does PLATFORM(MAC) handle scaling things for the viewport? Looks like PLATFORM(MAC) doesn't implement viewport: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm#L239

smfr: It looks like this value does change, which means that we'll need something like this patch to restore the unchanging nature of window.devicePixelRatio. It sounds like I should change viewport-133.html to expect the correct behavior.

I guess we all have different notions of what deviceScaleFactor means :( The zooming that mobile devices and Safari do on desktop should not affect deviceScaleFactor. They show up in pageScaleFactor etc.

Fascinating. Should we change viewport to affect the pageScaleFactor rather than the deviceScaleFactor? We could potentially eliminate the defaultDeviceScaleFactor in that case.

Using the page scale factor for the viewport appear problematic because the page scale factor is settable via a public API. For example, WKPageSetScaleFactor appears to set the page scale factor directly.

(In reply to comment #8) > > I'm confused by this change. Why is the old way wrong? > > Here is what johnme wrote when he made this in the chromium-android branch: > > ---8<--- > Tweak device-pixel-ratio & device-width/height metrics. > > 1. Always return the default devicePixelRatio (e.g. 2 on Galaxy Nexus) > instead of changing it based on the viewport tag. This matches both > iOS and Android Browser behavior, as well as being more useful. Why do we want this change? A port can change its implementation to do what it wants with the results of computeViewportAttributes. > > 2. Use device-independent pixels for @media query & CSSOM View > widths/heights. This matches iOS, and arguably better matches specs, > as well as being more useful. Aren't device-independent pixels independent from the default devicePixelRatio. I thought they were supposed to be the viewport's value?

kpiascik, I'm sorry, I didn't understand your questions. > > 1. Always return the default devicePixelRatio (e.g. 2 on Galaxy Nexus) > > instead of changing it based on the viewport tag. This matches both > > iOS and Android Browser behavior, as well as being more useful. > > Why do we want this change? A port can change its implementation to do what it wants with the results of computeViewportAttributes. I'm not sure I understand. Presumably whether window.devicePixelRatio is affected by viewport meta tag is something that should work the same in every browser. My understanding is that window.devicePixelRatio should be a constant property of the device and not dependent on the configured viewport. smfr appears to confirm that understanding in Comment #10. > > 2. Use device-independent pixels for @media query & CSSOM View > > widths/heights. This matches iOS, and arguably better matches specs, > > as well as being more useful. > > Aren't device-independent pixels independent from the default devicePixelRatio. I thought they were supposed to be the viewport's value? I don't understand this question either, but I'm inclined to sort out window.devicePixelRatio first and then figure out what the right thing to do with media queries second.

(In reply to comment #20) > kpiascik, I'm sorry, I didn't understand your questions. > > > > 1. Always return the default devicePixelRatio (e.g. 2 on Galaxy Nexus) > > > instead of changing it based on the viewport tag. This matches both > > > iOS and Android Browser behavior, as well as being more useful. > > > > Why do we want this change? A port can change its implementation to do what it wants with the results of computeViewportAttributes. > > I'm not sure I understand. Presumably whether window.devicePixelRatio is affected by viewport meta tag is something that should work the same in every browser. My understanding is that window.devicePixelRatio should be a constant property of the device and not dependent on the configured viewport. smfr appears to confirm that understanding in Comment #10. devicePixelRatio is basically the adjustment ratio. Like most mobile pages have been designed with a width of 320 in mind, and thus on iPhone4 it is 2x as 2x320 == 640. On many other devices such as android and firefox with device widths (in portrait mode) of 480, this value is often 1.5. When the target-densitydpi is set to something differently from 160 dpi, say device-dpi on a 480x... Android device, no scale adjustment is applied to the content, thus it might make sense to expose different device-pixel-ratio. I wonder what Android actually does here. John? > > > > 2. Use device-independent pixels for @media query & CSSOM View > > > widths/heights. This matches iOS, and arguably better matches specs, > > > as well as being more useful. > > > > Aren't device-independent pixels independent from the default devicePixelRatio. I thought they were supposed to be the viewport's value? > > I don't understand this question either, but I'm inclined to sort out window.devicePixelRatio first and then figure out what the right thing to do with media queries second.

(In reply to comment #21) > (In reply to comment #20) > > kpiascik, I'm sorry, I didn't understand your questions. > > > > > > 1. Always return the default devicePixelRatio (e.g. 2 on Galaxy Nexus) > > > > instead of changing it based on the viewport tag. This matches both > > > > iOS and Android Browser behavior, as well as being more useful. > > > > > > Why do we want this change? A port can change its implementation to do what it wants with the results of computeViewportAttributes. > > > > I'm not sure I understand. Presumably whether window.devicePixelRatio is affected by viewport meta tag is something that should work the same in every browser. My understanding is that window.devicePixelRatio should be a constant property of the device and not dependent on the configured viewport. smfr appears to confirm that understanding in Comment #10. > > devicePixelRatio is basically the adjustment ratio. Like most mobile pages have been designed with a width of 320 in mind, and thus on iPhone4 it is 2x as 2x320 == 640. On many other devices such as android and firefox with device widths (in portrait mode) of 480, this value is often 1.5. This is not how iOS treats it. It's simply a multiplier between device pixels and "UI" pixels (== CSS pixels on an unscaled page). It does not change with zooming or viewport settings.

> > devicePixelRatio is basically the adjustment ratio. Like most mobile pages have been designed with a width of 320 in mind, and thus on iPhone4 it is 2x as 2x320 == 640. On many other devices such as android and firefox with device widths (in portrait mode) of 480, this value is often 1.5. > > This is not how iOS treats it. It's simply a multiplier between device pixels and "UI" pixels (== CSS pixels on an unscaled page). It does not change with zooming or viewport settings. iOS also doesn't support the target-densitydpi argument, which is the only one that modified the device-pixel-ratio. Android supports that, but if it doesn't modify the device-pixel-ratio we should fix this, I just wonder whether that actually makes sense since target-densitydpi = device-dpi means that there is no upscaling of the content.

> > This is not how iOS treats it. It's simply a multiplier between device pixels and "UI" pixels (== CSS pixels on an unscaled page). It does not change with zooming or viewport settings. I don't think anyone changes this with zooming, but only when the target-densitydpi is set as it affects the upscaling of the contents.

@kenneth: We're talking about this in #webkit now if you'd like to talk more interactively.

We had a long discussion about this topic in IRC, and the net result is that we believe this patch is incorrect and should be reverted from the chromium-android branch.

http://menacingcloud.com/?c=highPixelDensityDisplays is a good resource for people not knowing what device-pixel-ratio is used for.

Other good resources mentioning this: http://dev.opera.com/articles/view/an-introduction-to-meta-viewport-and-viewport/ http://developer.android.com/guide/webapps/targeting.html http://developer.android.com/reference/android/webkit/WebView.html http://dev.w3.org/csswg/css-device-adapt/

More fun with pixel ratios in Bug 87440 for those who are interested.

Adam Barth wrote: > We had a long discussion about this topic in IRC, and the > net result is that we believe this patch is incorrect (...) Unfortunately I missed this due to timezones. Let me try to explain the reason for this change. On mobile pages with a "width=device-width" viewport, everyone agrees that window.devicePixelRatio and the corresponding media query should return the defaultDeviceScaleFactor, i.e. approximately (physical screen dpi / 160). That already works, with or without this patch. There are two controversial cases: 1. Desktop page with no viewport (or "width=980"). 2. Mobile page with "width=device-width, target-densitydpi=device=dpi". In both these cases WebCore currently always returns 1 for window.devicePixelRatio and the corresponding media query, but it would be better to return defaultDevicePixelRatio. Since there's no spec for devicePixelRatio, we need to choose our behavior based on: a) the de facto standard behavior b) usefulness A) First, let's try to work out the defacto standard behavior. I wrote a test page for both of the controversial cases above: 1. http://jsbin.com/urowoh/33#none 2. http://jsbin.com/urowoh/33#device-width,device-dpi and tested on high dpi devices with recent copies of Android Browser, Chrome for Android, Firefox, IE Mobile, Nokia N9 browser, Opera Mobile and Mobile Safari. On the 1st test page (no viewport): - IE9 Mobile doesn't support window.devicePixelRatio or the media query. - Android Browser, Chrome for Android, Opera Mobile and Mobile Safari all return the defaultDeviceScaleFactor (2.0 on a Samsung Galaxy Nexus, except Opera which returns 2.25 because they prefer to force their device-width metric to 320 instead of 360). - Firefox Mobile and Nokia N9 browser return 1 (Firefox only for the media query; it seems not to support window.devicePixelRatio). On the 2nd test page (width=device-width, target-densitydpi=device=dpi): - IE Mobile, Mobile Firefox and Mobile Safari don't support target-densitydpi. - Android Browser, Chrome for Android and Opera Mobile all return the defaultDeviceScaleFactor (2.0 on a Samsung Galaxy Nexus, except Opera which returns 2.25 because they prefer to force their device-width metric to 320 instead of 360). - Nokia N9 browser is the only browser that returns 1. In both cases it's clear that the de facto standard behavior is to return defaultDeviceScaleFactor, not 1. B) Now let's consider which behavior is actually useful to web developers. It's clear that always returning 1 when you are on a desktop page or a page with target-densitydpi=device-dpi provides no useful information. So what are we missing out on? Quite a lot, it turns out. On desktop pages that can be viewed on high dpi devices, authors use the defaultDeviceScaleFactor to determine whether to load high dpi images etc (window.devicePixelRatio can be used to provide polyfills for img srcset). On target-densitydpi=device-dpi pages, it's *crucial* for webpages to know the defaultDeviceScaleFactor, as they have taken on responsibility for scaling the page content appropriately for the device's dpi, e.g. they have to scale buttons in proportion to the defaultDeviceScaleFactor. Conclusion: both because it's the de facto standard implemented by 4 browsers (including 3 based on WebKit), and because it's much more useful to web developers, we should always return the fixed defaultDeviceScaleFactor value for window.devicePixelRatio and device-pixel-ratio media queries instead of returning 1. I'm ambivalent about the actual implementation details though. If we can make deviceScaleFactor constant so it doesn't depend on the viewport, then I'd be more than happy to get rid of defaultDeviceScaleFactor and use that instead. This might be cleaner, as I suspect there are several places where WebKit currently uses deviceScaleFactor where they intended to use defaultDeviceScaleFactor.

> On desktop pages that can be viewed on high dpi devices, authors use the defaultDeviceScaleFactor to determine whether to load high dpi images etc (window.devicePixelRatio can be used to provide polyfills for img srcset). How does that help? You are painting one css units as one device pixel? How would loading higher resolution images and down-scaling them help? > On target-densitydpi=device-dpi pages, it's *crucial* for webpages to know the defaultDeviceScaleFactor, as they have taken on responsibility for scaling the page content appropriately for the device's dpi, e.g. they have to scale buttons in proportion to the defaultDeviceScaleFactor. That is a pretty good point. > Conclusion: both because it's the de facto standard implemented by 4 browsers (including 3 based on WebKit), and because it's much more useful to web developers, we should always return the fixed defaultDeviceScaleFactor value for window.devicePixelRatio and device-pixel-ratio media queries instead of returning 1. I am pretty OK with it, I just wanted to know the advantages of always returning it. Now, how can I make use of the second use case if I do target-densitydpi=low-dpi? How will I know the actual difference in this case? When we agree on this, can you send an email to the www-style?

Did you not test iOS because it doesn't support target-densitydpi?

(In reply to comment #31) > > On desktop pages that can be viewed on high dpi devices, authors use the defaultDeviceScaleFactor to determine whether to load high dpi images etc (window.devicePixelRatio can be used to provide polyfills for img srcset). > > How does that help? You are painting one css units as one device pixel? How would loading higher resolution images and down-scaling them help? But we're not painting one CSS unit per device pixel: we're scaling the page by a pageScaleFactor which is less than one (e.g. 320/980 = 0.327) in overview mode, and often greater than one when zoomed in and reading content. In particular, when pageScaleFactor == defaultDeviceScaleFactor the content will appear the same size as it does on a desktop monitor, and so this is an important zoom level to allow the page to optimize for. This is complicated a little by Font Boosting / text size adjust, which means the user often doesn't need to zoom in so much on wide paragraphs, but defaultDeviceScaleFactor still provides a useful upper bound on how much a user is likely to need to zoom in. > Now, how can I make use of the second use case if I do target-densitydpi=low-dpi? How will I know the actual difference in this case? I never quite worked out why people would ever use values other than auto or device-dpi for this; I'd be keen to hear use cases where this is useful. In fact according to http://dev.w3.org/csswg/css-device-adapt/#the-target-densitydpi-property values other than device-dpi may be dropped. In any case, on browsers that default to target-densitydpi=160 (i.e. most phone browsers other than Opera on Galaxy Nexus) web developers can calculate desired ratios by noting that low-dpi, medium-dpi and high-dpi are fixed at 120, 160 and 240, so on low-dpi with width=device-width the ratio of CSS pixels to device pixels is approximately (window.devicePixelRatio * 160/120). I guess on devices with default target densities other than 160 (e.g. tablets and laptops, where distance from the screen is greater hence CSS pixels are larger due to http://www.w3.org/TR/css3-values/#reference-pixel) it might be useful to know what the default value of target-densitydpi=auto would have been if you hadn't overridden it with a fixed value, but generally if you're asking such questions you shouldn't have overridden it in the first place. So this may be a useful number to expose somehow, but it's pretty niche, and I don't think it's worth breaking the useful and commonplace uses of window.devicePixelRatio to achieve that. > When we agree on this, can you send an email to the www-style? Sure, I've been meaning to do that for a while. (In reply to comment #32) > Did you not test iOS because it doesn't support target-densitydpi? I did test iOS. Mobile Safari is included in the results for the no viewport test page (it returned the defaultDeviceScaleFactor, which was 2 on an iPod touch 4th gen). Obviously it wasn't possible to see what it would report when displaying a target-densitydpi page.

(In reply to comment #30) > In both cases it's clear that the de facto standard behavior is to return defaultDeviceScaleFactor, not 1. [snip] > Conclusion: both because it's the de facto standard implemented by 4 browsers (including 3 based on WebKit), and because it's much more useful to web developers, we should always return the fixed defaultDeviceScaleFactor value for window.devicePixelRatio and device-pixel-ratio media queries instead of returning 1. I'm sorry, but we (Apple) can't change what we've been returning for devicePixelRatio. As Simon mentioned, for us that exposes the scaling from CSS pixels to device pixels under no zoom (1 on normal resolution, 2 when we do pixel doubling for "retina" screens). The value never changes on a device. If you want to expose something more useful to developers, then it would have to be a new property.

(In reply to comment #34) > I'm sorry, but we (Apple) can't change what we've been returning for devicePixelRatio. As Simon mentioned, for us that exposes the scaling from CSS pixels to device pixels under no zoom (1 on normal resolution, 2 when we do pixel doubling for "retina" screens). The value never changes on a device. We seem to be talking at cross purposes. I am calling for upstream WebKit to match downstream iOS whose behavior is to return the defaultDeviceScaleFactor, which is fixed per device (i.e. 2 on retina displays). Apple doesn't need to change iOS, we just need to change upstream WebKit to match what most of the ports are already doing.

(In reply to comment #35) > (In reply to comment #34) > > I'm sorry, but we (Apple) can't change what we've been returning for devicePixelRatio. As Simon mentioned, for us that exposes the scaling from CSS pixels to device pixels under no zoom (1 on normal resolution, 2 when we do pixel doubling for "retina" screens). The value never changes on a device. > > We seem to be talking at cross purposes. I am calling for upstream WebKit to match downstream iOS whose behavior is to return the defaultDeviceScaleFactor, which is fixed per device (i.e. 2 on retina displays). Apple doesn't need to change iOS, we just need to change upstream WebKit to match what most of the ports are already doing. Excellent. Apologies for my confusion!

Comment on attachment 143859 [details] Work in progres Based on this discussion, it sounds like this patch is the one we should go with. There's a separate question of whether target-densitydpi is a good API, but that's something to worry about in a different bug. I'll spin up a new version of the patch that updates the test.

Created attachment 144644 [details] Patch

Comment on attachment 144644 [details] Patch Note: I'm not sure how to actually run that test, so I'll likely need to pull the right value off the bot after landing this patch.

Attachment 144644 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/MediaQueryEvaluator.cpp:297: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 144644 [details] Patch Attachment 144644 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12846655

Created attachment 144654 [details] Patch

Attachment 144654 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/MediaQueryEvaluator.cpp:298: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 144654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144654&action=review > Source/WebCore/ChangeLog:16 > + There's been some confusing about this patch because the Android typo: confusion

Thanks Dean.

Created attachment 144661 [details] Patch for landing

This patch looks like it will break the Mac because I don't see setDefaultDeviceScaleFactor() ever called in the Mac port.

(In reply to comment #47) > This patch looks like it will break the Mac because I don't see setDefaultDeviceScaleFactor() ever called in the Mac port. Settings::Settings has m_defaultDeviceScaleFactor(1). We'd need to call setDefaultDeviceScaleFactor for retina.

I'm composing an email to webkit-dev about this topic. I think we need to delete Settings::defaultDeviceScaleFactor and store three floats on Page.

Comment on attachment 144661 [details] Patch for landing Is it possible to make a test which sets it to 2.0 and then for a page without viewport meta tag, one with, etc?

I guess we should remove the devicePixelRatio from the WebCore::ViewportAttributes as part of this patch, as well?

See also bug 88114 which covers how the (default) deviceScaleFactor is calculated in the first place.

defaultDeviceScaleFactor no longer exists