Refactor how font scale factor appears in the emulation panel

Refactor how font scale factor appears in the emulation panel

This patch refactors the confusing font scale factor setting. A
screenshot is available at http://pr.gg/asnew.png

Changes included in this patch:
1) Added updated tooltips for DPR, text autosizing, and the use android
   text metrics checkbox. This patch also moves the tooltips
   to be on both the input element and the associated label.
2) Removes the font scale factor textbox. Instead of manually entering
   this value, it will automatically be calculated if the new
   "Use Android font metrics" checkbox is checked.
3) Adds a new "Use Android font metrics" checkbox and auto-check it
   based on which emulation preset is chosen. When text autosizing
   is disabled, this checkbox will be disabled.
4) Minor refactoring of metrics.fontScaleFactor() which now computes
   the factor on-demand (it's cheap).

Some usecases:
If an iOS device is emulated, the text autosizing box will be checked
but the android font metrics box will not.

If an Android device is emulated, both checkboxes are checked.

If a custom device is being emulated, the user can choose whether to
use android's font metrics.

BUG=319092
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162250

[dgozman, 2013-11-15 09:40:15]

I'd prefer "Use Android font metrics" checkbox to be below "Enable test
autosizing" with an indent, like we use in other places for dependent settings
(example: Settings->General->Rendering->Force accelerated compositing).

Everything else LGTM, but I'm not an owner.

[skobes, 2013-11-15 19:30:39]

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesSupport.js (right):

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesSupport.js:204: * Android uses a font scale
factor for text autosizing that is fudged for improved
Note that:

(1) I renamed the fudge factor to "device scale adjustment" so that it is more
clearly a separate entity from the font scale factor (otherwise "font scale
factor" means two different things depending on what layer of the code you are
in)

(2) Currently we only apply it when there is no meta viewport, though there is
an unresolved discussion on https://codereview.chromium.org/49913005/ about
changing that.  Not sure how closely you want to emulate that behavior in the
inspector.

[pdr., 2013-11-15 23:13:16]

Updated the checkboxes to not be in the table and used indentation for the
android font metrics setting.

@pfeldman, lgty?

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesSupport.js (right):

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesSupport.js:204: * Android uses a font scale
factor for text autosizing that is fudged for improved
On 2013/11/15 19:30:40, skobes wrote:
> Note that:
> 
> (1) I renamed the fudge factor to "device scale adjustment" so that it is more
> clearly a separate entity from the font scale factor (otherwise "font scale
> factor" means two different things depending on what layer of the code you are
> in)

I'll update this wording in the comment. The rename from font scale factor to
device scale adjustment will require a two-sided patch, so lets do that in
another patch :)

> 
> (2) Currently we only apply it when there is no meta viewport, though there is
> an unresolved discussion on https://codereview.chromium.org/49913005/ about
> changing that.  Not sure how closely you want to emulate that behavior in the
> inspector.

My understanding is that we'll use your updated code in TextAutosizer.cpp. The
inspector just enables that codepath during emulation.

[skobes, 2013-11-15 23:26:03]

lgtm

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
File Source/devtools/front_end/OverridesSupport.js (right):

https://codereview.chromium.org/73203003/diff/1/Source/devtools/front_end/Ove...
Source/devtools/front_end/OverridesSupport.js:204: * Android uses a font scale
factor for text autosizing that is fudged for improved
On 2013/11/15 23:13:16, pdr wrote:
> My understanding is that we'll use your updated code in TextAutosizer.cpp. The
> inspector just enables that codepath during emulation.

Are you sure?  It looks like the inspector hooks into
Settings::textAutosizingFontScaleFactor but not Settings::deviceScaleAdjustment.

[paulirish, 2013-11-16 00:59:05]

I think there's two risks with the word "Android" here in the UI:

1) While it's all good intentions it kinda looks like a google product (chrome)
is baking in extra special emulation support for android and not iOS.
2) The box will be checked on while you're emulating an iPhone, which is just
kinda WTF to users.

I was going to say let's rename it, but honestly shouldn't we just kill the
checkbox, and keep it always on?

Text autosizing is already all sorts of magic that developers don't understand
and can't affect (when it's on). I can't think of a use case where toggling the
font scale factor would be useful.


Regardless, thanks mucho for the patch.

[pdr., 2013-11-16 04:15:00]

On 2013/11/15 23:26:03, skobes wrote:
> > My understanding is that we'll use your updated code in TextAutosizer.cpp.
The
> > inspector just enables that codepath during emulation.
> 
> Are you sure?  It looks like the inspector hooks into
> Settings::textAutosizingFontScaleFactor but not
Settings::deviceScaleAdjustment.

Ack, you're right. This is confusing. I've filed crbug.com/320193 to either
unify these or to add some comments explaining the differences.


On 2013/11/16 00:59:05, paulirish wrote:
> I think there's two risks with the word "Android" here in the UI:
> 
> 1) While it's all good intentions it kinda looks like a google product
(chrome)
> is baking in extra special emulation support for android and not iOS.
> 2) The box will be checked on while you're emulating an iPhone, which is just
> kinda WTF to users.
> 
> I was going to say let's rename it, but honestly shouldn't we just kill the
> checkbox, and keep it always on?
> 
> Text autosizing is already all sorts of magic that developers don't understand
> and can't affect (when it's on). I can't think of a use case where toggling
the
> font scale factor would be useful.
> 
> 
> Regardless, thanks mucho for the patch.

Oh no, this isn't about treating Android special, it's more about getting the
OS-specific quirks right.

One way to think about this is that we have two autosizers: one for iOs and one
for Android. The "use Android font metrics" checkbox is really about switching
our autosizer between "iOS autosizing" and "Android autosizing" modes. When
emulating a device, we automatically check the appropriate boxes so 99% of users
will never have to think about this.

Does that explanation help or do you still have concerns?


One question you may get from users is that text autosizing can appear to do
nothing in "iOS mode" (when "use Android font metrics" is unchecked). iOS is
less aggressive with their text autosizer which leads to this behavior :/

[pfeldman, 2013-11-16 12:59:08]

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
File Source/devtools/front_end/OverridesView.js (right):

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:513:
this._useAndroidFontMetricsCheckbox.disabled = !metrics.textAutosizing;
Do we want these settings to be persisted when you use them off Screen tab?

If so, you could create _useAndroidFontMetricsCheckbox within a fieldset that is
controlled by the new textAutosizing setting (declare one in Settings.js beside
emulateViewport). Like this:

var fieldsetElement =
WebInspector.SettingsTab.createSettingFieldset(WebInspector.settings.emulateTextAutosizing);

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:567: cellElement.title = label;
inline

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:570:
this._deviceScaleFactorOverrideElement.title = label;
Here and below: you don't need this, input belongs to label, so label should be
the only one to have title.

[pdr., 2013-11-16 23:47:53]

Thanks for the review.

I've updated the change description and screenshot (http://pr.gg/asnew.png).

PTAL

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
File Source/devtools/front_end/OverridesView.js (right):

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:513:
this._useAndroidFontMetricsCheckbox.disabled = !metrics.textAutosizing;
On 2013/11/16 12:59:09, pfeldman wrote:
> Do we want these settings to be persisted when you use them off Screen tab?
> 
> If so, you could create _useAndroidFontMetricsCheckbox within a fieldset that
is
> controlled by the new textAutosizing setting (declare one in Settings.js
beside
> emulateViewport). Like this:
> 
> var fieldsetElement =
>
WebInspector.SettingsTab.createSettingFieldset(WebInspector.settings.emulateTextAutosizing);

I'm not sure this is needed because the setting is already persisted as part of
device metrics.

The way I was setting this._useAndroidFontMetricsCheckbox.disabled was pretty
confusing though so I updated it to be bound to
metrics.isUseAndroidFontMetricsDisabled().

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:567: cellElement.title = label;
On 2013/11/16 12:59:09, pfeldman wrote:
> inline

Done.

https://codereview.chromium.org/73203003/diff/80001/Source/devtools/front_end...
Source/devtools/front_end/OverridesView.js:570:
this._deviceScaleFactorOverrideElement.title = label;
On 2013/11/16 12:59:09, pfeldman wrote:
> Here and below: you don't need this, input belongs to label, so label should
be
> the only one to have title.

Done.

I put the title on the <tr> for device scale factor element so the tooltip
appears when you hover over the label or the input box. For the text autosizing
checkbox and the android font metrics field, the title is now just on the
outermost element.

[pfeldman, 2013-11-18 14:45:08]

https://codereview.chromium.org/73203003/diff/150001/Source/devtools/front_en...
File Source/devtools/front_end/OverridesView.js (right):

https://codereview.chromium.org/73203003/diff/150001/Source/devtools/front_en...
Source/devtools/front_end/OverridesView.js:572:
this._textAutosizingOverrideCheckbox =
textAutosizingOverrideElement.getElementsByTagName("input")[0];
nit: this is no better than textAutosizingOverrideElement.firstChild

https://codereview.chromium.org/73203003/diff/150001/Source/devtools/front_en...
Source/devtools/front_end/OverridesView.js:613: if
(this._useAndroidFontMetricsCheckbox.disabled !==
metrics.isUseAndroidFontMetricsDisabled())
The disabled state of this checkbox needs to be controlled by the state of
this._textAutosizingOverrideCheckbox, not the value in metrics. I think there is
a slight misunderstanding of what "Emulate" does with the given metrics: it just
applies predefined set of values to the emulation settings. User still needs to
be able to set up all these settings manually. If you don't want some of these
settings to be persisted, that is fine.

[pfeldman, 2013-11-18 19:14:55]

lgtm

[commit-bot: I haz the power, 2013-11-18 23:32:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/73203003/220001

[commit-bot: I haz the power, 2013-11-19 00:48:25]

Change committed as 162250