Bug 169674 – Use CopyFontDescriptorWithStrokeForStyle to get correct stroke width for captions.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

We should use the MediaAccessibility function CopyFontDescriptorWithStrokeForStyle to get correct captions stroke width from font size.

[Per Arne Vollan]

Created attachment 304628 [details]
Patch

[Per Arne Vollan]

rdar://problem/30956385

[Eric Carlson]

Comment on attachment 304628 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:367
> +    CFStringRef trackLanguage = nullptr;

A track may have a language tag (see TrackBase::validBCP47Language), why pass NULL?

[Per Arne Vollan]

Created attachment 304773 [details]
Patch

[Per Arne Vollan]

(In reply to comment #3)
> Comment on attachment 304628 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304628&action=review
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:367
> > +    CFStringRef trackLanguage = nullptr;
> 
> A track may have a language tag (see TrackBase::validBCP47Language), why
> pass NULL?

Thanks for reviewing! Added language parameter in last patch.

[Eric Carlson]

Comment on attachment 304773 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:1243
> +    String language;
> +

Ditto: blank line not necessary.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1251
> +    auto& tracks = mediaElement->textTracks();
> +    for (unsigned i = 0; i < tracks.length(); ++i) {
> +        auto track = tracks.item(i);
> +        if (track && track->mode() == TextTrack::Mode::Showing) {
> +            language = track->validBCP47Language();
> +            break;
> +        }
> +    }

It is possible to have more than one text track enabled so this may not choose the correct font for one or more tracks. 

The default UI only allows a user to enable one track at a time so I think this is OK for now, but please file a bug to consider doing this differently.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1257
> +    if (document().page()->group().captionPreferences().captionStrokeWidth(m_fontSize, language, strokeWidth, important))
> +        setInlineStyleProperty(CSSPropertyWebkitTextStrokeWidth, strokeWidth, CSSPrimitiveValue::CSS_PT, important);

I think it would be better to set this in the stylesheet like the other user style preferences. This is OK for now, but please file a bug and note it here to figure out another way to do this.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:361
> +bool CaptionUserPreferencesMediaAF::captionStrokeWidth(float fontSize, const String& language, float& strokeWidth, bool& important) const

Nit: something like "captionStrokeWidthForFont" would be better.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:379
> +    // Currently, MACaptionAppearanceCopyFontDescriptorWithStrokeForStyle is returning very large stroke widths.
> +    // To avoid stroke widths that are too large, we set a maximum value of 10% of the font size.

Please file a Radar against MediaAccessibility and note the number here.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:380
> +    strokeWidth = std::min(strokeWidth, fontSize / 10.0f);

Nit: the "f" is unnecessary.

[Jon Lee]

Comment on attachment 304773 [details]
Patch

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

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1257
>> +        setInlineStyleProperty(CSSPropertyWebkitTextStrokeWidth, strokeWidth, CSSPrimitiveValue::CSS_PT, important);
> 
> I think it would be better to set this in the stylesheet like the other user style preferences. This is OK for now, but please file a bug and note it here to figure out another way to do this.

We can switch to unprefixed stroke width now, right?

[Per Arne Vollan]

(In reply to comment #6)
> Comment on attachment 304773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304773&action=review
> 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1243
> > +    String language;
> > +
> 
> Ditto: blank line not necessary.
> 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1251
> > +    auto& tracks = mediaElement->textTracks();
> > +    for (unsigned i = 0; i < tracks.length(); ++i) {
> > +        auto track = tracks.item(i);
> > +        if (track && track->mode() == TextTrack::Mode::Showing) {
> > +            language = track->validBCP47Language();
> > +            break;
> > +        }
> > +    }
> 
> It is possible to have more than one text track enabled so this may not
> choose the correct font for one or more tracks. 
> 
> The default UI only allows a user to enable one track at a time so I think
> this is OK for now, but please file a bug to consider doing this differently.
> 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1257
> > +    if (document().page()->group().captionPreferences().captionStrokeWidth(m_fontSize, language, strokeWidth, important))
> > +        setInlineStyleProperty(CSSPropertyWebkitTextStrokeWidth, strokeWidth, CSSPrimitiveValue::CSS_PT, important);
> 
> I think it would be better to set this in the stylesheet like the other user
> style preferences. This is OK for now, but please file a bug and note it
> here to figure out another way to do this.
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:361
> > +bool CaptionUserPreferencesMediaAF::captionStrokeWidth(float fontSize, const String& language, float& strokeWidth, bool& important) const
> 
> Nit: something like "captionStrokeWidthForFont" would be better.
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:379
> > +    // Currently, MACaptionAppearanceCopyFontDescriptorWithStrokeForStyle is returning very large stroke widths.
> > +    // To avoid stroke widths that are too large, we set a maximum value of 10% of the font size.
> 
> Please file a Radar against MediaAccessibility and note the number here.
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:380
> > +    strokeWidth = std::min(strokeWidth, fontSize / 10.0f);
> 
> Nit: the "f" is unnecessary.

Thanks for reviewing! I will update the patch before landing.

[Per Arne Vollan]

(In reply to comment #7)
> Comment on attachment 304773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304773&action=review
> 
> >> Source/WebCore/html/shadow/MediaControlElements.cpp:1257
> >> +        setInlineStyleProperty(CSSPropertyWebkitTextStrokeWidth, strokeWidth, CSSPrimitiveValue::CSS_PT, important);
> > 
> > I think it would be better to set this in the stylesheet like the other user style preferences. This is OK for now, but please file a bug and note it here to figure out another way to do this.
> 
> We can switch to unprefixed stroke width now, right?

Yes, thanks for catching this! I will update the patch.

[Per Arne Vollan]

Committed <https://trac.webkit.org/changeset/214169>.