Bug 168889 – Expand font-weight and font-stretch to take any number

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

Letting font-weight take any arbitrary number is important for font variations.

[Jon Lee]

rdar://problem/30429792

[Myles C. Maxfield]

Created attachment 303420 [details]
WIP

[Myles C. Maxfield]

*** Bug 168890 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 303443 [details]
WIP

[Myles C. Maxfield]

Created attachment 303445 [details]
WIP

[Myles C. Maxfield]

Created attachment 303471 [details]
WIP

[Myles C. Maxfield]

Created attachment 303477 [details]
WIP

[WebKit Commit Bot]

Attachment 303477 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 303482 [details]
WIP

[Myles C. Maxfield]

Created attachment 303494 [details]
Patch

[Myles C. Maxfield]

Created attachment 303495 [details]
Patch

[Myles C. Maxfield]

Created attachment 303503 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 303503 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2571
> +    else if (weight == FontSelectionValue(100)
> +        || weight == FontSelectionValue(200)
> +        || weight == FontSelectionValue(300)
> +        || weight == FontSelectionValue(400)
> +        || weight == FontSelectionValue(500)
> +        || weight == FontSelectionValue(600)
> +        || weight == FontSelectionValue(700)
> +        || weight == FontSelectionValue(800)
> +        || weight == FontSelectionValue(900))

Maybe have a isWeightABuiltInValue or something.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2594
> +    if (stretch == FontSelectionValue(50))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueUltraCondensed);
> +    else if (stretch == FontSelectionValue(62.5f))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExtraCondensed);
> +    else if (stretch == FontSelectionValue(75))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueCondensed);
> +    else if (stretch == FontSelectionValue(87.5f))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueSemiCondensed);
> +    else if (stretch == normalStretchValue())
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueNormal);
> +    else if (stretch == FontSelectionValue(112.5f))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueSemiExpanded);
> +    else if (stretch == FontSelectionValue(125))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExpanded);
> +    else if (stretch == FontSelectionValue(150))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueExtraExpanded);
> +    else if (stretch == FontSelectionValue(200))
> +        computedFont->stretch = CSSValuePool::singleton().createIdentifierValue(CSSValueUltraExpanded);

It would be much nicer to see the magic numbers in a data table that maps float to identifier.

In fact, maybe you should group all of the font-related magic numbers into one place (its own header or something).

> Source/WebCore/css/CSSFontFace.cpp:152
> +    case CSSValueBold:
> +    case CSSValueBolder:
> +        result = boldWeightValue();

Odd that both map to the same number?

> Source/WebCore/css/CSSFontFace.cpp:155
> +        result = FontSelectionValue(200);

More magic numbers.

> Source/WebCore/css/CSSFontFace.cpp:230
>      case CSSValueUltraCondensed:
> -        return FontSelectionValue(50);
> +        result = FontSelectionValue(50);
> +        break;
>      case CSSValueExtraCondensed:
> -        return FontSelectionValue(62.5f);
> +        result = FontSelectionValue(62.5f);
> +        break;
>      case CSSValueCondensed:
> -        return FontSelectionValue(75);
> +        result = FontSelectionValue(75);
> +        break;
>      case CSSValueSemiCondensed:
> -        return FontSelectionValue(87.5f);
> +        result = FontSelectionValue(87.5f);
> +        break;
>      case CSSValueNormal:
> -        return FontSelectionValue(100);
> +        result = normalStretchValue();
> +        break;
>      case CSSValueSemiExpanded:
> -        return FontSelectionValue(112.5f);
> +        result = FontSelectionValue(112.5f);
> +        break;
>      case CSSValueExpanded:
> -        return FontSelectionValue(125);
> +        result = FontSelectionValue(125);
> +        break;
>      case CSSValueExtraExpanded:
> -        return FontSelectionValue(150);
> +        result = FontSelectionValue(150);
> +        break;
>      case CSSValueUltraExpanded:
> -        return FontSelectionValue(200);
> +        result = FontSelectionValue(200);
> +        break;

More fodder for a data table.

> Source/WebCore/css/CSSFontFace.cpp:258
> +static std::optional<FontSelectionRange> calculateStyleRange(CSSValue& value)

"style" is a bit confusing in this context. Not CSS-type style, or RenderStyle, right? Is this just about sloping?

> Source/WebCore/css/CSSFontFaceSet.cpp:314
> +    case CSSValueBold:
> +    case CSSValueBolder:
> +        return boldWeightValue();
> +    case CSSValueNormal:
> +        return normalWeightValue();
> +    case CSSValueLighter:
> +        return FontSelectionValue(200);

Seen these before.

> Source/WebCore/css/CSSFontFaceSet.cpp:357
> +    case CSSValueUltraCondensed:
> +        return FontSelectionValue(50);
> +    case CSSValueExtraCondensed:
> +        return FontSelectionValue(62.5f);
> +    case CSSValueCondensed:
> +        return FontSelectionValue(75);
> +    case CSSValueSemiCondensed:
> +        return FontSelectionValue(87.5f);
> +    case CSSValueNormal:
> +        return normalStretchValue();
> +    case CSSValueSemiExpanded:
> +        return FontSelectionValue(112.5f);
> +    case CSSValueExpanded:
> +        return FontSelectionValue(125);
> +    case CSSValueExtraExpanded:
> +        return FontSelectionValue(150);
> +    case CSSValueUltraExpanded:
> +        return FontSelectionValue(200);

More data table fodder.

> Source/WebCore/css/CSSFontFaceSet.cpp:386
> +    case CSSValueNormal:
> +        return FontSelectionValue();
> +    case CSSValueItalic:
> +    case CSSValueOblique:
> +        return italicThreshold();

Seen this before.

> Source/WebCore/css/StyleBuilderConverter.h:1179
> +    case CSSValueNormal:
> +        return FontSelectionValue(400);
> +    case CSSValueBold:
> +        return FontSelectionValue(700);
> +    case CSSValueBolder:

More fodder for the data table.

> Source/WebCore/css/StyleBuilderConverter.h:1249
> +    case CSSValueNormal:
> +        return FontSelectionValue();
> +    case CSSValueOblique:
> +    case CSSValueItalic:
> +        return FontSelectionValue(20);

Looks sharable.

> Source/WebCore/platform/graphics/FontDescription.cpp:117
> +    if (weight < FontSelectionValue(350))
> +        return FontSelectionValue(400);
> +    if (weight < FontSelectionValue(550))
> +        return FontSelectionValue(700);
> +    if (weight < FontSelectionValue(900))
> +        return FontSelectionValue(900);
> +    return weight;

Also this.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:201
>  static inline FontSelectionValue boldThreshold()
>  {
>      static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(600);
>      return result.get();
>  }
>  
> +static inline FontSelectionValue boldWeightValue()
> +{
> +    static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(700);
> +    return result.get();
> +}
> +
> +static inline FontSelectionValue normalWeightValue()
> +{
> +    static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(400);
> +    return result.get();
> +}
> +
> +static inline bool isFontWeightBold(FontSelectionValue fontWeight)
> +{
> +    return fontWeight >= boldThreshold();
> +}
> +
>  static inline FontSelectionValue weightSearchThreshold()
>  {
>      static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(500);
>      return result.get();
>  }
>  
> +static inline FontSelectionValue normalStretchValue()
> +{
> +    static NeverDestroyed<FontSelectionValue> result = FontSelectionValue(100);
> +    return result.get();

Could come from a data table.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:712
> +    if (weight < -0.6)
> +        return FontSelectionValue(100);
> +    if (weight < -0.365)
> +        return FontSelectionValue(200);
> +    if (weight < -0.115)
> +        return FontSelectionValue(300);
> +    if (weight <  0.130)
> +        return FontSelectionValue(400);
> +    if (weight <  0.235)
> +        return FontSelectionValue(500);
> +    if (weight <  0.350)
> +        return FontSelectionValue(600);
> +    if (weight <  0.500)
> +        return FontSelectionValue(700);
> +    if (weight <  0.700)
> +        return FontSelectionValue(800);

Also this.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:182
> +    if (weight < FontSelectionValue(150))
>          return FC_WEIGHT_THIN;
> -    case FontWeight200:
> +    if (weight < FontSelectionValue(250))
>          return FC_WEIGHT_ULTRALIGHT;
> -    case FontWeight300:
> +    if (weight < FontSelectionValue(350))
>          return FC_WEIGHT_LIGHT;
> -    case FontWeight400:
> +    if (weight < FontSelectionValue(450))
>          return FC_WEIGHT_REGULAR;
> -    case FontWeight500:
> +    if (weight < FontSelectionValue(550))
>          return FC_WEIGHT_MEDIUM;
> -    case FontWeight600:
> +    if (weight < FontSelectionValue(650))
>          return FC_WEIGHT_SEMIBOLD;
> -    case FontWeight700:
> +    if (weight < FontSelectionValue(750))
>          return FC_WEIGHT_BOLD;
> -    case FontWeight800:
> +    if (weight < FontSelectionValue(850))

Maybe this too.

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:91
> -    if (weight > FontWeight300) {
> +    if (weight >= FontSelectionValue(350)) {
>          if (bold)
>              fontType = kCTFontUIFontEmphasizedSystem;
> -    } else if (weight > FontWeight200)
> +    } else if (weight >= FontSelectionValue(250))
>          fontType = static_cast<CTFontUIFontType>(kCTFontUIFontSystemLight);
> -    else if (weight > FontWeight100)
> +    else if (weight >= FontSelectionValue(150))
>          fontType = static_cast<CTFontUIFontType>(kCTFontUIFontSystemThin);

And this.

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:118
> +    float ctWeight = kCTFontWeightRegular;
> +    if (weight < FontSelectionValue(150))
> +        ctWeight = kCTFontWeightUltraLight;
> +    else if (weight < FontSelectionValue(250))
> +        ctWeight = kCTFontWeightThin;
> +    else if (weight < FontSelectionValue(350))
> +        ctWeight = kCTFontWeightLight;
> +    else if (weight < FontSelectionValue(450))
> +        ctWeight = kCTFontWeightRegular;
> +    else if (weight < FontSelectionValue(550))
> +        ctWeight = kCTFontWeightMedium;
> +    else if (weight < FontSelectionValue(650))
> +        ctWeight = kCTFontWeightSemibold;
> +    else if (weight < FontSelectionValue(750))
> +        ctWeight = kCTFontWeightBold;
> +    else if (weight < FontSelectionValue(850))
> +        ctWeight = kCTFontWeightHeavy;

Def. this.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:73
> +    if (fontWeight < FontSelectionValue(150))
> +        return NSFontWeightUltraLight;
> +    if (fontWeight < FontSelectionValue(250))
> +        return NSFontWeightThin;
> +    if (fontWeight < FontSelectionValue(350))
> +        return NSFontWeightLight;
> +    if (fontWeight < FontSelectionValue(450))
> +        return NSFontWeightRegular;
> +    if (fontWeight < FontSelectionValue(550))
> +        return NSFontWeightMedium;
> +    if (fontWeight < FontSelectionValue(650))
> +        return NSFontWeightSemibold;
> +    if (fontWeight < FontSelectionValue(750))
> +        return NSFontWeightBold;
> +    if (fontWeight < FontSelectionValue(850))
> +        return NSFontWeightHeavy;
> +    return NSFontWeightBlack;

So many magic numbers.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:419
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_THIN;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_EXTRALIGHT;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_LIGHT;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_NORMAL;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_MEDIUM;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_SEMIBOLD;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_BOLD;
> +    if (fontWeight < FontSelectionValue(150))
> +        return FW_EXTRABOLD;
> +    return FW_HEAVY;

Gah.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:581
> +    case FW_THIN:
> +        weight = FontSelectionValue(100);
> +        break;
> +    case FW_EXTRALIGHT:
> +        weight = FontSelectionValue(200);
> +        break;
> +    case FW_LIGHT:
> +        weight = FontSelectionValue(300);
> +        break;
> +    case FW_NORMAL:
> +        weight = FontSelectionValue(400);
> +        break;
> +    case FW_MEDIUM:
> +        weight = FontSelectionValue(500);
> +        break;
> +    case FW_SEMIBOLD:
> +        weight = FontSelectionValue(600);
> +        break;
> +    case FW_BOLD:
> +        weight = FontSelectionValue(700);
> +        break;
> +    case FW_EXTRABOLD:
> +        weight = FontSelectionValue(800);
> +        break;
> +    default:
> +        weight = FontSelectionValue(900);
> +        break;

Oh my

> Source/WebCore/rendering/RenderThemeMac.mm:382
> +    static const FontSelectionValue fontWeights[] = {
> +        FontSelectionValue(100),
> +        FontSelectionValue(100),
> +        FontSelectionValue(200),
> +        FontSelectionValue(300),
> +        FontSelectionValue(400),
> +        FontSelectionValue(500),
> +        FontSelectionValue(600),
> +        FontSelectionValue(600),
> +        FontSelectionValue(700),
> +        FontSelectionValue(800),
> +        FontSelectionValue(800),
> +        FontSelectionValue(900),
> +        FontSelectionValue(900),
> +        FontSelectionValue(900)

Roll this into some shared data table?

[Myles C. Maxfield]

Created attachment 303525 [details]
WIP

[Myles C. Maxfield]

Comment on attachment 303503 [details]
Patch

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

>> Source/WebCore/css/CSSFontFace.cpp:152
>> +        result = boldWeightValue();
> 
> Odd that both map to the same number?

That's how it's supposed to work :/

[Myles C. Maxfield]

Created attachment 303531 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 303531 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:127
> +static std::optional<FontSelectionRange> calculateWeightRange(CSSValue& value)

You never return nullopt so why the option return value?

> Source/WebCore/css/CSSFontFace.cpp:209
> +static std::optional<FontSelectionRange> calculateItalicRange(CSSValue& value)

Ditto.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:879
>      if ((weight % 100) || weight < 100 || weight > 900)

Maybe these numbers should go into the header.

[Myles C. Maxfield]

Created attachment 303534 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 303535 [details]
Patch for committing

[Build Bot]

Comment on attachment 303531 [details]
Patch

Attachment 303531 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3253850

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 303538 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303531 [details]
Patch

Attachment 303531 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3253863

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 303539 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303531 [details]
Patch

Attachment 303531 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3253819

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 303540 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303531 [details]
Patch

Attachment 303531 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3253867

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 303542 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303535 [details]
Patch for committing

Attachment 303535 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3253912

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 303544 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Created attachment 303546 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 303546 [details]
Patch for committing

Clearing flags on attachment: 303546

Committed r213464: <http://trac.webkit.org/changeset/213464>