We should use the MediaAccessibility function CopyFontDescriptorWithStrokeForStyle to get correct captions stroke width from font size.
Created attachment 304628 [details] Patch
rdar://problem/30956385
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?
Created attachment 304773 [details] Patch
(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 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 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?
(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.
(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.
Committed <https://trac.webkit.org/changeset/214169>.