[ReaderMode] Persist font magnification settings on desktop

This CL is a follow up to https://crbug.com/1016615, and stores the
users chosen font magnification size as a preference.

AXRelnotes: N/A

Bug: 1016615
Change-Id: I4a2331edf7d36fa12a5e260fef863cd495d90bdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2212222Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Meredith Lane <meredithl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799381}

chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

@@ -649,9 +649,14 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, UISetsPrefs) {
 
   // Verify that the initial preferences aren't the same as those set below.
   ExpectBodyHasThemeAndFont(contents, "light", "sans-serif");
+  std::string initial_font_size;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(contents, kGetFontSize,
+                                                     &initial_font_size));
+  EXPECT_EQ(initial_font_size, "16px");
   EXPECT_NE(mojom::Theme::kDark, distilled_page_prefs->GetTheme());
   EXPECT_NE(mojom::FontFamily::kMonospace,
             distilled_page_prefs->GetFontFamily());
+  EXPECT_NE(3.0, distilled_page_prefs->GetFontScaling());
 
   // 'Click' the associated UI elements for changing each preference.
   const std::string script = R"(
@@ -674,6 +679,9 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, UISetsPrefs) {
         document.querySelector(
             '#font-family-selection option[value="monospace"]')
           .dispatchEvent(new Event("change", { bubbles: true }));
+        const slider = document.getElementById('font-size-selection');
+        slider.value = 9;
+        slider.dispatchEvent(new Event("input", {bubbles: true}));
       })();)";
   content::DOMMessageQueue queue(contents);
   std::string result;
@@ -685,7 +693,8 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest, UISetsPrefs) {
   PrefChangeObserver observer;
   while (distilled_page_prefs->GetTheme() != mojom::Theme::kDark ||
          mojom::FontFamily::kMonospace !=
-             distilled_page_prefs->GetFontFamily()) {
+             distilled_page_prefs->GetFontFamily() ||
+         3.0f != distilled_page_prefs->GetFontScaling()) {
     observer.WaitForChange(distilled_page_prefs);
   }
 }

components/dom_distiller/content/browser/distiller_javascript_service_impl.cc

@@ -33,6 +33,11 @@ void DistillerJavaScriptServiceImpl::HandleStoreFontFamilyPref(
   distilled_page_prefs_->SetFontFamily(font_family);
 }
 
+void DistillerJavaScriptServiceImpl::HandleStoreFontScalingPref(
+    float font_scale) {
+  distilled_page_prefs_->SetFontScaling(font_scale);
+}
+
 void CreateDistillerJavaScriptService(
     DistillerUIHandle* distiller_ui_handle,
     DistilledPagePrefs* distilled_page_prefs,

components/dom_distiller/content/browser/distiller_javascript_service_impl.h

@@ -30,6 +30,7 @@ class DistillerJavaScriptServiceImpl
 
   void HandleStoreThemePref(mojom::Theme theme) override;
   void HandleStoreFontFamilyPref(mojom::FontFamily font_family) override;
+  void HandleStoreFontScalingPref(float font_scale) override;
 
   DistillerJavaScriptServiceImpl(const DistillerJavaScriptServiceImpl&) =
       delete;

components/dom_distiller/content/browser/test/distilled_page_js_browsertest.cc

@@ -50,7 +50,8 @@ class DistilledPageJsTest : public content::ContentBrowserTest {
   std::unique_ptr<FakeDistilledPage> distilled_page_;
 };
 
-#if defined(OS_WIN)
+// Pincher is only used on Android.
+#if !defined(OS_ANDROID)
 #define MAYBE_Pinch DISABLED_Pinch
 #else
 #define MAYBE_Pinch Pinch
@@ -59,6 +60,16 @@ IN_PROC_BROWSER_TEST_F(DistilledPageJsTest, MAYBE_Pinch) {
   LoadAndExecuteTestScript("pinch_tester.js");
 }
 
+// FontSizeSlider is only used on Desktop.
+#if defined(OS_ANDROID)
+#define MAYBE_FontSizeSlider DISABLED_FontSizeSlider
+#else
+#define MAYBE_FontSizeSlider FontSizeSlider
+#endif
+IN_PROC_BROWSER_TEST_F(DistilledPageJsTest, MAYBE_FontSizeSlider) {
+  LoadAndExecuteTestScript("font_size_slider_tester.js");
+}
+
 IN_PROC_BROWSER_TEST_F(DistilledPageJsTest, SettingsDialogTest) {
   LoadAndExecuteTestScript("settings_dialog_tester.js");
 }

components/dom_distiller/content/common/mojom/distiller_javascript_service.mojom

@@ -17,4 +17,6 @@ interface DistillerJavaScriptService {
   HandleStoreThemePref(Theme theme);
   // Store selected font family as a synced preference.
   HandleStoreFontFamilyPref(FontFamily font_family);
+  // Store selected font size scaling factor as a preference.
+  HandleStoreFontScalingPref(float font_scale);
 };

components/dom_distiller/content/renderer/distiller_native_javascript.cc

@@ -18,6 +18,14 @@
 #include "third_party/blink/public/web/blink.h"
 #include "v8/include/v8.h"
 
+namespace {
+
+// These values should agree with those in distilled_page_prefs.cc.
+const float kMinFontScale = 0.4f;
+const float kMaxFontScale = 3.0f;
+
+}  // namespace
+
 namespace dom_distiller {
 
 DistillerNativeJavaScript::DistillerNativeJavaScript(
@@ -40,6 +48,12 @@ void DistillerNativeJavaScript::StoreIntFontFamily(int int_font_family) {
   distiller_js_service_->HandleStoreFontFamilyPref(font_family);
 }
 
+void DistillerNativeJavaScript::StoreFloatFontScaling(float float_font_scale) {
+  if (float_font_scale < kMinFontScale || float_font_scale > kMaxFontScale)
+    return;
+  distiller_js_service_->HandleStoreFontScalingPref(float_font_scale);
+}
+
 void DistillerNativeJavaScript::AddJavaScriptObjectToFrame(
     v8::Local<v8::Context> context) {
   v8::Isolate* isolate = blink::MainThreadIsolate();
@@ -72,6 +86,11 @@ void DistillerNativeJavaScript::AddJavaScriptObjectToFrame(
       isolate, distiller_obj, "storeFontFamilyPref",
       base::BindRepeating(&DistillerNativeJavaScript::StoreIntFontFamily,
                           base::Unretained(this)));
+
+  BindFunctionToObject(
+      isolate, distiller_obj, "storeFontScalingPref",
+      base::BindRepeating(&DistillerNativeJavaScript::StoreFloatFontScaling,
+                          base::Unretained(this)));
 }
 
 template <typename Sig>

components/dom_distiller/content/renderer/distiller_native_javascript.h

@@ -40,6 +40,7 @@ class DistillerNativeJavaScript {
   // the enum values to the browser process.
   void StoreIntTheme(int theme);
   void StoreIntFontFamily(int font_family);
+  void StoreFloatFontScaling(float font_scale);
 
   content::RenderFrame* render_frame_;
   mojo::Remote<mojom::DistillerJavaScriptService> distiller_js_service_;

components/dom_distiller/core/distilled_page_prefs.cc

@@ -14,6 +14,16 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 
+namespace {
+
+const float kDefaultFontScale = 1.0f;
+
+// These values should agree with those in distiller_native_javascript.cc.
+const float kMinFontScale = 0.4f;
+const float kMaxFontScale = 3.0f;
+
+}  // namespace
+
 namespace dom_distiller {
 
 DistilledPagePrefs::DistilledPagePrefs(PrefService* pref_service)
@@ -30,7 +40,7 @@ void DistilledPagePrefs::RegisterProfilePrefs(
   registry->RegisterIntegerPref(
       prefs::kFont, static_cast<int32_t>(mojom::FontFamily::kSansSerif),
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
-  registry->RegisterDoublePref(prefs::kFontScale, 1.0);
+  registry->RegisterDoublePref(prefs::kFontScale, kDefaultFontScale);
   registry->RegisterBooleanPref(
       prefs::kReaderForAccessibility, false,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
@@ -85,11 +95,11 @@ void DistilledPagePrefs::SetFontScaling(float scaling) {
 
 float DistilledPagePrefs::GetFontScaling() {
   float scaling = pref_service_->GetDouble(prefs::kFontScale);
-  if (scaling < 0.4 || scaling > 2.5) {
+  if (scaling < kMinFontScale || scaling > kMaxFontScale) {
     // Persisted data was incorrect, trying to clean it up by storing the
     // default.
-    SetFontScaling(1.0);
-    return 1.0;
+    SetFontScaling(kDefaultFontScale);
+    return kDefaultFontScale;
   }
   return scaling;
 }

components/dom_distiller/core/distilled_page_prefs_unittests.cc

@@ -16,9 +16,7 @@ namespace {
 class TestingObserver : public DistilledPagePrefs::Observer {
  public:
   TestingObserver()
-      : font_(mojom::FontFamily::kSansSerif),
-        theme_(mojom::Theme::kLight),
-        scaling_(1.0f) {}
+      : font_(mojom::FontFamily::kSansSerif), theme_(mojom::Theme::kLight) {}
 
   void OnChangeFontFamily(mojom::FontFamily new_font) override {
     font_ = new_font;
@@ -39,7 +37,7 @@ class TestingObserver : public DistilledPagePrefs::Observer {
  private:
   mojom::FontFamily font_;
   mojom::Theme theme_;
-  float scaling_;
+  float scaling_{1.0f};
 };
 
 }  // namespace

components/dom_distiller/core/dom_distiller_request_view_base.cc

@@ -54,6 +54,8 @@ void DomDistillerRequestViewBase::OnArticleReady(
     SendJavaScript(viewer::GetSetTitleJs(article_proto->title()));
     SendJavaScript(viewer::GetSetTextDirectionJs(text_direction));
     SendJavaScript(viewer::GetUnsafeArticleContentJs(article_proto));
+    SendJavaScript(viewer::GetDistilledPageFontScalingJs(
+        distilled_page_prefs_->GetFontScaling()));
   } else {
     // It's possible that we didn't get some incremental updates from the
     // distiller. Ensure all remaining pages are flushed to the viewer.
@@ -85,6 +87,8 @@ void DomDistillerRequestViewBase::OnArticleUpdated(
       // client.
       SendJavaScript(viewer::GetSetTitleJs(page.title()));
       SendJavaScript(viewer::GetSetTextDirectionJs(page.text_direction()));
+      SendJavaScript(viewer::GetDistilledPageFontScalingJs(
+          distilled_page_prefs_->GetFontScaling()));
     }
   }
 }

components/dom_distiller/core/javascript/dom_distiller_viewer.js

@@ -99,10 +99,6 @@ function updateToolbarColor(theme) {
   $('theme-color').content = toolbarColor;
 }
 
-function useFontScaling(scaling) {
-  pincher.useFontScaling(scaling);
-}
-
 function maybeSetWebFont() {
   // On iOS, the web fonts block the rendering until the resources are
   // fetched, which can take a long time on slow networks.
@@ -121,36 +117,44 @@ function maybeSetWebFont() {
 
 // TODO(https://crbug.com/1027612): Consider making this a custom HTML element.
 class FontSizeSlider {
-  constructor(element, supportedFontSizes) {
-    this.element = element;
-    this.supportedFontSizes = supportedFontSizes;
+  constructor() {
+    this.element = $('font-size-selection');
+    this.baseSize = 16;
+    // These scales are applied to a base size of 16px.
+    this.fontSizeScale = [0.875, 0.9375, 1, 1.125, 1.25, 1.5, 1.75, 2, 2.5, 3];
 
     this.element.addEventListener('input', (e) => {
-      document.body.style.fontSize =
-          this.supportedFontSizes[e.target.value] + 'px';
-      this.update(e.target.value);
+      const scale = this.fontSizeScale[e.target.value];
+      this.useFontScaling(scale);
+      distiller.storeFontScalingPref(parseFloat(scale));
     });
 
     this.tickmarks = document.createElement('datalist');
     this.tickmarks.setAttribute('class', 'tickmarks');
     this.element.after(this.tickmarks);
 
-    for (let i = 0; i < this.supportedFontSizes.length; i++) {
+    for (let i = 0; i < this.fontSizeScale.length; i++) {
       const option = document.createElement('option');
       option.setAttribute('value', i);
-      option.textContent = supportedFontSizes[i];
+      option.textContent = this.fontSizeScale[i] * this.baseSize;
       this.tickmarks.appendChild(option);
     }
-
+    this.element.value = 2;
+    this.update(this.element.value);
+  }
+  // TODO(meredithl): validate |scale| and snap to nearest supported font size.
+  useFontScaling(scale) {
+    this.element.value = this.fontSizeScale.indexOf(scale);
+    document.documentElement.style.fontSize = scale * this.baseSize + 'px';
     this.update(this.element.value);
   }
 
   update(position) {
     this.element.style.setProperty(
         '--fontSizePercent',
-        (position / (this.supportedFontSizes.length - 1) * 100) + '%');
+        (position / (this.fontSizeScale.length - 1) * 100) + '%');
     this.element.setAttribute(
-        'aria-valuetext', this.supportedFontSizes[position] + 'px');
+        'aria-valuetext', this.fontSizeScale[position] + 'px');
     for (let option = this.tickmarks.firstChild; option != null;
          option = option.nextSibling) {
       const isBeforeThumb = option.value < position;
@@ -160,9 +164,6 @@ class FontSizeSlider {
   }
 }
 
-const fontSizeSlider = new FontSizeSlider(
-    $('font-size-selection'), [14, 15, 16, 18, 20, 24, 28, 32, 40, 48]);
-
 maybeSetWebFont();
 
 // The zooming speed relative to pinching speed.
@@ -170,11 +171,6 @@ const FONT_SCALE_MULTIPLIER = 0.5;
 
 const MIN_SPAN_LENGTH = 20;
 
-// This has to be in sync with 'font-size' in distilledpage.css.
-// This value is hard-coded because JS might be injected before CSS is ready.
-// See crbug.com/1004663.
-const baseSize = 14;
-
 class Pincher {
   // When users pinch in Reader Mode, the page would zoom in or out as if it
   // is a normal web page allowing user-zoom. At the end of pinch gesture, the
@@ -193,6 +189,10 @@ class Pincher {
   // TODO(wychen): Improve scroll position when elementFromPoint is body.
 
   constructor() {
+    // This has to be in sync with 'font-size' in distilledpage.css.
+    // This value is hard-coded because JS might be injected before CSS is
+    // ready. See crbug.com/1004663.
+    this.baseSize = 14;
     this.pinching = false;
     this.fontSizeAnchor = 1.0;
 
@@ -266,7 +266,7 @@ class Pincher {
     document.body.style.transformOrigin = '';
     document.body.style.transform = '';
     document.documentElement.style.fontSize =
-        this.clampedScale * baseSize + 'px';
+        this.clampedScale * this.baseSize + 'px';
 
     this.restoreCenter_();
 
@@ -338,7 +338,7 @@ class Pincher {
     this.pinching = true;
     this.fontSizeAnchor =
         parseFloat(getComputedStyle(document.documentElement).fontSize) /
-        baseSize;
+        this.baseSize;
 
     const pinchOrigin = this.touchPageMid_(e);
     document.body.style.transformOrigin =
@@ -406,7 +406,7 @@ class Pincher {
     this.shiftY = 0;
     this.clampedScale = 1;
     document.documentElement.style.fontSize =
-        this.clampedScale * baseSize + 'px';
+        this.clampedScale * this.baseSize + 'px';
   }
 
   status() {
@@ -422,13 +422,29 @@ class Pincher {
     this.saveCenter_({x: window.innerWidth / 2, y: window.innerHeight / 2});
     this.shiftX = 0;
     this.shiftY = 0;
-    document.documentElement.style.fontSize = scaling * baseSize + 'px';
+    document.documentElement.style.fontSize = scaling * this.baseSize + 'px';
     this.clampedScale = scaling;
     this.restoreCenter_();
   }
 }
 
-const pincher = new Pincher;
+// The pincher is only defined on Android, and the font size slider only on
+// desktop.
+// eslint-disable-next-line no-var
+var pincher, fontSizeSlider;
+if (navigator.userAgent.toLowerCase().indexOf('android') > -1) {
+  pincher = new Pincher;
+} else {
+  fontSizeSlider = new FontSizeSlider;
+}
+
+function useFontScaling(scale) {
+  if (navigator.userAgent.toLowerCase().indexOf('android') > -1) {
+    pincher.useFontScaling(scale);
+  } else {
+    fontSizeSlider.useFontScaling(scale);
+  }
+}
 
 class SettingsDialog {
   constructor(

components/dom_distiller/core/viewer_unittest.cc

@@ -149,4 +149,10 @@ TEST_F(DomDistillerViewerTest, TestGetDistilledPageFontFamilyJsOutput) {
             0);
 }
 
+TEST_F(DomDistillerViewerTest, TestGetDistilledPageFontScalingJsOutput) {
+  std::string kJsFontScaling = "useFontScaling(5);";
+  EXPECT_EQ(kJsFontScaling.compare(viewer::GetDistilledPageFontScalingJs(5)),
+            0);
+}
+
 }  // namespace dom_distiller

components/test/data/dom_distiller/font_size_slider_tester.js

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+suite('FontSizeSlider', function() {
+  test('Font Scale Desktop Selection', function() {
+    chai.assert.strictEqual(pincher, undefined);
+    const documentElement = document.documentElement;
+    const fontSizeSelector = document.getElementById('font-size-selection');
+
+    useFontScaling(1);
+    chai.assert.equal(documentElement.style.fontSize, '16px');
+
+    useFontScaling(3);
+    chai.assert.equal(documentElement.style.fontSize, '48px');
+
+    useFontScaling(0.875);
+    chai.assert.equal(documentElement.style.fontSize, '14px');
+  });
+});