Bug 169674

Summary: Use CopyFontDescriptorWithStrokeForStyle to get correct stroke width for captions.
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebCore Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, eric.carlson, jonlee, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch eric.carlson: review+

Description Per Arne Vollan 2017-03-15 08:02:41 PDT
We should use the MediaAccessibility function CopyFontDescriptorWithStrokeForStyle to get correct captions stroke width from font size.
Comment 1 Per Arne Vollan 2017-03-16 03:24:08 PDT
Created attachment 304628 [details]
Patch
Comment 2 Per Arne Vollan 2017-03-16 04:30:16 PDT
rdar://problem/30956385
Comment 3 Eric Carlson 2017-03-16 06:46:26 PDT
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?
Comment 4 Per Arne Vollan 2017-03-17 06:34:39 PDT
Created attachment 304773 [details]
Patch
Comment 5 Per Arne Vollan 2017-03-17 06:37:06 PDT
(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.
Comment 6 Eric Carlson 2017-03-17 08:31:27 PDT
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.
Comment 7 Jon Lee 2017-03-17 09:38:07 PDT
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?
Comment 8 Per Arne Vollan 2017-03-17 10:09:02 PDT
(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.
Comment 9 Per Arne Vollan 2017-03-17 10:10:24 PDT
(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.
Comment 10 Per Arne Vollan 2017-03-20 05:04:34 PDT
Committed <https://trac.webkit.org/changeset/214169>.