Bug 148720 - Web Inspector: Add font-variant-* to the visual styles sidebar
Summary: Web Inspector: Add font-variant-* to the visual styles sidebar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-02 13:06 PDT by Timothy Hatcher
Modified: 2016-01-29 10:00 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.78 KB, patch)
2016-01-28 14:49 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.87 KB, patch)
2016-01-29 02:14 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-09-02 13:06:14 PDT
font-feature-settings is very cryptic, and the visual sidebar would be helpful.
Comment 1 Myles C. Maxfield 2015-09-03 15:48:11 PDT
No no no no :(

Please do not do this.

Instead, please add font-variant-* to the sidebar.
Comment 2 Radar WebKit Bug Importer 2015-09-03 17:53:06 PDT
<rdar://problem/22569974>
Comment 3 Devin Rousso 2015-09-08 14:45:29 PDT
So the only font-variant-* that I could find in CSSPropertyNames.in are:

font-variant (already implemented with "small-caps")
-webkit-font-variant-ligatures

I do know that the spec calls for many more than that, but since they aren't implemented yet (as far as I can tell), should we only add these two?


@Myles C. Maxfield <mmaxfield@apple.com>,
Just curious, but why is it that you don't want to implement font-feature-settings?
Comment 4 Myles C. Maxfield 2015-09-08 14:53:16 PDT
Correct, I have not implemented font-variant-* yet.

font-variant-* is the preferred API. It isn't font file format-specific, and provides synthesis for certain properties if the feature is unavailable. It conveys the authors intentions more clearly.
Comment 5 Devin Rousso 2016-01-27 15:43:50 PST
So now that <https://webkit.org/blog/5735/css-font-features/> is live, I am assuming that font-variant-* is implemented enough to allow this to move forward?  If so, should I just implement all of them, or are there a specific few that are more important?
Comment 6 Myles C. Maxfield 2016-01-27 15:57:04 PST
(In reply to comment #5)
> So now that <https://webkit.org/blog/5735/css-font-features/> is live, I am
> assuming that font-variant-* is implemented enough to allow this to move
> forward?  If so, should I just implement all of them, or are there a
> specific few that are more important?

Yes!

All the values are similar enough that I don't think it's worth discriminating important from not important; just doing them all in one fell swoop should be fairly straightforward.
Comment 7 Devin Rousso 2016-01-28 14:49:04 PST
Created attachment 270148 [details]
Patch
Comment 8 Timothy Hatcher 2016-01-28 16:24:07 PST
Comment on attachment 270148 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:748
> +        properties.fontVariantNumeric = new WebInspector.VisualStyleKeywordPicker("font-variant-numeric", WebInspector.UIString("Numeric"), this._keywords.normal.concat(["None", "Ordinal", "Slashed Zero", "Lining Nums", "Oddstyle Nums", "Proportional Nums", "Tabular Nums", "Diagonal Fractions", "Stacked Fractions"]));

Oldstyle Nums.

The spec seems to allow multiple values for one property. That isn't possible with a popup menu. That complicates things, hmm.
Comment 9 Devin Rousso 2016-01-28 17:57:55 PST
Comment on attachment 270148 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:748
>> +        properties.fontVariantNumeric = new WebInspector.VisualStyleKeywordPicker("font-variant-numeric", WebInspector.UIString("Numeric"), this._keywords.normal.concat(["None", "Ordinal", "Slashed Zero", "Lining Nums", "Oddstyle Nums", "Proportional Nums", "Tabular Nums", "Diagonal Fractions", "Stacked Fractions"]));
> 
> Oldstyle Nums.
> 
> The spec seems to allow multiple values for one property. That isn't possible with a popup menu. That complicates things, hmm.

I actually also noticed this, but I wasn't sure how to approach the idea of allowing multiple.  The most straightforward idea I had was to do some sort of "&" option (like "Lining & Proportional Nums"), but the number of combinations could potentially be large).  The other idea I came up with was to create a modified form of the VisualStyleCommaSeparatedKeywordEditor but for a token list instead (which would allow for easier autocompletion), but this also seemed to be excessive for something that rarely has more than two values.

Since I wasn't sure how to move forward, I figured that I would add something basic that encompassed the majority of cases in the meantime while some other option was considered (similar to the VisualStyleBasicInput I used for the content and font-variant-alternates properties, since each has special functions that are hard to add cases for).
Comment 10 Timothy Hatcher 2016-01-28 21:31:12 PST
Okay. Fix the typo and add a FIXME, and we can land it.

An option would be to allow multiple popups to be side by side, maybe one popup and when it has a value show a plus button that adds another popup.
Comment 11 Devin Rousso 2016-01-29 02:14:05 PST
Created attachment 270195 [details]
Patch
Comment 12 WebKit Commit Bot 2016-01-29 10:00:13 PST
Comment on attachment 270195 [details]
Patch

Clearing flags on attachment: 270195

Committed r195819: <http://trac.webkit.org/changeset/195819>
Comment 13 WebKit Commit Bot 2016-01-29 10:00:17 PST
All reviewed patches have been landed.  Closing bug.