Bug 220474 - Web Inspector: Font Details sidebar - Fractional variation axis ranges/default values are rounded.
Summary: Web Inspector: Font Details sidebar - Fractional variation axis ranges/defaul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-08 12:37 PST by Patrick Angle
Modified: 2021-01-19 14:55 PST (History)
6 users (show)

See Also:


Attachments
Screenshot before patch (1.33 MB, image/png)
2021-01-08 12:37 PST, Patrick Angle
no flags Details
Patch v1.0 (8.77 KB, patch)
2021-01-14 14:27 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.0 (69.41 KB, image/png)
2021-01-14 14:28 PST, Patrick Angle
no flags Details
Patch v1.5 - Revised approach for converting fractions to strings (9.60 KB, patch)
2021-01-15 08:16 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.6 - Review Notes (9.88 KB, patch)
2021-01-19 14:05 PST, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-01-08 12:37:42 PST
Created attachment 417291 [details]
Screenshot before patch

Inspect the font sample on: https://v-fonts.com/fonts/skia. Skia’s `wght` and `wdth` registered axes have ranges of 0.47–3.19 and 0.619–1.3, respectively. They are instead listed as 0-3 and 0-1.
Comment 1 Radar WebKit Bug Importer 2021-01-08 12:39:14 PST
<rdar://problem/72940358>
Comment 2 Patrick Angle 2021-01-14 14:27:47 PST
Created attachment 417654 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-01-14 14:28:12 PST
Created attachment 417656 [details]
Screenshot of Patch v1.0
Comment 4 Devin Rousso 2021-01-14 14:41:29 PST
Comment on attachment 417654 [details]
Patch v1.0

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

r=me

> Source/WebInspectorUI/ChangeLog:14
> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.

Are we sure that localizers will know what this is?

Aside: I wonder if we actually want to use this instead of `f` more often 🤔

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1096
> +        g: function(substitution, token)

Please add some tests for this :)

IIRC there's tests for other stuff in `LayoutTests/inspector/unit-tests/string-utilities.html`.

Style: you should be able to `g(substitution, token)` instead of having the `: function` :)
Comment 5 Patrick Angle 2021-01-14 14:45:46 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:14
>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
> 
> Are we sure that localizers will know what this is?
> 
> Aside: I wonder if we actually want to use this instead of `f` more often 🤔

I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.
Comment 6 Devin Rousso 2021-01-14 14:47:44 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/ChangeLog:14
>>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
>> 
>> Are we sure that localizers will know what this is?
>> 
>> Aside: I wonder if we actually want to use this instead of `f` more often 🤔
> 
> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.

I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved?  Perhaps Greg or someone working on Safari knows who to contact.
Comment 7 Patrick Angle 2021-01-14 15:01:46 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>>>> Source/WebInspectorUI/ChangeLog:14
>>>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
>>> 
>>> Are we sure that localizers will know what this is?
>>> 
>>> Aside: I wonder if we actually want to use this instead of `f` more often 🤔
>> 
>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.
> 
> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved?  Perhaps Greg or someone working on Safari knows who to contact.

In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. 😔

This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts?
Comment 8 Devin Rousso 2021-01-14 15:06:23 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>>>>> Source/WebInspectorUI/ChangeLog:14
>>>>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
>>>> 
>>>> Are we sure that localizers will know what this is?
>>>> 
>>>> Aside: I wonder if we actually want to use this instead of `f` more often 🤔
>>> 
>>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.
>> 
>> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved?  Perhaps Greg or someone working on Safari knows who to contact.
> 
> In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. 😔
> 
> This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts?

Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead?
Comment 9 Patrick Angle 2021-01-14 15:26:40 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>>>>>> Source/WebInspectorUI/ChangeLog:14
>>>>>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
>>>>> 
>>>>> Are we sure that localizers will know what this is?
>>>>> 
>>>>> Aside: I wonder if we actually want to use this instead of `f` more often 🤔
>>>> 
>>>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.
>>> 
>>> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved?  Perhaps Greg or someone working on Safari knows who to contact.
>> 
>> In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. 😔
>> 
>> This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts?
> 
> Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead?

I don't think that'll work here, since the decimal point needs to be localized still, and `maxDecimals` just rounds the number off. We would need to pass through `toLocaleString` at some point, with a `minimumFractionDigits` of `0` and a `maximumFractionDigits` of `2`, at which point the rounding wasn't necessary.
Comment 10 Devin Rousso 2021-01-14 15:33:09 PST
Comment on attachment 417654 [details]
Patch v1.0

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

>>>>>>> Source/WebInspectorUI/ChangeLog:14
>>>>>>> +        - Add basic support for IEEE printf `g` which truncates trailing zeros in the fractional portion of the number.
>>>>>> 
>>>>>> Are we sure that localizers will know what this is?
>>>>>> 
>>>>>> Aside: I wonder if we actually want to use this instead of `f` more often 🤔
>>>>> 
>>>>> I'm not sure. Should I just note this in the string descriptions? I'm not sure the best way to communicate this otherwise.
>>>> 
>>>> I'd look at some of our older "fix a localization bug" changes and maybe email some of the people involved?  Perhaps Greg or someone working on Safari knows who to contact.
>>> 
>>> In looking this up in our own docs (https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html) which in turn references (http://www.opengroup.org/onlinepubs/009695399/functions/printf.html), I'm surprised to find that the behavior I've always taken for granted with `g` is actually a side effect, and the main purpose is actually to selectively use exponential form. There doesn't appear to be a clear analogue to the behavior I'm trying to achieve in `printf` parlance. 😔
>>> 
>>> This leads me down a path where maybe it makes more sense to have a utility function on Number (e.g. `Number.decimalString(fraction, maximumPrecision, minimumPrecision)` or similar?) for this functionality. This patch is one of those cases where for 90%+ of fonts I don't expect any fractions, and the clutter makes it more difficult to read the values at a glance. Thoughts?
>> 
>> Maybe just use `Number.maxDecimals` (another one of our utilities) and `%s` instead?
> 
> I don't think that'll work here, since the decimal point needs to be localized still, and `maxDecimals` just rounds the number off. We would need to pass through `toLocaleString` at some point, with a `minimumFractionDigits` of `0` and a `maximumFractionDigits` of `2`, at which point the rounding wasn't necessary.

Well if need be we could just have this be a special case and call `.toLocaleString(undefined, {minimumFractionDigits: 0, maximumFractionDigits: 2})` where needed in `Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js` (maybe even make a private `_formatVariationValue`).
Comment 11 Patrick Angle 2021-01-15 08:16:58 PST
Created attachment 417701 [details]
Patch v1.5 - Revised approach for converting fractions to strings
Comment 12 BJ Burg 2021-01-19 10:15:08 PST
Comment on attachment 417701 [details]
Patch v1.5 - Revised approach for converting fractions to strings

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

r=me

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:5
> +localizedStrings[" (Range: %s-%s, Default: %s) @ Font Details Sidebar"] = " (Range: %s-%s, Default: %s)";

Hopefully localizers can figure out the context based on the string.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:78
> +localizedStrings["%s%%"] = "%s%%";

It would be wise to explain what this is with a localization comment.
Comment 13 Patrick Angle 2021-01-19 14:05:32 PST
Created attachment 417910 [details]
Patch v1.6 - Review Notes
Comment 14 EWS 2021-01-19 14:55:32 PST
Committed r271620: <https://trac.webkit.org/changeset/271620>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417910 [details].