feat: add text layer support to PDF viewer #237

fukudasjp · 2021-11-09T07:46:52Z

What do these changes do/fix?

Add showTextLayer option to render text layer on PDFViewer as it's necessary for implementing highlighting on PDF. It would be useful for text selection and in-document search feature for future. Users can disable text layer rendering when they don't need it (for such as rendering mini-map images).

For implementation, this PR does the following things, which are tied to commits:

Add @types/pdfjs-dist for better code
Control PDF rendering tasks properly in PDFViewer (properly handles cancelled rendering)
Add PDFViewerTextLayer component
- Update the storybook withShow text layer option

How do you test/verify these changes?

Use storybook to test the feature

Have you documented your changes (if necessary)?

No

Are there any breaking changes included in this pull request?

no

jhpedemonte

Tested locally in Storybook and works great. But have many suggestions/questions for the code.

...discovery-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewer.tsx

...-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewerTextLayer.tsx

.../discovery-react-components/src/components/DocumentPreview/components/PdfViewer/typings.d.ts

packages/discovery-styles/scss/components/document-preview/_document-preview-pdf-viewer.scss

jhpedemonte · 2021-11-15T16:47:10Z

...discovery-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewer.tsx

+  /**
+   * Render text layer
+   */
+  showTextLayer?: boolean;


Is there a reason for making this an option? Why not always show the text layer when rendering a PDF?

I think that we reuse this component to render thumbnail image or tile.

I'm worried that this would take up a lot of resources, if we need to create many "thumbnails". We'll have to test this out and make sure we don't overwhelm the browser.

SDU Annotator has a server-side process to create the thumbnails. I don't believe we will be able to make use of that, but a similar approach may be needed here.

OK. Let's think about it when we implement thumbnails. With that, our usage is only to display a (full-page) document and I drop the option.

package.json

packages/discovery-styles/scss/components/document-preview/_document-preview-pdf-viewer.scss

...discovery-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewer.tsx

jhpedemonte · 2021-11-18T23:23:39Z

...-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewerTextLayer.tsx

+
+const { RenderingCancelledException } = PDFJSLib as any;
+
+interface Props {


Are these just a subset of the PdfViewer props? If so, should this just extend those? Something like:

type Props = Pick<PdfViewerProps, 'loadedPage' | 'scale'> & { setTextLayerInfo?: ... };

Only scale prop is so. I updated the type.

Did this change? I see same Props object as previously (and my comment above still stands).

Updated to use the same property both in PdfViewer and PdfViewerTextLayer

...-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewerTextLayer.tsx

...react-components/src/components/DocumentPreview/components/PdfViewer/useAsyncFunctionCall.ts

...discovery-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewer.tsx

jhpedemonte · 2021-12-01T21:09:14Z

...-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewerTextLayer.tsx

+          const { textContent, viewport, scale, page } = loadedText;
+
+          const builder = new TextLayerBuilder({
+            textLayerDiv,
+            viewport,
+            eventBus: new EventBus(),
+            pageIndex: page - 1
+          });
+          signal.addEventListener('abort', () => builder.cancel());


I'm really liking this AbortController usage pattern. We should consider bringing this in to main Tooling (as an import from this library).

packages/discovery-styles/package.json

packages/discovery-styles/scripts/update-styles.sh

packages/discovery-styles/scss/components/document-preview/_document-preview-pdf-viewer.scss

packages/discovery-styles/scss/components/document-preview/_pdfjs_web_mixins.scss

lgtm-com · 2021-12-02T12:29:02Z

This pull request introduces 1 alert when merging 6ba0703 into f904849 - view on LGTM.com

new alerts:

1 for Useless conditional

lgtm-com · 2021-12-02T13:13:46Z

This pull request introduces 1 alert when merging 052336a into f904849 - view on LGTM.com

new alerts:

1 for Useless conditional

jhpedemonte

Code is looking good. I have a few more suggestions, plus a few more from previous discussions (which I haven't closed as "resolved").

With those fixed, approved.

jhpedemonte · 2021-12-02T17:12:17Z

packages/discovery-styles/scripts/update-styles.sh

@@ -0,0 +1,19 @@
+#!/bin/sh


Nit: let's rename this to something more clear, like update-styles-from-pdfjs

And we should update the NPM script name in package.json to match.

jhpedemonte · 2021-12-02T17:12:56Z

packages/discovery-styles/scripts/update-styles.sh

+PDFJS_SCSS=$BASEDIR/scss/components/document-preview/_pdfjs_web_mixins.scss
+node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"
+
+# perttier


Suggested change

# perttier

# prettier

jhpedemonte · 2021-12-02T17:13:30Z

packages/discovery-styles/scripts/generate-pdfjs_web_mixin.js

+const originalPdfjsWebCss = process.argv[2];
+const mixinPdfjsWebScss = process.argv[3];
+
+// load teh original style


Suggested change

// load teh original style

// load the original style

jhpedemonte · 2021-12-02T17:14:07Z

packages/discovery-styles/scripts/generate-pdfjs_web_mixin.js

+}
+`;
+
+fs.writeFileSync(mixinPdfjsWebScss, generatedCss, { encoding: 'utf-8' });


This is great. Thanks.

jhpedemonte · 2021-12-02T17:16:49Z

packages/discovery-styles/scripts/update-styles.sh

+# pdfjs textLayer styles
+PDFJS_WEB_CSS=$BASEDIR/../../node_modules/pdfjs-dist/web/pdf_viewer.css
+PDFJS_SCSS=$BASEDIR/scss/components/document-preview/_pdfjs_web_mixins.scss
+node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"


yarn v2 recommends running using yarn node instead of simply node, since yarn adds some environment values which may be necessary when importing.

Suggested change

node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"

yarn node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"

jhpedemonte · 2021-12-02T17:17:21Z

packages/discovery-styles/scripts/update-styles.sh

+node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"
+
+# perttier
+../../node_modules/.bin/prettier --write "$PDFJS_SCSS"


Suggested change

../../node_modules/.bin/prettier --write "$PDFJS_SCSS"

yarn run prettier --write "$PDFJS_SCSS"

fukudasjp added 3 commits November 9, 2021 14:37

chore: add pdfjs-dist typings

5e1902c

fix: control PDF rendering tasks properly

824c2d7

feat: add pdf text layer support

c5e09cb

fukudasjp requested review from JohnWColby, broulaye, jhpedemonte and noah-eigenfeld as code owners November 9, 2021 07:46

fukudasjp mentioned this pull request Nov 11, 2021

feat: add PDF viewer with highlighting #238

Merged

jhpedemonte reviewed Nov 15, 2021

View reviewed changes

fukudasjp added 2 commits November 16, 2021 22:33

fix: apply review comments

9f0dcd5

refactor: extract text rendering hook

cc06446

jhpedemonte reviewed Nov 18, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

packages/discovery-styles/scss/components/document-preview/_document-preview-pdf-viewer.scss Outdated Show resolved Hide resolved

jhpedemonte reviewed Nov 18, 2021

View reviewed changes

...discovery-react-components/src/components/DocumentPreview/components/PdfViewer/PdfViewer.tsx Outdated Show resolved Hide resolved

jhpedemonte reviewed Nov 18, 2021

View reviewed changes

fukudasjp added 6 commits November 19, 2021 23:46

refactor: extract async func call hook

de5731f

fix: revise how to import css from pdfjs

f64fa07

Merge remote-tracking branch 'upstream/master' into feat/redner-pdf-text

85e3538

fix: install @types/pdfjs-dist to yarn2

3c08df9

feat: add script to update style

c722221

refactor: revise script for importing css

69ff042

jhpedemonte reviewed Dec 1, 2021

View reviewed changes

fukudasjp added 5 commits December 2, 2021 20:43

fix: use postcss to manupulate pdfjs-web css

17feb71

fix: add comment to the style update script

158b8a2

refactor: move useAsyncFunctionCall to utils

0705d2e

fix: name of package script

9e9cad5

Merge remote-tracking branch 'upstream/master' into feat/redner-pdf-text

6ba0703

fix: apply CI comment

052336a

fix: fix broken logic

e081c92

fix: remove unused code

68d895d

jhpedemonte approved these changes Dec 2, 2021

View reviewed changes

fukudasjp added 3 commits December 3, 2021 13:57

fix: apply review comments around pdfjs css

8c82fae

fix: pdfjs typings version

2c28bd9

fix: apply review comments

032842c

fukudasjp merged commit b9dc342 into master Dec 6, 2021

fukudasjp deleted the feat/redner-pdf-text branch December 6, 2021 04:41

fukudasjp mentioned this pull request Dec 6, 2021

fix: fix build error #247

Merged


		const { RenderingCancelledException } = PDFJSLib as any;

		interface Props {

	node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"
	yarn node $BASEDIR/scripts/generate-pdfjs_web_mixin.js "$PDFJS_WEB_CSS" "$PDFJS_SCSS"

	../../node_modules/.bin/prettier --write "$PDFJS_SCSS"
	yarn run prettier --write "$PDFJS_SCSS"

feat: add text layer support to PDF viewer #237

feat: add text layer support to PDF viewer #237

Uh oh!

Conversation

fukudasjp commented Nov 9, 2021

What do these changes do/fix?

How do you test/verify these changes?

Have you documented your changes (if necessary)?

Are there any breaking changes included in this pull request?

Uh oh!

jhpedemonte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Dec 2, 2021

Uh oh!

lgtm-com bot commented Dec 2, 2021

Uh oh!

jhpedemonte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!