Skip to content

Commit

Permalink
Introduce a --viewer-container-height CSS variable to simplify the …
Browse files Browse the repository at this point in the history
…code

This new CSS variable will allow us to simplify a couple of different viewer components, since we no longer need to use JavaScript-based hacks and can directly set the CSS rules instead. In particular:

 - The `BaseViewer.#previousContainerHeight`-parameter, used as part of the code that will center pages *vertically* in PresentationMode, can be removed.
   By using CSS to control the height of the "dummy"-page we avoid unnecessarily invalidating the scale-value, which can reduce *some* unneeded re-rendering while PresentationMode is active.

 - The `SecondaryToolbar.#setMaxHeight`-method, and its associated parameters, are no longer necessary and can be completely removed.

Note that the code to update the new `--viewer-container-height` CSS variable was purposely placed in the `web/app.js` file, since it's only used in the default viewer. In particular, neither PresentationMode nor Toolbar-functionality is included in the "viewer components".
  • Loading branch information
Snuffleupagus committed May 6, 2022
1 parent cfac6fa commit 4d7d43a
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 55 deletions.
25 changes: 22 additions & 3 deletions web/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ const PDFViewerApplication = {
_docStats: null,
_wheelUnusedTicks: 0,
_idleCallbacks: new Set(),
_doc: document.documentElement,
_PDFBug: null,

// Called once when the document is loaded.
Expand All @@ -275,14 +276,14 @@ const PDFViewerApplication = {
}
await this._initializeViewerComponents();

this._updateViewerContainerHeightCSS();
// Bind the various event handlers *after* the viewer has been
// initialized, to prevent errors if an event arrives too soon.
this.bindEvents();
this.bindWindowEvents();

// We can start UI localization now.
const appContainer = appConfig.appContainer || document.documentElement;
this.l10n.translate(appContainer).then(() => {
this.l10n.translate(appConfig.appContainer || this._doc).then(() => {
// Dispatch the 'localized' event on the `eventBus` once the viewer
// has been fully initialized and translated.
this.eventBus.dispatch("localized", { source: this });
Expand Down Expand Up @@ -574,7 +575,6 @@ const PDFViewerApplication = {

this.secondaryToolbar = new SecondaryToolbar(
appConfig.secondaryToolbar,
container,
eventBus
);

Expand Down Expand Up @@ -2077,6 +2077,23 @@ const PDFViewerApplication = {
}
},

/**
* @ignore
*/
_updateViewerContainerHeightCSS() {
const containerHeight = this.appConfig.mainContainer.clientHeight;

if (containerHeight === this._previousContainerHeight) {
return;
}
this._previousContainerHeight = containerHeight;

this._doc.style.setProperty(
"--viewer-container-height",
`${containerHeight}px`
);
},

/**
* Used together with the integration-tests, to enable awaiting full
* initialization of the scripting/sandbox.
Expand Down Expand Up @@ -2400,6 +2417,8 @@ function webViewerSpreadModeChanged(evt) {
}

function webViewerResize() {
PDFViewerApplication._updateViewerContainerHeightCSS();

const { pdfDocument, pdfViewer } = PDFViewerApplication;
if (!pdfDocument) {
return;
Expand Down
16 changes: 1 addition & 15 deletions web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,6 @@ class BaseViewer {

#enablePermissions = false;

#previousContainerHeight = 0;

#scrollModePageState = null;

#onVisibilityChange = null;
Expand Down Expand Up @@ -936,7 +934,6 @@ class BaseViewer {
if (this.isInPresentationMode) {
const dummyPage = document.createElement("div");
dummyPage.className = "dummyPage";
dummyPage.style.height = `${this.container.clientHeight}px`;
spread.appendChild(dummyPage);
}

Expand Down Expand Up @@ -994,14 +991,6 @@ class BaseViewer {
* only because of limited numerical precision.
*/
#isSameScale(newScale) {
if (
this.isInPresentationMode &&
this.container.clientHeight !== this.#previousContainerHeight
) {
// Ensure that the current page remains centered vertically if/when
// the window is resized while PresentationMode is active.
return false;
}
return (
newScale === this._currentScale ||
Math.abs(newScale - this._currentScale) < 1e-15
Expand Down Expand Up @@ -1062,8 +1051,6 @@ class BaseViewer {
if (this.defaultRenderingQueue) {
this.update();
}

this.#previousContainerHeight = this.container.clientHeight;
}

/**
Expand Down Expand Up @@ -1096,8 +1083,7 @@ class BaseViewer {
hPadding = vPadding = 4;
} else if (this.removePageBorders) {
hPadding = vPadding = 0;
}
if (this._scrollMode === ScrollMode.HORIZONTAL) {
} else if (this._scrollMode === ScrollMode.HORIZONTAL) {
[hPadding, vPadding] = [vPadding, hPadding]; // Swap the padding values.
}
const pageWidthScale =
Expand Down
2 changes: 1 addition & 1 deletion web/pdf_viewer.css
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
.pdfViewer .dummyPage {
position: relative;
width: 0;
/* The height is set via JS, see `BaseViewer.#ensurePageViewVisible`. */
height: var(--viewer-container-height);
}

.pdfViewer.removePageBorders .page {
Expand Down
34 changes: 2 additions & 32 deletions web/secondary_toolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* limitations under the License.
*/

import { SCROLLBAR_PADDING, ScrollMode, SpreadMode } from "./ui_utils.js";
import { ScrollMode, SpreadMode } from "./ui_utils.js";
import { CursorTool } from "./pdf_cursor_tools.js";
import { PagesCountLimit } from "./base_viewer.js";

Expand All @@ -22,9 +22,6 @@ import { PagesCountLimit } from "./base_viewer.js";
* @property {HTMLDivElement} toolbar - Container for the secondary toolbar.
* @property {HTMLButtonElement} toggleButton - Button to toggle the visibility
* of the secondary toolbar.
* @property {HTMLDivElement} toolbarButtonContainer - Container where all the
* toolbar buttons are placed. The maximum height of the toolbar is controlled
* dynamically by adjusting the 'max-height' CSS property of this DOM element.
* @property {HTMLButtonElement} presentationModeButton - Button for entering
* presentation mode.
* @property {HTMLButtonElement} openFileButton - Button to open a file.
Expand Down Expand Up @@ -52,13 +49,11 @@ import { PagesCountLimit } from "./base_viewer.js";
class SecondaryToolbar {
/**
* @param {SecondaryToolbarOptions} options
* @param {HTMLDivElement} mainContainer
* @param {EventBus} eventBus
*/
constructor(options, mainContainer, eventBus) {
constructor(options, eventBus) {
this.toolbar = options.toolbar;
this.toggleButton = options.toggleButton;
this.toolbarButtonContainer = options.toolbarButtonContainer;
this.buttons = [
{
element: options.presentationModeButton,
Expand Down Expand Up @@ -154,12 +149,8 @@ class SecondaryToolbar {
pageRotateCcw: options.pageRotateCcwButton,
};

this.mainContainer = mainContainer;
this.eventBus = eventBus;

this.opened = false;
this.containerHeight = null;
this.previousContainerHeight = null;

this.reset();

Expand All @@ -169,9 +160,6 @@ class SecondaryToolbar {
this.#bindCursorToolsListener(options);
this.#bindScrollModeListener(options);
this.#bindSpreadModeListener(options);

// Bind the event listener for adjusting the 'max-height' of the toolbar.
this.eventBus._on("resize", this.#setMaxHeight.bind(this));
}

/**
Expand Down Expand Up @@ -322,8 +310,6 @@ class SecondaryToolbar {
return;
}
this.opened = true;
this.#setMaxHeight();

this.toggleButton.classList.add("toggled");
this.toggleButton.setAttribute("aria-expanded", "true");
this.toolbar.classList.remove("hidden");
Expand All @@ -346,22 +332,6 @@ class SecondaryToolbar {
this.open();
}
}

#setMaxHeight() {
if (!this.opened) {
return; // Only adjust the 'max-height' if the toolbar is visible.
}
this.containerHeight = this.mainContainer.clientHeight;

if (this.containerHeight === this.previousContainerHeight) {
return;
}
this.toolbarButtonContainer.style.maxHeight = `${
this.containerHeight - SCROLLBAR_PADDING
}px`;

this.previousContainerHeight = this.containerHeight;
}
}

export { SecondaryToolbar };
3 changes: 2 additions & 1 deletion web/viewer.css
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

:root {
--dir-factor: 1;
--viewer-container-height: 440px;
--sidebar-width: 200px;
--sidebar-transition-duration: 200ms;
--sidebar-transition-timing-function: ease;
Expand Down Expand Up @@ -482,7 +483,7 @@ select {

#secondaryToolbarButtonContainer {
max-width: 220px;
max-height: 400px;
max-height: calc(var(--viewer-container-height) - 40px);
overflow-y: auto;
margin-bottom: -4px;
}
Expand Down
3 changes: 0 additions & 3 deletions web/viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,6 @@ function getViewerConfiguration() {
secondaryToolbar: {
toolbar: document.getElementById("secondaryToolbar"),
toggleButton: document.getElementById("secondaryToolbarToggle"),
toolbarButtonContainer: document.getElementById(
"secondaryToolbarButtonContainer"
),
presentationModeButton: document.getElementById(
"secondaryPresentationMode"
),
Expand Down

0 comments on commit 4d7d43a

Please sign in to comment.