feat: add PDF viewer with highlighting #238

fukudasjp · 2021-11-10T06:22:44Z

What do these changes do/fix?

This depends on #237.

This PR is to add PDFViewerHighlighter component which is a highlight layer for PDF based on text spans on fields in search result document. This also implements PDFViewerWithHighlight which is PDFViewerHighlighter integrated with PDFViewer.

The implementation includes a logic to find calculate bboxes for highlighting. Overview of the logic is in README.md.

Add generic utilities used by the logic
- feat: add types and common utilities 45e5da8
Add an option to extract HTML source in bbox from HTML field to processDoc
- feat: add option for bbox text to processDoc c2ab1f6
Implement the logic to calculate bboxes for highlighting (please refer to README.md to get high-level view of the logic)
- feat: add text layer classes 20e4866
  - Implement TextLayouts
- feat: add highlighting logic and README 839f948
  - Implement textBoxMapping and Highlighter
Implement Rect components and storybook
- feat: add PDF highlight component 67c0ac9

How do you test/verify these changes?

Open the storybook DocumentPreview > components > PdfViewerWithHighlight > with text selection,
In the right pane, select a field except for html from the dropdown on the top
Select a range on the text
Then, verify that the text on PDF is highlighted

Have you documented your changes (if necessary)?

Are there any breaking changes included in this pull request?

jhpedemonte · 2021-11-15T17:29:25Z

How do you test/verify these changes?

Can you please add to this section?

Did a quick test in Storybook. I see the new PdfViewerWithHighlight section and I can select text on the right and see it appear on the left when selecting text[0] (I assume this is what it is supposed to show off). However, I don't see the same when selecting text in either header[0] or footer[0] -- should those work, too?

jhpedemonte

One of the major things you were investigating was highlighting on Japanese text, correct? You should include a sample doc to show this off (similar to __fixtures__/Art Effects.pdf).

...mponents/src/components/DocumentPreview/components/PdfViewerHighlight/PdfViewerHighlight.tsx

...ents/src/components/DocumentPreview/components/PdfViewerHighlight/PdfViewerWithHighlight.tsx

...umentPreview/components/PdfViewerHighlight/utils/textBoxMapping/MappingTargetCellProvider.ts

.../components/DocumentPreview/components/PdfViewerHighlight/utils/textLayout/BaseTextLayout.ts

...s/DocumentPreview/components/PdfViewerHighlight/utils/textLayout/PdfTextContentTextLayout.ts

...ponents/src/components/DocumentPreview/components/PdfViewerHighlight/utils/textLayout/dom.ts

fukudasjp · 2021-11-16T14:00:47Z

Did a quick test in Storybook. I see the new PdfViewerWithHighlight section and I can select text on the right and see it appear on the left when selecting text[0] (I assume this is what it is supposed to show off). However, I don't see the same when selecting text in either header[0] or footer[0] -- should those work, too?

Thank you for pointing this out. They should work. I found some issue with them and will fix.

fukudasjp · 2021-11-18T12:10:24Z

One of the major things you were investigating was highlighting on Japanese text, correct? You should include a sample doc to show this off (similar to fixtures/Art Effects.pdf).

I just added a small Japanese PDF sample.

- add return type - add document to methods/classes

jhpedemonte · 2021-12-06T16:59:33Z

The story shows selecting text and having it highlighted on the PDF. Does the reverse work? I'm thinking of scenario where user is presented with a PDF and selects some text and app needs to figure out which text-based enrichment it belongs to.
Have you tested scanned (non-programmatic) PDFs yet, which have gone through OCR? I'm wondering how well this code works in those cases.
- Also as mentioned, we'll need to consider scanned documents which are skewed and how we handle highlighting there. I think the suggestion at the time (from Mauricio?) was that we may have enough info to rotate the original document so it doesn't appear skewed any more. In which case, highlighting shouldn't be an issue.

jhpedemonte

Looks good overall and functions well in Storybook. Found a few minor things. With those fixed, approved.

jhpedemonte · 2021-12-06T17:03:01Z

...mponents/src/components/DocumentPreview/components/PdfViewerHighlight/PdfViewerHighlight.tsx

+
+  /**
+   * Flag to whether or not to use bbox information from html field in the document.
+   * True by default. This is for testing and debugging purpose.


I think that we should better denote that these properties aren't meant to be used normally by end users. Best I can think is to prepend an underscore to the name:

_useHtmlBbox?: boolean;

...onents/src/components/DocumentPreview/components/PdfViewerHighlight/utils/common/nonEmpty.ts

jhpedemonte · 2021-12-06T17:23:48Z

...components/DocumentPreview/components/PdfViewerHighlight/PdfViewerWithHighlight.stories.scss

@@ -0,0 +1,25 @@
+.withTextSelection {
+  display: flex;
+  height: 800px;


Why does this have an explicit height? Is it possible to make it relative?

jhpedemonte · 2021-12-06T17:26:21Z

...components/DocumentPreview/components/PdfViewerHighlight/PdfViewerWithHighlight.stories.scss

+
+  .highlight {
+    opacity: 0.4;
+    background: rgba(255, 64, 128, 1);


We should check what Carbon says about highlighting text and which colors/opacity to use.

Only thing I found so far is $highlight from https://www.carbondesignsystem.com/guidelines/color/usage. That's fine if there's a single highlight in the component, but we'll need more guidance if we have to handle multiple highlight colors.

I update the code to use the $highlight color so far.

jhpedemonte · 2021-12-06T17:38:08Z

...nents/src/components/DocumentPreview/components/PdfViewerHighlight/utils/common/bboxUtils.ts

+  const heightB = bottomB - topB;
+
+  // compare height ratio
+  const OVERLAP_RATIO = 0.8;


Move this constant out of the function and to the top of the file.

Also, what is the purpose of this constance? How was it calculated? Add a description comment.

jhpedemonte · 2021-12-06T17:40:49Z

...onents/src/components/DocumentPreview/components/PdfViewerHighlight/utils/common/nonEmpty.ts

+ */
+export function nonEmpty<T>(value: T | null | undefined): value is T {
+  return value !== null && value !== undefined;
+}


This function is generic and not specific to PdfViewerHighlight. Move it up to the top-level utils/.

jhpedemonte · 2021-12-06T17:42:08Z

...ponents/DocumentPreview/components/PdfViewerHighlight/utils/textBoxMapping/TextBoxMapping.ts

+}
+
+/**
+ * Text box mapping


This doesn't really explain this component. What does this map between?

jhpedemonte · 2021-12-06T17:43:48Z

...omponents/DocumentPreview/components/PdfViewerHighlight/utils/textBoxMapping/TextProvider.ts

+const MAX_HISTORY = 3;
+
+export type TextMatch = {
+  /** matched text span */


For single line comments like these, let's keep with convention of just starting with // and leave /** for multi-line comments (here and elsewhere in PR).

(Wonder if there's an eslint/prettier plugin for this?)

jhpedemonte · 2021-12-06T17:46:42Z

packages/discovery-react-components/src/components/DocumentPreview/utils/box.ts

+export const LEFT = 0;
+export const TOP = 1;
+export const RIGHT = 2;
+export const BOTTOM = 3;


Where are these used?

They were used in dom.ts. I refactored the code to use destructuring assignment.

jhpedemonte · 2021-12-06T17:48:13Z

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

+  opacity: 0.5;
+  background: rgba(0, 0, 255, 1);


Same here: need to check with Design about use of highlight color/opacity.

OK. Use the Carbon color with opacity as a default and check with design.

fukudasjp · 2021-12-08T12:44:24Z

The story shows selecting text and having it highlighted on the PDF. Does the reverse work? I'm thinking of scenario where user is presented with a PDF and selects some text and app needs to figure out which text-based enrichment it belongs to.

Not yet, but I agree that it has to be considered to make this work as editor, too. I'm thinking to add a selection handler such as onSelect({ field, fieldIndex, span }) to the component

Have you tested scanned (non-programmatic) PDFs yet, which have gone through OCR? I'm wondering how well this code works in those cases.

I tried scanned document. Currently no highlight is shown on scanned document and need to fix it. On scanned PDF, highlight relies on bboxes in HTML field. With a document I tried, the bboxes spans on multiple lines and highlight were not accurate. Smaller bboxes are required for more accuracy.

* origin/master: chore: publish v1.5.0-beta.10 [ci skip] build: support Node 16 (#268) chore: publish v1.5.0-beta.9 [ci skip] chore: create a global rollup cmd (#262) chore: publish v1.5.0-beta.8 [ci skip] feat: document provider interface (#249) fix: mitigate overlapped or segmented highlight on PDF (#252) chore: publish v1.5.0-beta.7 [ci skip] fix: fix PDF highlight misalignment issue (#253) chore: publish v1.5.0-beta.6 [ci skip] chore: update Carbon to 10.46.0/7.46.0 (#254) chore: update Carbon to 10.46.0/7.46.0 (#250) chore: publish v1.5.0-beta.5 [ci skip] feat: add PDF viewer with highlighting (#238)

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

jhpedemonte reviewed Nov 15, 2021

View reviewed changes

fix: apply review comments

9f0dcd5

refactor: extract text rendering hook

cc06446

fukudasjp added 21 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

feat: add types and common utilities

c0771fc

feat: add option for bbox text to processDoc

66c7c56

feat: add text layer classes

47855f4

feat: add highlighting logic and README

66b3e43

feat: add PDF highlight component

e43318a

fix: fix readme

1d46e11

fix: revise readme

9b33674

fix: fix readme

73c2ff8

refactor: extract logic of iterating range rects

050db2d

fix: apply review comments

f62c419

- add return type - add document to methods/classes

fix: remove unnecessary commets

3e06fda

fix: highlighting on header and footer

b1defdd

fix: fix boxUtil test failure

7a6ef58

refactor: use one bbox intersection logic

3d02caf

feat: add Japanese PDF sample

3b810af

fukudasjp added 2 commits December 3, 2021 14:57

fix: apply review comments

032842c

Merge branch 'feat/redner-pdf-text' into feat/highligh-on-pdf

58ec1c5

Base automatically changed from feat/redner-pdf-text to master December 6, 2021 04:41

fukudasjp added 9 commits December 6, 2021 14:42

Merge remote-tracking branch 'upstream/master' into feat/highligh-on-pdf

0ccaf6c

refactor: refactor common utils

3eedcf8

refactor: refacto getTextBoxMapping

c13942b

fix: fix yarn error

a9dd38e

fix: fix test error

f6fbcd2

refactor: move utility methods

dc09472

fix: apply review comments

0967b05

fix: remove unnecessary change

bb448d5

chore: add comment

d651ea8

fukudasjp changed the base branch from master to fix-yarn-lock-build-error December 6, 2021 13:01

fukudasjp changed the base branch from fix-yarn-lock-build-error to master December 6, 2021 13:02

fukudasjp marked this pull request as ready for review December 6, 2021 13:02

fukudasjp requested review from JohnWColby, broulaye and noah-eigenfeld as code owners December 6, 2021 13:02

jhpedemonte approved these changes Dec 6, 2021

View reviewed changes

fukudasjp added 5 commits December 7, 2021 15:35

Merge remote-tracking branch 'upstream/master' into feat/highligh-on-pdf

fbac5ee

chore: upadte yarn lock file

e1fe864

fix: apply review comments

0911538

refactor: extract common props

457f2ec

feat: export PdfViewerWithHighlight via DocPreview

89e4f75

fix: fix app build

21799dc

fukudasjp merged commit 5e06d62 into master Dec 9, 2021

fukudasjp deleted the feat/highligh-on-pdf branch December 9, 2021 05:12

feat: add PDF viewer with highlighting #238

feat: add PDF viewer with highlighting #238

Uh oh!

Conversation

fukudasjp commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Nov 15, 2021

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!

Uh oh!

fukudasjp commented Nov 16, 2021

Uh oh!

fukudasjp commented Nov 18, 2021

Uh oh!

jhpedemonte commented Dec 6, 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!

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!

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!

fukudasjp commented Dec 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

fukudasjp commented Nov 10, 2021 •

edited

Loading