Continue filling in the list of FontFeatures #23730

Hixie · 2021-01-16T00:13:54Z

This adds support and documentation for:

FontFeature.alternative(n)
FontFeature.alternativeFractions()
FontFeature.contextualAlternates()
FontFeature.caseSensitiveForms()
FontFeature.characterVariant(v)
FontFeature.denominator()
FontFeature.fractions()
FontFeature.historicalForms()
FontFeature.historicalLigatures()
FontFeature.liningFigures()
FontFeature.localeAware(enable: false)
FontFeature.notationalForms(v)
FontFeature.numerators()
FontFeature.oldstyleFigures() (documentation)
FontFeature.ordinalForms()
FontFeature.proportionalFigures() (documentation)
FontFeature.randomize() (minor additions to documentation)
FontFeature.stylisticAlternates()
FontFeature.scientificInferiors()
FontFeature.stylisticSet(n) (documentation)
FontFeature.subscripts()
FontFeature.superscripts()
FontFeature.swash(v)
FontFeature.tabularFigures() (documentation)
FontFeature.slashedZero() (documentation)

There's hundreds more we could add but this is what I have so far.

See flutter/assets-for-api-docs#131 for the diagrams.

Hixie · 2021-01-24T21:19:00Z

cc @GaryQian this is the PR that uses those FontFeature diagrams

dnfield · 2021-01-26T22:45:49Z

lib/ui/text.dart

+    this.feature,
+    [ this.value = 1 ]
+  ) : assert(feature != null), // ignore: unnecessary_null_comparison
+      assert(feature.length == 4, 'Feature tag must be exactly four characters long.'),


These are nice to add, but I'm pretty sure they just show up as <optimized out> to users when thrown from dart:ui. I'm not sure why and whether that's on us or on Dart, or what the cost of fixing it would be.

Is it the messages or the expressions that are optimized out? Any idea?

Just messages

dnfield

I just learned so much about font features.

This LGTM - the only non-nit in here is the incorrect link for swsh

dnfield · 2021-01-26T22:56:09Z

lib/ui/text.dart

+  // Features listed below are those we deemed "interesting enough" to
+  // have their own constructor, mostly on the basis of whether we
+  // could find a font where the feature had a useful effect that
+  // could be demonstrated...


Suggested change

// could be demonstrated...

// could be demonstrated.

(alternatively, ...., but I don't see a good reason to trail off here)

haha that was literally me trailing off when explaining my dubious criteria for inclusion :-)
fixed

dnfield · 2021-01-26T23:11:16Z

lib/ui/text.dart

  ///
  /// See also:
  ///
-  ///  * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_pt#pnum>
-  const FontFeature.proportionalFigures() : feature = 'pnum', value = 1;
+  ///  * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#calt>


Suggested change

/// * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#calt>

/// * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_pt#swsh>

good catch!

dnfield · 2021-01-26T23:12:29Z

lib/ui/text.dart

+  // swsh
+  /// Enable swash glyphs. (`swsh`)
+  ///
+  /// Some fonts have beautiful flourishes on some characters. This


Can we be a little more descriptive on this line? Beautiful flourishes sounds nice but a bit vague. The image really helps me here (and elsewhere too, they're great!), but maybe we could describe this as something like This feature, when enabled, draws flourishes that continue the ending stroke of a letter.? I'm not sure if it applies to beginning as well though.

I've added more text explaining the concept, and a link to wikipedia.

dnfield · 2021-01-26T23:14:27Z

lib/ui/text.dart

  final String feature;

  /// The value assigned to this feature.
  ///
-  /// Must be a positive integer.  Many features are Boolean values that accept
+  /// Must be a positive integer. Many features are Boolean values that accept
  /// values of either 0 (feature is disabled) or 1 (feature is enabled).


nit: might want to mention here that other features use a bound range of acceptable values, and they're documented here for the ones we document or elsewhere for the ones we don't.

added more color

dnfield · 2021-01-26T23:15:21Z

lib/ui/text.dart

@@ -312,7 +1431,7 @@ class FontFeature {
  int get hashCode => hashValues(feature, value);

  @override
-  String toString() => 'FontFeature($feature, $value)';
+  String toString() => 'FontFeature(\'$feature\', $value)';


nit: consider either double quoting the outer or inner string rather than escaping.

changed outer quotes to ".

I'm pretty mixed on this. I kinda like the consistency of always using ', and don't mind the \ so much. I find it confusing to have mixed quoting styles and it gets particularly annoying when dealing with copy/paste. But I also like avoiding escapes to some extent, it does look a bit cleaner. I dunno.

dnfield · 2021-01-26T23:16:06Z

lib/web_ui/lib/src/ui/text.dart

@@ -115,7 +127,7 @@ class FontFeature {
  int get hashCode => hashValues(feature, value);

  @override
-  String toString() => 'FontFeature($feature, $value)';
+  String toString() => 'FontFeature(\'$feature\', $value)';


ditto (nit)

This adds support and documentation for: * FontFeature.alternative(n) * FontFeature.alternativeFractions() * FontFeature.contextualAlternates() * FontFeature.caseSensitiveForms() * FontFeature.characterVariant(v) * FontFeature.denominator() * FontFeature.fractions() * FontFeature.historicalForms() * FontFeature.historicalLigatures() * FontFeature.liningFigures() * FontFeature.localeAware(enable: false) * FontFeature.notationalForms(v) * FontFeature.numerators() * FontFeature.oldstyleFigures() * FontFeature.ordinalForms() * FontFeature.proportionalFigures() * FontFeature.randomize() (minor additions to documentation) * FontFeature.stylisticAlternates() * FontFeature.scientificInferiors() * FontFeature.stylisticSet(n) (documentation) * FontFeature.subscripts() * FontFeature.superscripts() * FontFeature.swash(v) * FontFeature.tabularFigures() (documentation) * FontFeature.slashedZero() There's hundreds more we could add but this is what I have so far. See flutter/assets-for-api-docs#131 for the diagrams.

Hixie · 2021-01-26T23:39:27Z

Thanks for the review; applied all suggested changes. Will land on green.

fluttergithubbot · 2021-01-27T05:39:01Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…23730)

google-cla bot added the cla: yes label Jan 16, 2021

Hixie mentioned this pull request Jan 16, 2021

FontFeature diagrams flutter/assets-for-api-docs#131

Merged

Hixie force-pushed the font_feature branch 5 times, most recently from 8dbb697 to c346cc5 Compare January 24, 2021 06:21

Hixie requested a review from GaryQian January 24, 2021 21:19

dnfield reviewed Jan 26, 2021

View reviewed changes

dnfield approved these changes Jan 26, 2021

View reviewed changes

Hixie force-pushed the font_feature branch from c346cc5 to 07b2186 Compare January 26, 2021 23:39

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 27, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 27, 2021

Hixie added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 27, 2021

fluttergithubbot merged commit 0540bc8 into flutter:master Jan 27, 2021

engine-flutter-autoroll mentioned this pull request Jan 27, 2021

Roll Engine from 7ffca994cc25 to 63d5fa8207f6 (13 revisions) flutter/flutter#74845

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2021

0540bc8 Continue filling in the list of FontFeatures (flutter/engine#…

4355fa4

…23730)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Continue filling in the list of FontFeatures (flutter#23730)

b20ca16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continue filling in the list of FontFeatures #23730

Continue filling in the list of FontFeatures #23730

Hixie commented Jan 16, 2021 •

edited

Loading

Hixie commented Jan 24, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

dnfield left a comment

dnfield Jan 26, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

Hixie Jan 26, 2021

dnfield Jan 26, 2021

Hixie commented Jan 26, 2021

fluttergithubbot commented Jan 27, 2021

	/// * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_ae#calt>
	/// * <https://docs.microsoft.com/en-us/typography/opentype/spec/features_pt#swsh>

Continue filling in the list of FontFeatures #23730

Continue filling in the list of FontFeatures #23730

Conversation

Hixie commented Jan 16, 2021 • edited Loading

Hixie commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented Jan 26, 2021

fluttergithubbot commented Jan 27, 2021

Hixie commented Jan 16, 2021 •

edited

Loading