From 16a94412e4d093b8880fc5ffbe7f486e9352eef3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 11 Jan 2020 13:55:13 +0100 Subject: [PATCH 1/2] Remove the unused `id` properties from page and thumbnail canvas/image DOM elements (issue 11499) As described in the issue, having a DOM element with `id=page2` (or any other number) will automatically cause that element to become linkable through the URL hash. That's currently leading to some confusing and outright wrong behaviour, since it obviously only works for pages that have been loaded and rendered. For PDF documents the only officially supported way to reference a particular page through the URL hash is using the `#page=2` format, which also works for all pages regardless if they're loaded or not. As far as I can tell there's nothing in the PDF.js default viewer that actually depends on the page/thumbnail `id` at this point in time, hence why I believe that this removal ought to be safe. Just as a pre-caution this patch adds an `aria-label` to the page canvas, similar to the thumbnail canvas/image, to at least keep this information in the DOM. --- l10n/en-US/viewer.properties | 2 ++ l10n/sv-SE/viewer.properties | 2 ++ web/pdf_page_view.js | 6 +++++- web/pdf_thumbnail_view.js | 3 --- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/l10n/en-US/viewer.properties b/l10n/en-US/viewer.properties index 2dd7751aa..93dc4e313 100644 --- a/l10n/en-US/viewer.properties +++ b/l10n/en-US/viewer.properties @@ -148,6 +148,8 @@ thumbs_label=Thumbnails findbar.title=Find in Document findbar_label=Find +# LOCALIZATION NOTE (page_canvas): "{{page}}" will be replaced by the page number. +page_canvas=Page {{page}} # Thumbnails panel item (tooltip and alt text for images) # LOCALIZATION NOTE (thumb_page_title): "{{page}}" will be replaced by the page # number. diff --git a/l10n/sv-SE/viewer.properties b/l10n/sv-SE/viewer.properties index b1b60bd40..3d2f0cf43 100644 --- a/l10n/sv-SE/viewer.properties +++ b/l10n/sv-SE/viewer.properties @@ -148,6 +148,8 @@ thumbs_label=Miniatyrer findbar.title=Sök i dokument findbar_label=Sök +# LOCALIZATION NOTE (page_canvas): "{{page}}" will be replaced by the page number. +page_canvas=Sida {{page}} # Thumbnails panel item (tooltip and alt text for images) # LOCALIZATION NOTE (thumb_page_title): "{{page}}" will be replaced by the page # number. diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 319d59977..856f5bb4c 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -559,7 +559,11 @@ class PDFPageView { const viewport = this.viewport; const canvas = document.createElement("canvas"); - canvas.id = this.renderingId; + this.l10n + .get("page_canvas", { page: this.id }, "Page {{page}}") + .then(msg => { + canvas.setAttribute("aria-label", msg); + }); // Keep the canvas hidden until the first draw callback, or until drawing // is complete when `!this.renderingQueue`, to prevent black flickering. diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index 5d8c9ae1f..3defbafb3 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -258,11 +258,9 @@ class PDFThumbnailView { if (this.renderingState !== RenderingStates.FINISHED) { return; } - const id = this.renderingId; const className = "thumbnailImage"; if (this.disableCanvasToImageConversion) { - this.canvas.id = id; this.canvas.className = className; this.l10n .get( @@ -279,7 +277,6 @@ class PDFThumbnailView { return; } const image = document.createElement("img"); - image.id = id; image.className = className; this.l10n .get( From 34e7d42ce6ae7ab368bcd75ac57f60f99bc8a7ac Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 11 Jan 2020 14:28:29 +0100 Subject: [PATCH 2/2] Add helper functions to reduce unnecessary duplication when fetching l10n messages in `PDFThumbnailView` --- web/pdf_thumbnail_view.js | 76 +++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 43 deletions(-) diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index 3defbafb3..6ab9be758 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -125,11 +125,9 @@ class PDFThumbnailView { const anchor = document.createElement("a"); anchor.href = linkService.getAnchorUrl("#page=" + id); - this.l10n - .get("thumb_page_title", { page: id }, "Page {{page}}") - .then(msg => { - anchor.title = msg; - }); + this._thumbPageTitle.then(msg => { + anchor.title = msg; + }); anchor.onclick = function() { linkService.page = id; return false; @@ -262,15 +260,9 @@ class PDFThumbnailView { if (this.disableCanvasToImageConversion) { this.canvas.className = className; - this.l10n - .get( - "thumb_page_canvas", - { page: this.pageId }, - "Thumbnail of Page {{page}}" - ) - .then(msg => { - this.canvas.setAttribute("aria-label", msg); - }); + this._thumbPageCanvas.then(msg => { + this.canvas.setAttribute("aria-label", msg); + }); this.div.setAttribute("data-loaded", true); this.ring.appendChild(this.canvas); @@ -278,15 +270,9 @@ class PDFThumbnailView { } const image = document.createElement("img"); image.className = className; - this.l10n - .get( - "thumb_page_canvas", - { page: this.pageId }, - "Thumbnail of Page {{page}}" - ) - .then(msg => { - image.setAttribute("aria-label", msg); - }); + this._thumbPageCanvas.then(msg => { + image.setAttribute("aria-label", msg); + }); image.style.width = this.canvasWidth + "px"; image.style.height = this.canvasHeight + "px"; @@ -452,8 +438,20 @@ class PDFThumbnailView { this._convertCanvasToImage(); } - get pageId() { - return this.pageLabel !== null ? this.pageLabel : this.id; + get _thumbPageTitle() { + return this.l10n.get( + "thumb_page_title", + { page: this.pageLabel !== null ? this.pageLabel : this.id }, + "Page {{page}}" + ); + } + + get _thumbPageCanvas() { + return this.l10n.get( + "thumb_page_canvas", + { page: this.pageLabel !== null ? this.pageLabel : this.id }, + "Thumbnail of Page {{page}}" + ); } /** @@ -462,29 +460,21 @@ class PDFThumbnailView { setPageLabel(label) { this.pageLabel = typeof label === "string" ? label : null; - this.l10n - .get("thumb_page_title", { page: this.pageId }, "Page {{page}}") - .then(msg => { - this.anchor.title = msg; - }); + this._thumbPageTitle.then(msg => { + this.anchor.title = msg; + }); if (this.renderingState !== RenderingStates.FINISHED) { return; } - this.l10n - .get( - "thumb_page_canvas", - { page: this.pageId }, - "Thumbnail of Page {{page}}" - ) - .then(ariaLabel => { - if (this.image) { - this.image.setAttribute("aria-label", ariaLabel); - } else if (this.disableCanvasToImageConversion && this.canvas) { - this.canvas.setAttribute("aria-label", ariaLabel); - } - }); + this._thumbPageCanvas.then(msg => { + if (this.image) { + this.image.setAttribute("aria-label", msg); + } else if (this.disableCanvasToImageConversion && this.canvas) { + this.canvas.setAttribute("aria-label", msg); + } + }); } static cleanup() {