From 2594f0c7380332a4e12ceee6a4c32b86c838904f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 9 Sep 2020 13:26:38 +0200 Subject: [PATCH 1/2] Revert "[PDFSidebarResizer] Refactor the clamping in `_updateWidth`" This reverts commit 9e4552d7920fcba13b3d74e1e180fc49180f4436 for causing the sidebar to become too narrow when the entire viewer is resized. **Steps to reproduce:** 1. Load the viewer. 2. Open the sidebar. 3. Resize the sidebar, making it wider. 4. Resize the entire viewer, i.e. the browser window, making it *narrower* than 400 pixels. **Expected result:** The sidebar width is clamped at 200 pixels. **Actual result:** The sidebar becomes too narrow. The cause of this bug is, in hindsight, quite obvious since the `clamp` helper function implicitly assumes that the `min`/`max` arguments are correctly sorted. At viewer widths *below* 400 pixels, that assumption is broken which explains the bug. --- web/pdf_sidebar_resizer.js | 20 +++++++++++--------- web/ui_utils.js | 1 - 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/web/pdf_sidebar_resizer.js b/web/pdf_sidebar_resizer.js index 668c59dcb..b3f5b178c 100644 --- a/web/pdf_sidebar_resizer.js +++ b/web/pdf_sidebar_resizer.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { clamp, NullL10n } from "./ui_utils.js"; +import { NullL10n } from "./ui_utils.js"; const SIDEBAR_WIDTH_VAR = "--sidebar-width"; const SIDEBAR_MIN_WIDTH = 200; // pixels @@ -88,17 +88,19 @@ class PDFSidebarResizer { } // Prevent the sidebar from becoming too narrow, or from occupying more // than half of the available viewer width. - const newWidth = clamp( - width, - SIDEBAR_MIN_WIDTH, - Math.floor(this.outerContainerWidth / 2) - ); + const maxWidth = Math.floor(this.outerContainerWidth / 2); + if (width > maxWidth) { + width = maxWidth; + } + if (width < SIDEBAR_MIN_WIDTH) { + width = SIDEBAR_MIN_WIDTH; + } // Only update the UI when the sidebar width did in fact change. - if (newWidth === this._width) { + if (width === this._width) { return false; } - this._width = newWidth; - this.doc.style.setProperty(SIDEBAR_WIDTH_VAR, `${newWidth}px`); + this._width = width; + this.doc.style.setProperty(SIDEBAR_WIDTH_VAR, `${width}px`); return true; } diff --git a/web/ui_utils.js b/web/ui_utils.js index d08497165..4d007703d 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -1006,7 +1006,6 @@ export { SpreadMode, NullL10n, EventBus, - clamp, ProgressBar, getPDFFileNameFromURL, noContextMenuHandler, From 01c1d87171cd7d5a72edaedeb86360242d211ddb Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 9 Sep 2020 13:29:44 +0200 Subject: [PATCH 2/2] Remove CSS variables feature-testing from `PDFSidebarResizer` CSS variables are now supported in all reasonably modern browsers, according to: - https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties#Browser_compatibility - https://caniuse.com/css-variables --- web/pdf_sidebar_resizer.js | 25 +------------------------ web/viewer.html | 2 +- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/web/pdf_sidebar_resizer.js b/web/pdf_sidebar_resizer.js index b3f5b178c..e69c55bf9 100644 --- a/web/pdf_sidebar_resizer.js +++ b/web/pdf_sidebar_resizer.js @@ -34,7 +34,6 @@ class PDFSidebarResizer { * @param {IL10n} l10n - Localization service. */ constructor(options, eventBus, l10n = NullL10n) { - this.enabled = false; this.isRTL = false; this.sidebarOpen = false; this.doc = document.documentElement; @@ -45,24 +44,8 @@ class PDFSidebarResizer { this.outerContainer = options.outerContainer; this.resizer = options.resizer; this.eventBus = eventBus; - this.l10n = l10n; - if ( - (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) && - (typeof CSS === "undefined" || - typeof CSS.supports !== "function" || - !CSS.supports(SIDEBAR_WIDTH_VAR, `calc(-1 * ${SIDEBAR_MIN_WIDTH}px)`)) - ) { - console.warn( - "PDFSidebarResizer: " + - "The browser does not support resizing of the sidebar." - ); - return; - } - this.enabled = true; - this.resizer.classList.remove("hidden"); // Show the resizer DOM element. - - this.l10n.getDirection().then(dir => { + l10n.getDirection().then(dir => { this.isRTL = dir === "rtl"; }); this._addEventListeners(); @@ -83,9 +66,6 @@ class PDFSidebarResizer { * returns {boolean} Indicating if the sidebar width was updated. */ _updateWidth(width = 0) { - if (!this.enabled) { - return false; - } // Prevent the sidebar from becoming too narrow, or from occupying more // than half of the available viewer width. const maxWidth = Math.floor(this.outerContainerWidth / 2); @@ -134,9 +114,6 @@ class PDFSidebarResizer { * @private */ _addEventListeners() { - if (!this.enabled) { - return; - } const _boundEvents = this._boundEvents; _boundEvents.mouseMove = this._mouseMove.bind(this); _boundEvents.mouseUp = this._mouseUp.bind(this); diff --git a/web/viewer.html b/web/viewer.html index 1ae684876..0bef5178b 100644 --- a/web/viewer.html +++ b/web/viewer.html @@ -101,7 +101,7 @@ See https://github.com/adobe-type-tools/cmap-resources - +