From bb9c905c5d6d11883888f328d288384e764f19fa Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 30 Sep 2021 10:40:21 +0200 Subject: [PATCH 1/2] Ensure that various URL-related options are applied in the `xfaLayer` too Note how both the annotationLayer and the document outline will apply various URL-related options when creating the link-elements. For consistency the `xfaLayer`-rendering should obviously use the same options, to ensure that the existing options are indeed applied to all URLs regardless of where they originate. --- src/core/xfa/template.js | 3 +-- src/display/xfa_layer.js | 35 ++++++++++++++++++++++++++++++++--- test/driver.js | 1 + test/unit/xfa_tohtml_spec.js | 4 ++-- web/base_viewer.js | 2 ++ web/interfaces.js | 9 ++++++++- web/xfa_layer_builder.js | 8 +++++++- 7 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/core/xfa/template.js b/src/core/xfa/template.js index 5619a964dd00f..a778eccc858a8 100644 --- a/src/core/xfa/template.js +++ b/src/core/xfa/template.js @@ -1094,7 +1094,6 @@ class Button extends XFAObject { if (!href) { continue; } - const target = jsURL.newWindow ? "_blank" : undefined; // we've an url so generate a htmlButton.children.push({ @@ -1102,7 +1101,7 @@ class Button extends XFAObject { attributes: { id: "link" + this[$uid], href, - target, + newWindow: jsURL.newWindow, class: ["xfaLink"], style: {}, }, diff --git a/src/display/xfa_layer.js b/src/display/xfa_layer.js index 88c2bada3d558..346a5b0e96e15 100644 --- a/src/display/xfa_layer.js +++ b/src/display/xfa_layer.js @@ -13,6 +13,7 @@ * limitations under the License. */ +import { addLinkAttributes, LinkTarget } from "./display_utils.js"; import { XfaText } from "./xfa_text.js"; class XfaLayer { @@ -84,8 +85,10 @@ class XfaLayer { } } - static setAttributes(html, element, storage, intent) { + static setAttributes({ html, element, storage = null, intent, linkService }) { const { attributes } = element; + const isHTMLAnchorElement = html instanceof HTMLAnchorElement; + if (attributes.type === "radio") { // Avoid to have a radio group when printing with the same as one // already displayed. @@ -105,6 +108,9 @@ class XfaLayer { } else if (key === "class") { html.setAttribute(key, value.join(" ")); } else { + if (isHTMLAnchorElement && (key === "href" || key === "newWindow")) { + continue; // Handled below. + } html.setAttribute(key, value); } } else { @@ -112,6 +118,17 @@ class XfaLayer { } } + if (isHTMLAnchorElement) { + addLinkAttributes(html, { + url: attributes.href, + target: attributes.newWindow + ? LinkTarget.BLANK + : linkService.externalLinkTarget, + rel: linkService.externalLinkRel, + enabled: linkService.externalLinkEnabled, + }); + } + // Set the value after the others to be sure overwrite // any other values. if (storage && attributes.dataId) { @@ -121,11 +138,17 @@ class XfaLayer { static render(parameters) { const storage = parameters.annotationStorage; + const linkService = parameters.linkService; const root = parameters.xfa; const intent = parameters.intent || "display"; const rootHtml = document.createElement(root.name); if (root.attributes) { - this.setAttributes(rootHtml, root); + this.setAttributes({ + html: rootHtml, + element: root, + intent, + linkService, + }); } const stack = [[root, -1, rootHtml]]; @@ -169,7 +192,13 @@ class XfaLayer { html.appendChild(childHtml); if (child.attributes) { - this.setAttributes(childHtml, child, storage, intent); + this.setAttributes({ + html: childHtml, + element: child, + storage, + intent, + linkService, + }); } if (child.children && child.children.length > 0) { diff --git a/test/driver.js b/test/driver.js index e89cb8da307ed..13d4882eb369a 100644 --- a/test/driver.js +++ b/test/driver.js @@ -334,6 +334,7 @@ var rasterizeXfaLayer = (function rasterizeXfaLayerClosure() { div, viewport: viewport.clone({ dontFlip: true }), annotationStorage, + linkService: new SimpleLinkService(), intent: isPrint ? "print" : "display", }); diff --git a/test/unit/xfa_tohtml_spec.js b/test/unit/xfa_tohtml_spec.js index eccf75a1da5a2..e9ba898bbd2f2 100644 --- a/test/unit/xfa_tohtml_spec.js +++ b/test/unit/xfa_tohtml_spec.js @@ -640,10 +640,10 @@ describe("XFAFactory", function () { const pages = factory.getPages(); let a = searchHtmlNode(pages, "name", "a"); expect(a.attributes.href).toEqual("https://github.com/mozilla/pdf.js"); - expect(a.attributes.target).toEqual("_blank"); + expect(a.attributes.newWindow).toEqual(true); a = searchHtmlNode(pages, "name", "a", false, [1]); expect(a.attributes.href).toEqual("https://github.com/allizom/pdf.js"); - expect(a.attributes.target).toBe(undefined); + expect(a.attributes.newWindow).toEqual(false); }); }); diff --git a/web/base_viewer.js b/web/base_viewer.js index b1b1086da7ce4..3ce5d91977a2e 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1358,6 +1358,7 @@ class BaseViewer { * @param {PDFPage} pdfPage * @param {AnnotationStorage} [annotationStorage] - Storage for annotation * data in forms. + * @property {IPDFLinkService} linkService * @returns {XfaLayerBuilder} */ createXfaLayerBuilder(pageDiv, pdfPage, annotationStorage = null) { @@ -1366,6 +1367,7 @@ class BaseViewer { pdfPage, annotationStorage: annotationStorage || this.pdfDocument?.annotationStorage, + linkService: this.linkService, }); } diff --git a/web/interfaces.js b/web/interfaces.js index efc66145d45b8..ebe1ad5183200 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -191,9 +191,16 @@ class IPDFXfaLayerFactory { /** * @param {HTMLDivElement} pageDiv * @param {PDFPage} pdfPage + * @param {AnnotationStorage} [annotationStorage] + * @param {Object} [xfaHtml] * @returns {XfaLayerBuilder} */ - createXfaLayerBuilder(pageDiv, pdfPage) {} + createXfaLayerBuilder( + pageDiv, + pdfPage, + annotationStorage = null, + xfaHtml = null + ) {} } /** diff --git a/web/xfa_layer_builder.js b/web/xfa_layer_builder.js index b08c4e3b43ca3..fdab0c9012c80 100644 --- a/web/xfa_layer_builder.js +++ b/web/xfa_layer_builder.js @@ -15,6 +15,7 @@ /** @typedef {import("./interfaces").IPDFXfaLayerFactory} IPDFXfaLayerFactory */ +import { SimpleLinkService } from "./pdf_link_service.js"; import { XfaLayer } from "pdfjs-lib"; /** @@ -22,6 +23,7 @@ import { XfaLayer } from "pdfjs-lib"; * @property {HTMLDivElement} pageDiv * @property {PDFPage} pdfPage * @property {AnnotationStorage} [annotationStorage] + * @property {IPDFLinkService} linkService * @property {Object} [xfaHtml] */ @@ -29,10 +31,11 @@ class XfaLayerBuilder { /** * @param {XfaLayerBuilderOptions} options */ - constructor({ pageDiv, pdfPage, annotationStorage, xfaHtml }) { + constructor({ pageDiv, pdfPage, annotationStorage, linkService, xfaHtml }) { this.pageDiv = pageDiv; this.pdfPage = pdfPage; this.annotationStorage = annotationStorage; + this.linkService = linkService; this.xfaHtml = xfaHtml; this.div = null; @@ -54,6 +57,7 @@ class XfaLayerBuilder { xfa: this.xfaHtml, page: null, annotationStorage: this.annotationStorage, + linkService: this.linkService, intent, }; @@ -80,6 +84,7 @@ class XfaLayerBuilder { xfa, page: this.pdfPage, annotationStorage: this.annotationStorage, + linkService: this.linkService, intent, }; @@ -129,6 +134,7 @@ class DefaultXfaLayerFactory { pageDiv, pdfPage, annotationStorage, + linkService: new SimpleLinkService(), xfaHtml, }); } From 8cb6efec2d449f7c4599fb32f7d4cddddee876a7 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 30 Sep 2021 13:30:55 +0200 Subject: [PATCH 2/2] [api-minor] Add a wrapper around the `addLinkAttributes`-function, in the API, to the `PDFLinkService` implementations This patch helps reduce some duplication, given that we now have a few essentially identical `addLinkAttributes` call-sites in the code-base. To prevent runtime errors in the Annotation/XFA-layer code, we'll warn if a custom/incomplete `PDFLinkService` is being used (limited to GENERIC builds). --- src/display/annotation_layer.js | 29 ++++++++++++++--------------- src/display/display_utils.js | 3 +-- src/display/xfa_layer.js | 23 ++++++++++++++--------- web/base_viewer.js | 1 - web/interfaces.js | 7 +++++++ web/pdf_link_service.js | 28 +++++++++++++++++++++++++--- web/pdf_outline_viewer.js | 13 ++----------- web/secondary_toolbar.js | 4 ++-- web/toolbar.js | 4 ++-- 9 files changed, 67 insertions(+), 45 deletions(-) diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index ee1b47a720f33..1ec70bfb993a6 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -13,13 +13,6 @@ * limitations under the License. */ -import { - addLinkAttributes, - DOMSVGFactory, - getFilenameFromUrl, - LinkTarget, - PDFDateString, -} from "./display_utils.js"; import { AnnotationBorderStyleType, AnnotationType, @@ -30,6 +23,11 @@ import { Util, warn, } from "../shared/util.js"; +import { + DOMSVGFactory, + getFilenameFromUrl, + PDFDateString, +} from "./display_utils.js"; import { AnnotationStorage } from "./annotation_storage.js"; import { ColorConverters } from "../shared/scripting_utils.js"; @@ -443,14 +441,15 @@ class LinkAnnotationElement extends AnnotationElement { const link = document.createElement("a"); if (data.url) { - addLinkAttributes(link, { - url: data.url, - target: data.newWindow - ? LinkTarget.BLANK - : linkService.externalLinkTarget, - rel: linkService.externalLinkRel, - enabled: linkService.externalLinkEnabled, - }); + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && + !linkService.addLinkAttributes + ) { + warn( + "LinkAnnotationElement.render - missing `addLinkAttributes`-method on the `linkService`-instance." + ); + } + linkService.addLinkAttributes?.(link, data.url, data.newWindow); } else if (data.action) { this._bindNamedAction(link, data.action); } else if (data.dest) { diff --git a/src/display/display_utils.js b/src/display/display_utils.js index cf3d9bd6740fb..c34035f2cca3d 100644 --- a/src/display/display_utils.js +++ b/src/display/display_utils.js @@ -338,7 +338,7 @@ const LinkTarget = { /** * Adds various attributes (href, title, target, rel) to hyperlinks. - * @param {HTMLLinkElement} link - The link element. + * @param {HTMLAnchorElement} link - The link element. * @param {ExternalLinkParameters} params */ function addLinkAttributes(link, { url, target, rel, enabled = true } = {}) { @@ -633,7 +633,6 @@ function getXfaPageViewport(xfaPage, { scale = 1, rotation = 0 }) { export { addLinkAttributes, - DEFAULT_LINK_REL, deprecated, DOMCanvasFactory, DOMCMapReaderFactory, diff --git a/src/display/xfa_layer.js b/src/display/xfa_layer.js index 346a5b0e96e15..2844b5bff57ab 100644 --- a/src/display/xfa_layer.js +++ b/src/display/xfa_layer.js @@ -13,7 +13,7 @@ * limitations under the License. */ -import { addLinkAttributes, LinkTarget } from "./display_utils.js"; +import { warn } from "../shared/util.js"; import { XfaText } from "./xfa_text.js"; class XfaLayer { @@ -119,14 +119,19 @@ class XfaLayer { } if (isHTMLAnchorElement) { - addLinkAttributes(html, { - url: attributes.href, - target: attributes.newWindow - ? LinkTarget.BLANK - : linkService.externalLinkTarget, - rel: linkService.externalLinkRel, - enabled: linkService.externalLinkEnabled, - }); + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && + !linkService.addLinkAttributes + ) { + warn( + "XfaLayer.setAttribute - missing `addLinkAttributes`-method on the `linkService`-instance." + ); + } + linkService.addLinkAttributes?.( + html, + attributes.href, + attributes.newWindow + ); } // Set the value after the others to be sure overwrite diff --git a/web/base_viewer.js b/web/base_viewer.js index 3ce5d91977a2e..21de192141e13 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -1358,7 +1358,6 @@ class BaseViewer { * @param {PDFPage} pdfPage * @param {AnnotationStorage} [annotationStorage] - Storage for annotation * data in forms. - * @property {IPDFLinkService} linkService * @returns {XfaLayerBuilder} */ createXfaLayerBuilder(pageDiv, pdfPage, annotationStorage = null) { diff --git a/web/interfaces.js b/web/interfaces.js index ebe1ad5183200..ad9af67e13069 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -63,6 +63,13 @@ class IPDFLinkService { */ goToPage(val) {} + /** + * @param {HTMLAnchorElement} link + * @param {string} url + * @param {boolean} [newWindow] + */ + addLinkAttributes(link, url, newWindow = false) {} + /** * @param dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index cd83e271114df..4286f2e3a378f 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -15,6 +15,7 @@ /** @typedef {import("./interfaces").IPDFLinkService} IPDFLinkService */ +import { addLinkAttributes, LinkTarget } from "pdfjs-lib"; import { parseQueryString } from "./ui_utils.js"; /** @@ -227,6 +228,21 @@ class PDFLinkService { this.pdfViewer.scrollPageIntoView({ pageNumber }); } + /** + * Wrapper around the `addLinkAttributes`-function in the API. + * @param {HTMLAnchorElement} link + * @param {string} url + * @param {boolean} [newWindow] + */ + addLinkAttributes(link, url, newWindow = false) { + addLinkAttributes(link, { + url, + target: newWindow ? LinkTarget.BLANK : this.externalLinkTarget, + rel: this.externalLinkRel, + enabled: this.externalLinkEnabled, + }); + } + /** * @param {string|Array} dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. @@ -514,10 +530,7 @@ function isValidExplicitDestination(dest) { */ class SimpleLinkService { constructor() { - this.externalLinkTarget = null; - this.externalLinkRel = null; this.externalLinkEnabled = true; - this._ignoreDestinationZoom = false; } /** @@ -561,6 +574,15 @@ class SimpleLinkService { */ goToPage(val) {} + /** + * @param {HTMLAnchorElement} link + * @param {string} url + * @param {boolean} [newWindow] + */ + addLinkAttributes(link, url, newWindow = false) { + addLinkAttributes(link, { url, enabled: this.externalLinkEnabled }); + } + /** * @param dest - The PDF destination object. * @returns {string} The hyperlink to the PDF object. diff --git a/web/pdf_outline_viewer.js b/web/pdf_outline_viewer.js index 00cabc81d8d06..70feaa58a64f0 100644 --- a/web/pdf_outline_viewer.js +++ b/web/pdf_outline_viewer.js @@ -13,12 +13,8 @@ * limitations under the License. */ -import { - addLinkAttributes, - createPromiseCapability, - LinkTarget, -} from "pdfjs-lib"; import { BaseTreeViewer } from "./base_tree_viewer.js"; +import { createPromiseCapability } from "pdfjs-lib"; import { SidebarView } from "./ui_utils.js"; /** @@ -115,12 +111,7 @@ class PDFOutlineViewer extends BaseTreeViewer { const { linkService } = this; if (url) { - addLinkAttributes(element, { - url, - target: newWindow ? LinkTarget.BLANK : linkService.externalLinkTarget, - rel: linkService.externalLinkRel, - enabled: linkService.externalLinkEnabled, - }); + linkService.addLinkAttributes(element, url, newWindow); return; } diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js index 532e2d27491be..92cbf728b98d6 100644 --- a/web/secondary_toolbar.js +++ b/web/secondary_toolbar.js @@ -31,8 +31,8 @@ import { PDFSinglePageViewer } from "./pdf_single_page_viewer.js"; * @property {HTMLButtonElement} printButton - Button to print the document. * @property {HTMLButtonElement} downloadButton - Button to download the * document. - * @property {HTMLLinkElement} viewBookmarkButton - Button to obtain a bookmark - * link to the current location in the document. + * @property {HTMLAnchorElement} viewBookmarkButton - Button to obtain a + * bookmark link to the current location in the document. * @property {HTMLButtonElement} firstPageButton - Button to go to the first * page in the document. * @property {HTMLButtonElement} lastPageButton - Button to go to the last page diff --git a/web/toolbar.js b/web/toolbar.js index a9995d0a149a5..48e89dc6bc0ec 100644 --- a/web/toolbar.js +++ b/web/toolbar.js @@ -43,8 +43,8 @@ const PAGE_NUMBER_LOADING_INDICATOR = "visiblePageIsLoading"; * @property {HTMLButtonElement} presentationModeButton - Button to switch to * presentation mode. * @property {HTMLButtonElement} download - Button to download the document. - * @property {HTMLAElement} viewBookmark - Element to link current url of - * the page view. + * @property {HTMLAnchorElement} viewBookmark - Button to obtain a bookmark link + * to the current location in the document. */ class Toolbar {