Bug 219996 - Web Inspector: Font Details sidebar - Improve visibility of values by emphasizing them/de-emphasizing range information
Summary: Web Inspector: Font Details sidebar - Improve visibility of values by emphasi...
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: 2020-12-17 14:51 PST by Patrick Angle
Modified: 2021-01-08 15:20 PST (History)
5 users (show)

See Also:


Attachments
Screenshot of Issue (2.48 MB, image/png)
2020-12-17 14:51 PST, Patrick Angle
no flags Details
Patch v1.0 (7.86 KB, patch)
2021-01-07 09:35 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.0 (70.59 KB, image/png)
2021-01-07 09:35 PST, Patrick Angle
no flags Details
Patch v1.1 - Review Notes (7.47 KB, patch)
2021-01-08 11:46 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.1 (2.54 MB, image/png)
2021-01-08 11:51 PST, Patrick Angle
no flags Details
Patch v1.2 - Review Notes (7.46 KB, patch)
2021-01-08 13: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 2020-12-17 14:51:58 PST
Created attachment 416472 [details]
Screenshot of Issue

In the Font details sidebar values, particularly for variation axes, are difficult to read quickly. We should either emphasize the actual value, de-emphasize the range/default information, or both.
Comment 1 Radar WebKit Bug Importer 2020-12-17 14:55:19 PST
<rdar://problem/72442309>
Comment 2 Patrick Angle 2021-01-07 09:35:20 PST
Created attachment 417183 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-01-07 09:35:43 PST
Created attachment 417184 [details]
Screenshot of Patch v1.0
Comment 4 Devin Rousso 2021-01-07 10:38:02 PST
Comment on attachment 417183 [details]
Patch v1.0

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

r=me, with a few fixes/questions :)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:26
> +.font > .details-section > .content > .group > .row.simple > .value .secondary {

Starting with `.font` is super generic.  Please change it to `.sidebar > .panel.details.font` to be more specific (and match other details sidebar panels).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:28
> +    word-break: normal;

Is this necessary?  IIRC the default value of `word-break` is `normal`.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:180
> +        let secondaryElement = document.createElement("span");

NIT: you can combine this and 183 to be one line
```
    let secondaryElement = valueElement.appendChild(document.createElement("span"));
```

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:182
> +        secondaryElement.textContent = WI.UIString(" (Range: %d-%d)", " (Range: %d-%d) @ Font Details Sidebar", "A range for a single variation axis of a font.").format(minimumValue, maximumValue);

Do we no longer care about the `defaultValue`?
Comment 5 Patrick Angle 2021-01-07 11:18:28 PST
Comment on attachment 417183 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:28
>> +    word-break: normal;
> 
> Is this necessary?  IIRC the default value of `word-break` is `normal`.

`.details-section > .content > .group > .row.simple > .value` defines `word-break:break-all`, which looks odd here since this isn't just a number or code.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:182
>> +        secondaryElement.textContent = WI.UIString(" (Range: %d-%d)", " (Range: %d-%d) @ Font Details Sidebar", "A range for a single variation axis of a font.").format(minimumValue, maximumValue);
> 
> Do we no longer care about the `defaultValue`?

For weight/stretch the default won't be the default that is used anyways since CSS defines its own defaults there (I haven't noticed any fonts that define odd defaults for these yet, either), and for other variation axes the default will be shown as the value unless the developer has defined their own value for the axis, so this information becomes more or less redundant.
Comment 6 Patrick Angle 2021-01-08 11:46:58 PST
Created attachment 417283 [details]
Patch v1.1 - Review Notes
Comment 7 Patrick Angle 2021-01-08 11:51:48 PST
Created attachment 417284 [details]
Screenshot of Patch v1.1
Comment 8 EWS 2021-01-08 13:02:17 PST
ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.
Comment 9 Patrick Angle 2021-01-08 13:05:41 PST
Created attachment 417294 [details]
Patch v1.2 - Review Notes
Comment 10 EWS 2021-01-08 15:19:59 PST
Committed r271329: <https://trac.webkit.org/changeset/271329>

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