fix: fix and improve PDF highlighting by fukudasjp · Pull Request #313 · watson-developer-cloud/discovery-components

fukudasjp · 2022-02-27T13:33:44Z

What do these changes do/fix?

fix redundant DOM elements fix: fix and improve PDF highlighting #313 (comment)
fix text mapping fallback fix: fix and improve PDF highlighting #313 (comment)
fix fragmented highlights fix: fix and improve PDF highlighting #313 (comment)
fix PDF text mappings fix: fix and improve PDF highlighting #313 (comment)
improve PDF text mappings by giving priority to longer matches. So, updated the code to repeat text match for the minimum length of 27, 9, 3, 1. Added minLength parameter to many of text match functions. fix: fix and improve PDF highlighting #313 (comment)

How do you test/verify these changes?

Please try the storybook and see if PDF highlights works as before.

Have you documented your changes (if necessary)?

n/a

Are there any breaking changes included in this pull request?

no

- story for user-provided PDF and document JSON - fix cmap URL - make storybook provide cmap files

- to fix highlight boxes - for more efficient text box mapping

- give priority to longer match - add min match length to match functions - support `rewind` in cell provider

fukudasjp · 2022-02-27T13:36:39Z

...ery-react-components/src/components/DocumentPreview/components/PdfHighlight/PdfHighlight.tsx

+  if (shape?.boxes.length === 0) {
+    return null;
+  }


this is to avoid useless empty <div/> containers being generated. The divs are generated for all the highlights in a document regardless of the page where the highlight exists. With this fix, divs are generated for highlights on the current page.

fukudasjp · 2022-02-27T13:46:32Z

...-components/src/components/DocumentPreview/components/PdfHighlight/utils/common/bboxUtils.ts

+  if (horizontalGap < 0) {
+    // gap < 0 means that the boxes are overlapped
+    const overlap = -horizontalGap;
+    // consider two horizontally aligned overlapped boxes are next to each other when
+    // the overlap is smaller than half of the box height.
+    return overlap < avgHeight / 2;
+  }


the previous code of disallowing any overlapping was too strict.

if (bboxesIntersect(boxA, boxB)) { return false; }

Practically, PDF sometimes includes bboxes overlapped each other for a text. It caused an issue of fragmented highlights like the following screenshot.

So, this code is to allow that kind of overlapping. It will fix the issue of split highlights.

I don't fully understand why you are using height to determine if boxes are horizontally aligned. That seems odd. What's the reasoning behind that?

Not sure, but my guess is that this is for picking a small size that is relative to the size of the characters. (Width wouldn't work due to differences in word length.) My initial thought would be to pick a specific small size, but this solution better accounts for potential issues with zoom.

This is to to adapt to various font sizes. For font height of 72px, 30px is small enough. But for height of 12px, 30px overlapping means that some characters are most likely overlapped. In the case, given boxes cannot be considered as 'next to each other'.

fukudasjp · 2022-02-27T13:52:31Z

...components/DocumentPreview/components/PdfHighlight/utils/textBoxMapping/getTextBoxMapping.ts

      markSourceAsMapped: (text: string) => {
        const mappedSource = matchedSourceProvider.getMatch(text);
+        if (mappedSource?.span) {
+          matchedSourceProvider.consume(mappedSource.span);
+        }
        debug('>> target cell %o to source %o', text, mappedSource);
        return mappedSource?.span;
      },
-      markSourceMappedBySpan: (span: TextSpan) => {
-        if (spanLen(span) > 0) {
-          matchedSourceProvider.consume(span);
-        }
+      markAsMapped: () => {
+        matchedSourceProvider.consume(matchedSourceSpan);
      }


These changes are to consume text in source cell properly.

Without the first one, a single character in a source could be mapped to multiple characters in target (i.e. bbox in PDF text item). Without the second one, many characters in target couldn't be mapped to source text.

Add comments to explain the code.

fukudasjp · 2022-03-02T08:54:42Z

...react-components/src/components/DocumentPreview/components/PdfHighlight/utils/Highlighter.ts

+      const mappedItems = doMapping(items, textToPdfTextItemMappings, this.textMappingsLayout);
+      if (mappedItems.length > 0) {
+        items = mappedItems;
+      }


When the length of the result of doMapping is zero, it means that mapping fails to find bboxes on PDF using PDF text items.

In the case, mapping from bboxes on HTML fields should be used as a fallback. But the previous code didn't do that.

So, this is to fix the behavior. In the updated code, when mapping returns bboxes, use them. Fall back to use bbox form HTML fields otherwise.

Please add a comment in the code about this, since the intent isn't evident just from the code itself.

fukudasjp · 2022-03-02T10:39:29Z

...components/DocumentPreview/components/PdfHighlight/utils/textBoxMapping/getTextBoxMapping.ts

+  for (const minMatchLength of [27, 9, 3, 1]) {
+    debug('getTextBoxMapping: processText with minMatchLength: %d', minMatchLength);


Match longer chunks first to fix scattered highlights

jhpedemonte · 2022-03-03T15:34:26Z

Not sure if related to this PR or if it's a separate issue, but in Storybook > DocumentPreview > table highlight, when selecting the HTML option in Knobs, it fails to render and shows this error:

Uncaught Error: Failed to find text node. Node: Information Security., offset: 274502
    at getTextNodeAndOffset (documentUtils.ts:108:1)
    at findOffsetInDOM (documentUtils.ts:53:1)
    at HtmlView.tsx:153:1
    at Array.forEach (<anonymous>)
    at HtmlView.tsx:147:1
    at invokePassiveEffectCreate (react-dom.development.js:23487:1)
    at HTMLUnknownElement.callCallback (react-dom.development.js:3945:1)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:1)
    at invokeGuardedCallback (react-dom.development.js:4056:1)
    at flushPassiveEffectsImpl (react-dom.development.js:23574:1)

jhpedemonte

Code looks good. Have some minor suggestions to add code comments, plus a few questions. With those fixed, approved.

jhpedemonte · 2022-03-03T15:49:45Z

...react-components/src/components/DocumentPreview/components/PdfHighlight/utils/Highlighter.ts

+      const mappedItems = doMapping(items, textToPdfTextItemMappings, this.textMappingsLayout);
+      if (mappedItems.length > 0) {
+        items = mappedItems;
+      }


Please add a comment in the code about this, since the intent isn't evident just from the code itself.

jhpedemonte · 2022-03-03T15:51:42Z

...-components/src/components/DocumentPreview/components/PdfHighlight/utils/common/bboxUtils.ts

+  if (horizontalGap < 0) {
+    // gap < 0 means that the boxes are overlapped
+    const overlap = -horizontalGap;
+    // consider two horizontally aligned overlapped boxes are next to each other when
+    // the overlap is smaller than half of the box height.
+    return overlap < avgHeight / 2;
+  }


I don't fully understand why you are using height to determine if boxes are horizontally aligned. That seems odd. What's the reasoning behind that?

jhpedemonte · 2022-03-03T15:55:37Z

...components/DocumentPreview/components/PdfHighlight/utils/textBoxMapping/getTextBoxMapping.ts

@@ -137,12 +138,12 @@ class Source<SourceCell extends TextLayoutCell, TargetCell extends TextLayoutCel
   * @param text


Add mention of param minLength and what it does.

jhpedemonte · 2022-03-03T15:56:24Z

...components/DocumentPreview/components/PdfHighlight/utils/textBoxMapping/getTextBoxMapping.ts

      markSourceAsMapped: (text: string) => {
        const mappedSource = matchedSourceProvider.getMatch(text);
+        if (mappedSource?.span) {
+          matchedSourceProvider.consume(mappedSource.span);
+        }
        debug('>> target cell %o to source %o', text, mappedSource);
        return mappedSource?.span;
      },
-      markSourceMappedBySpan: (span: TextSpan) => {
-        if (spanLen(span) > 0) {
-          matchedSourceProvider.consume(span);
-        }
+      markAsMapped: () => {
+        matchedSourceProvider.consume(matchedSourceSpan);
      }


Add comments to explain the code.

fukudasjp · 2022-03-04T07:06:16Z

In most cases, this improves highlight accuracy, but there are some sideeffects by changing the algorithm. I'll further update the algorithm in upcoming PR.

(left: highlight is shown, right: highlight is missing - this reproduces when same text are repeated (in title and body in this case))

fukudasjp added 7 commits February 27, 2022 21:37

chore: update yarn.lock

c76e552

feat: add storybook for user's PDF

2d320ac

- story for user-provided PDF and document JSON - fix cmap URL - make storybook provide cmap files

feat: adjust bbox next to each other condition

c77169e

- to fix highlight boxes - for more efficient text box mapping

fix: use html bbox mapping

c08dd88

fix: eliminate unnecessary dom element

a047c71

feat: improve text box mapping

9827c60

- give priority to longer match - add min match length to match functions - support `rewind` in cell provider

fix: fix invalid mapping

5898089

fukudasjp commented Feb 27, 2022

View reviewed changes

fukudasjp added 2 commits March 2, 2022 16:07

Merge remote-tracking branch 'origin/master' into fix/pdf-highlight

66860f2

fix: fix highlight code

4f6bcac

fukudasjp commented Mar 2, 2022

View reviewed changes

fukudasjp requested a review from wataken-ibm March 2, 2022 10:58

fukudasjp marked this pull request as ready for review March 2, 2022 10:59

fukudasjp requested review from JohnWColby, broulaye, jhpedemonte and noah-eigenfeld as code owners March 2, 2022 10:59

jhpedemonte approved these changes Mar 3, 2022

View reviewed changes

fix: apply review comment

9fab717

fukudasjp merged commit cc53e51 into master Mar 4, 2022

fukudasjp deleted the fix/pdf-highlight branch March 4, 2022 07:06

fukudasjp mentioned this pull request Mar 7, 2022

fix: fix PDF highlighting #319

Merged

fukudasjp mentioned this pull request Mar 28, 2022

fix: fix pdf text box mapping #328

Open

		for (const minMatchLength of [27, 9, 3, 1]) {
		debug('getTextBoxMapping: processText with minMatchLength: %d', minMatchLength);

		@@ -137,12 +138,12 @@ class Source<SourceCell extends TextLayoutCell, TargetCell extends TextLayoutCel
		* @param text

Conversation

fukudasjp commented Feb 27, 2022 • 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!

fukudasjp Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fukudasjp Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jhpedemonte commented Mar 3, 2022

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!

fukudasjp commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

fukudasjp commented Feb 27, 2022 •

edited

Loading

fukudasjp Feb 27, 2022 •

edited

Loading

fukudasjp Feb 27, 2022 •

edited

Loading