Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
49913005:
Add Settings::deviceScaleAdjustment. (Closed)
|
Created:
7 years, 1 month ago by skobes Modified:
7 years, 1 month ago CC:
blink-reviews, jamesr, bemjb+rendering_chromium.org, zoltan1, eae+blinkwatch, leviw+renderwatch, abarth-chromium, dglazkov+blink, jchaffraix+rendering, aelias_OOO_until_Jul13 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd Settings::deviceScaleAdjustment.
This will be used to plumb the 1.05-1.3x fudge factor separately from the font
scale factor. It is applied to the autosizer's cluster multiplier only when
the page does not set an explicit meta viewport or @viewport.
BUG=252828
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160957
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address review comments. #
Total comments: 4
Patch Set 3 : Address review comments. #
Total comments: 2
Messages
Total messages: 25 (0 generated)
I think the Source/core side looks good with a few nits. https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.cpp File Source/core/page/Settings.cpp (right): https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.cpp... Source/core/page/Settings.cpp:262: void Settings::recalculateAutosizingMultipliers() Confusingly, autosizing and text autosizing are different. Do you mind naming this recalculateTextAutosizingMultipliers? https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.h File Source/core/page/Settings.h (right): https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.h#n... Source/core/page/Settings.h:94: void setDeviceScaleAdjustment(float); Can you add a comment here describing what this is? https://codereview.chromium.org/49913005/diff/1/Source/core/rendering/TextAut... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/49913005/diff/1/Source/core/rendering/TextAut... Source/core/rendering/TextAutosizer.cpp:213: if (viewportDescription.type == ViewportDescription::UserAgentStyleSheet) { Can you add this as a helper on ViewportDescription or maybe reuse ViewportStyleResolver's m_hasAuthorStyle?
https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.cpp File Source/core/page/Settings.cpp (right): https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.cpp... Source/core/page/Settings.cpp:262: void Settings::recalculateAutosizingMultipliers() On 2013/10/29 02:04:44, pdr wrote: > Confusingly, autosizing and text autosizing are different. Do you mind naming > this recalculateTextAutosizingMultipliers? Done. https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.h File Source/core/page/Settings.h (right): https://codereview.chromium.org/49913005/diff/1/Source/core/page/Settings.h#n... Source/core/page/Settings.h:94: void setDeviceScaleAdjustment(float); On 2013/10/29 02:04:44, pdr wrote: > Can you add a comment here describing what this is? Done. https://codereview.chromium.org/49913005/diff/1/Source/core/rendering/TextAut... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/49913005/diff/1/Source/core/rendering/TextAut... Source/core/rendering/TextAutosizer.cpp:213: if (viewportDescription.type == ViewportDescription::UserAgentStyleSheet) { On 2013/10/29 02:04:44, pdr wrote: > Can you add this as a helper on ViewportDescription or maybe reuse > ViewportStyleResolver's m_hasAuthorStyle? Done.
LGTM. @jamesr, do you like this too?
I don't know much about this fudge factor, but the plumbing of it lgtm
Two nits I discovered while plumbing this through for the inspector emulation side. https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... File Source/core/page/Settings.cpp (right): https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... Source/core/page/Settings.cpp:123: , m_deviceScaleAdjustment(1.0f) I think you'll need to move this above m_textAutosizingEnabled to make the clang build happy. error: field 'm_textAutosizingEnabled' will be initialized after field 'm_deviceScaleAdjustment'. https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... Source/core/page/Settings.cpp:277: void Settings::setDeviceScaleAdjustment(float deviceScaleAdjustment) In plumbing this through for the inspector side I found they use "device scale" in place of DPR. It may be cleaner to call this "setDeviceFontScaleAdjustment", but this is certainly optional.
https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... File Source/core/page/Settings.cpp (right): https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... Source/core/page/Settings.cpp:123: , m_deviceScaleAdjustment(1.0f) On 2013/10/30 04:53:08, pdr wrote: > I think you'll need to move this above m_textAutosizingEnabled to make the clang > build happy. > > error: field 'm_textAutosizingEnabled' will be initialized after field > 'm_deviceScaleAdjustment'. Done. https://codereview.chromium.org/49913005/diff/80001/Source/core/page/Settings... Source/core/page/Settings.cpp:277: void Settings::setDeviceScaleAdjustment(float deviceScaleAdjustment) On 2013/10/30 04:53:08, pdr wrote: > In plumbing this through for the inspector side I found they use "device scale" > in place of DPR. It may be cleaner to call this "setDeviceFontScaleAdjustment", > but this is certainly optional. I tried to make the name agnostic as to where it is applied... today it is on the font scale factor but in the future it may be used to adjust layout width or DPR instead.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/49913005/180001
Message was sent while issue was closed.
Change committed as 160957
Message was sent while issue was closed.
https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... Source/core/rendering/TextAutosizer.cpp:214: if (!viewportDescription.isSpecifiedByAuthor()) { Didn't we say we'd disable the fontScaleFactor if you have a <= device-width viewport, or an unscalable viewport? But I didn't think we wanted to disable it for e.g. a width=1000 viewport. IIRC the check you want here is the same as I added in https://codereview.chromium.org/18850005/diff/446001/content/public/android/j... for disabling double tap zoom, though you'd need to get the values from Blink not Java :)
Message was sent while issue was closed.
https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... File Source/core/rendering/TextAutosizer.cpp (right): https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... Source/core/rendering/TextAutosizer.cpp:214: if (!viewportDescription.isSpecifiedByAuthor()) { On 2013/11/05 18:56:40, johnme wrote: > Didn't we say we'd disable the fontScaleFactor if you have a <= device-width > viewport, or an unscalable viewport? But I didn't think we wanted to disable it > for e.g. a width=1000 viewport. Actually what we are doing is disabling the 1.05-1.3x multiplier on the font scale factor for sites with a meta viewport (I believe this is the proposal we agreed on in our last meeting). We will still autosize when layout width > device width.
Message was sent while issue was closed.
On 2013/11/05 19:02:45, skobes wrote: > https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... > File Source/core/rendering/TextAutosizer.cpp (right): > > https://codereview.chromium.org/49913005/diff/180001/Source/core/rendering/Te... > Source/core/rendering/TextAutosizer.cpp:214: if > (!viewportDescription.isSpecifiedByAuthor()) { > On 2013/11/05 18:56:40, johnme wrote: > > Didn't we say we'd disable the fontScaleFactor if you have a <= device-width > > viewport, or an unscalable viewport? But I didn't think we wanted to disable > it > > for e.g. a width=1000 viewport. > > Actually what we are doing is disabling the 1.05-1.3x multiplier on the font > scale factor for sites with a meta viewport Sorry, that's what I meant. > (I believe this is the proposal we agreed on in our last meeting). We will > still autosize when layout width > device width. Having a width=980 (or similar) meta viewport is considered by web developers to be 100% equivalent to having no viewport specified at all. We shouldn't be distinguishing between those two cases, as there is no semantic difference; both just mean it's a desktop site. And that extra 1.05-1.3x is (unfortunately) useful on many desktop sites. IIRC we'd agreed to only remove it for mobile or responsive websites, and websites that aren't scalable?
Message was sent while issue was closed.
On 2013/11/05 19:10:39, johnme wrote: > > (I believe this is the proposal we agreed on in our last meeting). We will > > still autosize when layout width > device width. > > Having a width=980 (or similar) meta viewport is considered by web developers to > be 100% equivalent to having no viewport specified at all Do you have a source for this? This is the first time I've heard it. I wonder if this will actually matter in real sites. I'm happy to take a poll of the top 1000 sites, but I suspect very few set viewport width=xyz without initial scale.
Message was sent while issue was closed.
On 2013/11/05 20:35:54, pdr wrote: > On 2013/11/05 19:10:39, johnme wrote: > > > (I believe this is the proposal we agreed on in our last meeting). We will > > > still autosize when layout width > device width. > > > > Having a width=980 (or similar) meta viewport is considered by web developers > to > > be 100% equivalent to having no viewport specified at all > > Do you have a source for this? This is the first time I've heard it. > > I wonder if this will actually matter in real sites. I'm happy to take a poll of > the > top 1000 sites, but I suspect very few set viewport width=xyz without initial > scale. It's very common. For example theverge.com (when viewed on desktop) has <meta name="viewport" content="width=1060">, and news.bbc.co.uk (again on desktop) has <meta name="viewport" content="width = 996">. These are both explicitly desktop sites (presumably as a result of server-side UA detection), and they are using viewport correctly. They do this to make sure browsers zoom to fit the website properly. For example old versions of Chrome (not sure about current state) would have loaded theverge.com slightly zoomed in unless it had that viewport tag, as they would zoom such that 980 CSS pixels are visible, and then the site would be slightly horizontally scrollable (by 1060-980 = 80px).
Message was sent while issue was closed.
> Having a width=980 (or similar) meta viewport is considered by web developers to > be 100% equivalent to having no viewport specified at all. We shouldn't be > distinguishing between those two cases, as there is no semantic difference; both > just mean it's a desktop site. And that extra 1.05-1.3x is (unfortunately) > useful on many desktop sites. IIRC we'd agreed to only remove it for mobile or > responsive websites, and websites that aren't scalable? We agreed on proposal #4 from the doc, the first half of which is to "remove the fudge factor for mobile sites (meta viewport present)". But it seems there is some confusion over what constitutes a "mobile site". My concern with disabling only for layout width <= device width is that it makes the cluster multiplier a discontinuous function of the layout width. E.g. on a Nexus 10 in portrait mode with the accessibility slider at 120% the cluster multiplier would be 1.0 for layout width up to 667px, increase linearly from 1.0 to 1.2 for layout width between 667px and 800px, then suddenly jump to 1.56 when layout width goes past 800px (increasing linearly thereafter). IOW, having a weird device-dependent threshold for when the weird device-dependent multiplier kicks in makes things more confusing for web developers than simply gating it on the presence of meta viewport. Explicit viewport parameters from the page author seems like a good signal that they are at least thinking about mobile platforms.
Message was sent while issue was closed.
On 2013/11/05 20:49:23, johnme wrote: > It's very common. For example http://theverge.com (when viewed on desktop) has <meta > name="viewport" content="width=1060">, and news.bbc.co.uk (again on desktop) has > <meta name="viewport" content="width = 996">. These are both explicitly desktop > sites (presumably as a result of server-side UA detection), and they are using > viewport correctly. But if they're doing server-side UA detection, they would serve something different to a mobile client, right? How much do we care about desktop versions of sites viewed on mobile where there is a mobile-optimized alternative that is served by default?
Message was sent while issue was closed.
(@aelias, let me know if you have any thoughts)
Message was sent while issue was closed.
I'd just note as an additional data point that "meta viewport" isn't respected by desktop browsers and likely never will be, but the upcoming @viewport property described in http://dev.w3.org/csswg/css-device-adapt/ will be supported on all browsers and allow setting different width configurations using media queries for responsive layout. Within Blink, "meta viewport" is already converted to an @viewport data structure and desktop sites on Android get the 980 value from a UA stylesheet specifying "@viewport { min-width: 980px; }". So this all leads to a world where desktop sites set the width, yes. In general, it's rare for sites to explicitly set initial-scale.
Message was sent while issue was closed.
On 2013/11/05 20:55:28, skobes wrote: > > Having a width=980 (or similar) meta viewport is considered by web developers > to > > be 100% equivalent to having no viewport specified at all. We shouldn't be > > distinguishing between those two cases, as there is no semantic difference; > both > > just mean it's a desktop site. And that extra 1.05-1.3x is (unfortunately) > > useful on many desktop sites. IIRC we'd agreed to only remove it for mobile or > > responsive websites, and websites that aren't scalable? > > We agreed on proposal #4 from the doc, the first half of which is to "remove the > fudge factor for mobile sites (meta viewport present)". But it seems there is > some confusion over what constitutes a "mobile site". Sorry, I missed the bracketed part. I wouldn't have agreed to that (incorrect) definition of a mobile site ;-) > My concern with disabling only for layout width <= device width is that it makes > the cluster multiplier a discontinuous function of the layout width. E.g. on a > Nexus 10 in portrait mode with the accessibility slider at 120% the cluster > multiplier would be 1.0 for layout width up to 667px, increase linearly from 1.0 > to 1.2 for layout width between 667px and 800px, then suddenly jump to 1.56 when > layout width goes past 800px (increasing linearly thereafter). I dislike jumps too. But it's more important for the browser to behave consistently on websites that the user (and author) perceive to be similar. For example we disable double-tap zoom if the page width (specifically document.documentElement.scrollWidth) is <= the window width in DIPs (specifically window.outerWidth). See https://codereview.chromium.org/18850005/. It would be wrong for double-tap to be enabled depending on whether the page has no viewport or has a width=980 viewport, since those two behave identically to users (and are semantically identical to web developers). > IOW, having a weird device-dependent threshold for when the weird > device-dependent multiplier kicks in makes things more confusing for web > developers than simply gating it on the presence of meta viewport. Explicit > viewport parameters from the page author seems like a good signal that they are > at least thinking about mobile platforms. Thinking about mobile platforms != is a mobile site. I agree that explicit viewport parameters is a good signal, but we should additionally require that they use a mobile viewport, not a desktop viewport (or make it not scalable). My definition provides a very simple opt-in: just use width=device-width (or an equivalent variant), which is something that every mobile or responsive site should be doing anyway. On 2013/11/05 21:05:15, skobes wrote: > On 2013/11/05 20:49:23, johnme wrote: > > It's very common. For example http://theverge.com (when viewed on desktop) has > <meta > > name="viewport" content="width=1060">, and news.bbc.co.uk (again on desktop) > has > > <meta name="viewport" content="width = 996">. These are both explicitly > desktop > > sites (presumably as a result of server-side UA detection), and they are using > > viewport correctly. > > But if they're doing server-side UA detection, they would serve something > different to a mobile client, right? How much do we care about desktop versions > of sites viewed on mobile where there is a mobile-optimized alternative that is > served by default? The point is, that many sites don't have mobile versions. For example apple.com (perhaps surprisingly) serves a desktop site to phones, with a <meta name="viewport" content="width=1024"> viewport. It would be incorrect to assume they don't want to be treated as a desktop site. For example, it would be incorrect to disable double-tap zoom just because they have that viewport tag. Similarly, it doesn't make sense to blanket disable Text Autosizing on apple.com.
Message was sent while issue was closed.
On 2013/11/11 14:29:45, johnme wrote: > The point is, that many sites don't have mobile versions. For example http://apple.com > (perhaps surprisingly) serves a desktop site to phones, with a <meta > name="viewport" content="width=1024"> viewport. It would be incorrect to assume > they don't want to be treated as a desktop site. For example, it would be > incorrect to disable double-tap zoom just because they have that viewport tag. > Similarly, it doesn't make sense to blanket disable Text Autosizing on > http://apple.com. Ok, I am convinced. I will send you a patch to fix the logic in clusterMultipler. The most important aspect of the change is to not autosize for width=device-width which I think we are in agreement on.
Message was sent while issue was closed.
On 2013/11/11 19:29:28, skobes wrote: > On 2013/11/11 14:29:45, johnme wrote: > > The point is, that many sites don't have mobile versions. For example > http://apple.com > > (perhaps surprisingly) serves a desktop site to phones, with a <meta > > name="viewport" content="width=1024"> viewport. It would be incorrect to > assume > > they don't want to be treated as a desktop site. For example, it would be > > incorrect to disable double-tap zoom just because they have that viewport tag. > > Similarly, it doesn't make sense to blanket disable Text Autosizing on > > http://apple.com. > > Ok, I am convinced. I will send you a patch to fix the logic in > clusterMultipler. > > The most important aspect of the change is to not autosize for > width=device-width which I think we are in agreement on. Actually the consequences will be even stranger than I initially thought. If the threshold for "being a desktop site" is the device width, which varies with orientation, on N10 we will autosize in portrait mode but not landscape mode, which seems very wrong. Do you think we should base it on min(device width, device height), like the logic for computing the fudge factor? Or something else?
Message was sent while issue was closed.
On 2013/11/12 19:33:13, skobes wrote: > If the threshold for "being a desktop site" is the device width, which varies with > orientation, on N10 we will autosize in portrait mode but not landscape mode, > which seems very wrong. That's actually the optimal behaviour. Portrait N10 is a narrow screen device. Landscape N10 is not. Therefore only portrait should be autosized. Similarly when you rotate your phone, we Autosize more/less accordingly. The funny thing is, this actually looks more consistent to users! Note that on a portrait N10, your typical desktop site will be zoomed out to 800/980. Text Autosizing will then multiply body text font sizes by up to 980/800. The end result, is that when you rotate from landscape to portrait, the main body text will often appear to stay roughly the same size, merely getting wrapped (whereas if we hadn't autosized it, the font size would appear to decrease due to the zoom out ).
Message was sent while issue was closed.
On 2013/11/12 20:11:30, johnme wrote: > On 2013/11/12 19:33:13, skobes wrote: > > If the threshold for "being a desktop site" is the device width, which varies > with > > orientation, on N10 we will autosize in portrait mode but not landscape mode, > > which seems very wrong. s/autosize/apply the fudge factor/ > The funny thing is, this actually looks more consistent to users! Note that on a > portrait N10, your typical desktop site will be zoomed out to 800/980. Text > Autosizing will then multiply body text font sizes by up to 980/800. Yes it would be consistent if there were no fudge factor. But if we selectively apply the fudge factor for "desktop sites" defined as wider than device, it becomes inconsistent - the text will look larger in portrait mode even after accounting for the initial zoom level.
Message was sent while issue was closed.
On 2013/11/12 21:13:38, skobes wrote: > s/autosize/apply the fudge factor/ Ooh! Though I still don't get your meaning. We will apply the fudge factor on both portrait and landscape N10; it's just that in landscape the fudge factor gets applied by reducing the layout width and uniformly scaling the site, whereas in portrait the fudge factor gets applied by performing Text Autosizing. But the end result is that autosized text will look the same size in both cases.
Message was sent while issue was closed.
On 2013/11/12 22:38:41, johnme wrote: > On 2013/11/12 21:13:38, skobes wrote: > > s/autosize/apply the fudge factor/ > > Ooh! Though I still don't get your meaning. We will apply the fudge factor on > both portrait and landscape N10; it's just that in landscape the fudge factor > gets applied by reducing the layout width and uniformly scaling the site, > whereas in portrait the fudge factor gets applied by performing Text Autosizing. > But the end result is that autosized text will look the same size in both cases. The intent of THIS patch was to stop applying the fudge factor to mobile websites. The intent of https://codereview.chromium.org/61593006/ was, for websites where we DO apply the fudge factor, to change the manner in which it is applied, by applying it to layout width instead of text autosizing. If a website with no meta viewport on a landscape N10 is considered a "mobile website", then https://codereview.chromium.org/61593006/ becomes a nop, because the only scenario in which we have room to reduce the layout width will have been reclassified as one in which we don't want to apply the fudge factor. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
