WebVTT: Remove paddings around ::cue boxes

Before this change, captions with 2 or more lines at the bottom of a
video are displayed out of order. This happens because captions are
assigned initial positions that overlap with other captions or with the
media controls and are then placed out of order.

We can avoid overlap by making the following 2 changes. The first
change removes the excess padding that wasn't accounted for in the
position calculation. We tried to use the bounding box height that
includes the padding in the position calculation but it changes cue
placement depending on the number of lines in the cue which is
not ideal. Safari and Firefox do not use paddings with their cues
either so we have decided to go forward with the padding removal.
Our PM has already reviewed and approved this change. Here is a
screenshot showing the difference before and after the padding removal:
https://screenshot.googleplex.com/3jg2r28G6nBzLd4
Also, the original CL that introduced window padding in the first place
can be found here: crrev.com/c/1658914

The second change accounts for the height of the media controls
(crrev.com/c/3804205)

Bug: 1141592
Change-Id: I26e124fa838c6c066c94cc1ffa9bb50129762f9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3782908
Commit-Queue: Jocelyn Tran <jocelyntran@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Abigail Klein <abigailbklein@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1031268}

chrome/browser/chrome_content_browser_client.cc

@@ -3812,7 +3812,6 @@ void ChromeContentBrowserClient::OverrideWebkitPrefs(
     web_prefs->text_track_font_family = style->font_family;
     web_prefs->text_track_font_variant = style->font_variant;
     web_prefs->text_track_window_color = style->window_color;
-    web_prefs->text_track_window_padding = style->window_padding;
     web_prefs->text_track_window_radius = style->window_radius;
   }
 

third_party/blink/common/web_preferences/web_preferences_mojom_traits.cc

@@ -36,7 +36,6 @@ bool StructTraits<blink::mojom::WebPreferencesDataView,
       !data.ReadTextTrackFontStyle(&out->text_track_font_style) ||
       !data.ReadTextTrackFontVariant(&out->text_track_font_variant) ||
       !data.ReadTextTrackWindowColor(&out->text_track_window_color) ||
-      !data.ReadTextTrackWindowPadding(&out->text_track_window_padding) ||
       !data.ReadTextTrackWindowRadius(&out->text_track_window_radius) ||
       !data.ReadPrimaryPointerType(&out->primary_pointer_type) ||
       !data.ReadPrimaryHoverType(&out->primary_hover_type) ||

third_party/blink/public/common/web_preferences/web_preferences.h

@@ -195,8 +195,6 @@ struct BLINK_COMMON_EXPORT WebPreferences {
   // around WebVTT text tracks.
   // Window color can be any legal CSS color descriptor.
   std::string text_track_window_color;
-  // Window padding is in em.
-  std::string text_track_window_padding;
   // Window radius is in pixels.
   std::string text_track_window_radius;
 

third_party/blink/public/common/web_preferences/web_preferences_mojom_traits.h

@@ -496,11 +496,6 @@ struct BLINK_COMMON_EXPORT StructTraits<blink::mojom::WebPreferencesDataView,
     return r.text_track_window_color;
   }
 
-  static const std::string& text_track_window_padding(
-      const blink::web_pref::WebPreferences& r) {
-    return r.text_track_window_padding;
-  }
-
   static const std::string& text_track_window_radius(
       const blink::web_pref::WebPreferences& r) {
     return r.text_track_window_radius;

third_party/blink/public/mojom/webpreferences/web_preferences.mojom

@@ -246,8 +246,6 @@ struct WebPreferences {
   // around WebVTT text tracks.
   // Window color can be any legal CSS color descriptor.
   string text_track_window_color;
-  // Window padding is in em.
-  string text_track_window_padding;
   // Window radius is in pixels.
   string text_track_window_radius;
 

third_party/blink/public/web/web_settings.h

@@ -233,7 +233,6 @@ class WebSettings {
   virtual void SetTextTrackTextShadow(const WebString&) = 0;
   virtual void SetTextTrackTextSize(const WebString&) = 0;
   virtual void SetTextTrackWindowColor(const WebString&) = 0;
-  virtual void SetTextTrackWindowPadding(const WebString&) = 0;
   virtual void SetTextTrackWindowRadius(const WebString&) = 0;
   virtual void SetThreadedScrollingEnabled(bool) = 0;
   virtual void SetTouchDragDropEnabled(bool) = 0;

third_party/blink/renderer/core/css/css_default_style_sheets.cc

@@ -297,8 +297,6 @@ bool CSSDefaultStyleSheets::EnsureDefaultStyleSheetsForElement(
       builder.Append("video::-webkit-media-text-track-display { ");
       AddTextTrackCSSProperties(&builder, CSSPropertyID::kBackgroundColor,
                                 settings->GetTextTrackWindowColor());
-      AddTextTrackCSSProperties(&builder, CSSPropertyID::kPadding,
-                                settings->GetTextTrackWindowPadding());
       AddTextTrackCSSProperties(&builder, CSSPropertyID::kBorderRadius,
                                 settings->GetTextTrackWindowRadius());
       builder.Append(" } video::cue { ");

third_party/blink/renderer/core/exported/web_settings_impl.cc

@@ -380,10 +380,6 @@ void WebSettingsImpl::SetTextTrackWindowColor(const WebString& color) {
   settings_->SetTextTrackWindowColor(color);
 }
 
-void WebSettingsImpl::SetTextTrackWindowPadding(const WebString& padding) {
-  settings_->SetTextTrackWindowPadding(padding);
-}
-
 void WebSettingsImpl::SetTextTrackWindowRadius(const WebString& radius) {
   settings_->SetTextTrackWindowRadius(radius);
 }

third_party/blink/renderer/core/exported/web_settings_impl.h

@@ -175,7 +175,6 @@ class CORE_EXPORT WebSettingsImpl final : public WebSettings {
   void SetTextTrackTextShadow(const WebString&) override;
   void SetTextTrackTextSize(const WebString&) override;
   void SetTextTrackWindowColor(const WebString&) override;
-  void SetTextTrackWindowPadding(const WebString&) override;
   void SetTextTrackWindowRadius(const WebString&) override;
   void SetThreadedScrollingEnabled(bool) override;
   void SetTouchDragDropEnabled(bool) override;

third_party/blink/renderer/core/exported/web_view_impl.cc

@@ -1616,8 +1616,6 @@ void WebView::ApplyWebPreferences(const web_pref::WebPreferences& prefs,
   settings->SetTextTrackMarginPercentage(prefs.text_track_margin_percentage);
   settings->SetTextTrackWindowColor(
       WebString::FromASCII(prefs.text_track_window_color));
-  settings->SetTextTrackWindowPadding(
-      WebString::FromASCII(prefs.text_track_window_padding));
   settings->SetTextTrackWindowRadius(
       WebString::FromASCII(prefs.text_track_window_radius));
 

third_party/blink/renderer/core/frame/settings.json5

@@ -772,10 +772,6 @@
       name: "textTrackWindowColor",
       type: "String",
     },
-    {
-      name: "textTrackWindowPadding",
-      type: "String",
-    },
     {
       name: "textTrackWindowRadius",
       type: "String",

third_party/blink/web_tests/media/track/track-css-user-settings-override-internal-settings.html

@@ -43,7 +43,6 @@
         internals.settings.setTextTrackTextColor("rgb(0, 255, 255)");
         internals.settings.setTextTrackTextSize("14px");
         internals.settings.setTextTrackWindowColor("rgba(0, 0, 0, 0.8)");
-        internals.settings.setTextTrackWindowPadding("5px");
         var video2 = addVideoTo(container);
         video2.oncanplaythrough = t.step_func_done(function() {
             var displayTree = textTrackCueElementByIndex(video2, 0);

third_party/blink/web_tests/media/track/track-cue-rendering-negative-line-numbers.html

@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="../media-controls.js"></script>
+<style>
+::cue { font-size: 24px; }
+</style>
+<video src="../content/test.ogv" width="640"></video>
+<script>
+
+function animationFrame() {
+  return new Promise(resolve => { requestAnimationFrame(resolve); });
+}
+
+promise_test(async () => {
+  let v = document.querySelector('video');
+  let track = v.addTextTrack('subtitles', 'label', 'en');
+  track.mode = 'showing';
+  let cue0 = new VTTCue(1, 60, 'First subtitle');
+  cue0.line = -3;
+  track.addCue(cue0);
+  let cue1 = new VTTCue(1, 60, 'Second subtitle');
+  cue1.line = -2;
+  track.addCue(cue1);
+  let cue2 = new VTTCue(1, 60, 'Third subtitle');
+  cue2.line = -1;
+  track.addCue(cue2);
+
+  v.currentTime = 3;
+  await new Promise(resolve => { v.addEventListener('seeked', resolve, {once:true}); });
+  v.controls = false;
+  await animationFrame();
+  await animationFrame();
+  // Now cues are shown.
+  let top0 = textTrackCueElementByIndex(v, 0).offsetTop;
+  let top1 = textTrackCueElementByIndex(v, 1).offsetTop;
+  let top2 = textTrackCueElementByIndex(v, 2).offsetTop;
+  assert_true(top0<top1, 'subtitle 1 is above subtitle 2');
+  assert_true(top1<top2, 'subtitle 2 is above subtitle 3');
+}, 'Cues with negative line numbers should be displayed in order');
+</script>

ui/native_theme/caption_style.h

@@ -42,7 +42,6 @@ struct NATIVE_THEME_EXPORT CaptionStyle {
   std::string font_family;
   std::string font_variant;
   std::string window_color;
-  std::string window_padding;
   std::string window_radius;
 };
 

ui/native_theme/caption_style_mac.mm

@@ -159,16 +159,6 @@ void GetMAFontAsCSSFontSpecifiers(std::string* font_family,
                               important);
 }
 
-// If the window is visible (its opacity is greater than 0), give it padding so
-// it surrounds the text track cue. If it is not visible, its padding should be
-// 0. Webkit uses 0.4em padding so we match that here.
-std::string GetMAWindowPaddingAsCSSNumberInEm() {
-  MACaptionAppearanceBehavior behavior;
-  float opacity = MACaptionAppearanceGetWindowOpacity(kUserDomain, &behavior);
-  bool important = behavior == kMACaptionAppearanceBehaviorUseValue;
-  return MaybeAddCSSImportant(opacity > 0 ? "0.4em" : "0em", important);
-}
-
 std::string GetMAWindowRadiusAsCSSNumberInPixels() {
   MACaptionAppearanceBehavior behavior;
   float radius =
@@ -191,7 +181,6 @@ void GetMAFontAsCSSFontSpecifiers(std::string* font_family,
   style.text_size = GetMATextScaleAsCSSPercent();
   style.text_shadow = GetMATextEdgeStyleAsCSSShadow();
   style.window_color = GetMAWindowColorAsCSSColor();
-  style.window_padding = GetMAWindowPaddingAsCSSNumberInEm();
   style.window_radius = GetMAWindowRadiusAsCSSNumberInPixels();
 
   GetMAFontAsCSSFontSpecifiers(&style.font_family, &style.font_variant);