From be89d381812aeb769a14768cfbddad013fdbf096 Mon Sep 17 00:00:00 2001
From: Brian Birtles <birtles@gmail.com>
Date: Sat, 23 Apr 2022 17:04:06 +0900
Subject: [PATCH] fix: fix handling of hold-to-show key for text elements with
 alt/title attributes

Fixes #946.
---
 CHANGELOG.md            |  4 +++-
 src/content/get-text.ts | 16 +++++++++++++--
 tests/get-text.test.ts  | 45 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d3adb1c1e1..0c030c2382 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,7 +8,9 @@ app.
 
 ## [Unreleased]
 
-(Nothing yet)
+- Made the popup not automatically show for text elements with `title`/`alt`
+  attributes when a key is required to show the popup
+  ([#946](https://github.com/birchill/10ten-ja-reader/issues/946)).
 
 ## [1.8.4] - 2022-04-07
 
diff --git a/src/content/get-text.ts b/src/content/get-text.ts
index 3663e60821..aaa0d3b04f 100644
--- a/src/content/get-text.ts
+++ b/src/content/get-text.ts
@@ -206,7 +206,7 @@ export function getTextAtPoint({
 
   const elem = document.elementFromPoint(point.x, point.y);
   if (elem) {
-    const text = getTextFromRandomElement({ elem, matchImages });
+    const text = getTextFromRandomElement({ elem, matchImages, matchText });
     if (text) {
       const result: GetTextAtPointResult = { text, textRange: null };
       previousResult = { point, position: undefined, result };
@@ -231,6 +231,11 @@ export function getTextAtPoint({
   return null;
 }
 
+// For unit testing
+export function clearPreviousResult() {
+  previousResult = undefined;
+}
+
 function caretPositionFromPoint(
   point: Point
 ): (CursorPosition & { usedCaretRangeFromPoint?: boolean }) | null {
@@ -846,9 +851,11 @@ function getTextFromCoveringLink({
 function getTextFromRandomElement({
   elem,
   matchImages,
+  matchText,
 }: {
   elem: Element;
   matchImages: boolean;
+  matchText: boolean;
 }): string | null {
   // Don't return anything for an iframe since this script will run inside the
   // iframe's contents as well.
@@ -856,7 +863,12 @@ function getTextFromRandomElement({
     return null;
   }
 
-  if (!matchImages && elem.tagName === 'IMG') {
+  // We divide the world into two types of elements: image-like elements and the
+  // rest which we presume to be "text" elements.
+  const isImage = ['IMG', 'PICTURE', 'VIDEO'].includes(elem.tagName);
+  if (isImage && !matchImages) {
+    return null;
+  } else if (!isImage && !matchText) {
     return null;
   }
 
diff --git a/tests/get-text.test.ts b/tests/get-text.test.ts
index f1a7a49906..031a6a6ba0 100644
--- a/tests/get-text.test.ts
+++ b/tests/get-text.test.ts
@@ -1,6 +1,10 @@
 import { assert } from 'chai';
 
-import { getTextAtPoint, GetTextAtPointResult } from '../src/content/get-text';
+import {
+  clearPreviousResult,
+  getTextAtPoint,
+  GetTextAtPointResult,
+} from '../src/content/get-text';
 import { empty } from '../src/utils/dom-utils';
 import { isChromium } from '../src/utils/ua-utils';
 
@@ -20,7 +24,8 @@ describe('getTextAtPoint', () => {
   });
 
   afterEach(() => {
-    document.getElementById('test-div')!.remove();
+    testDiv.remove();
+    clearPreviousResult();
   });
 
   it('should find a range in a div', () => {
@@ -1336,6 +1341,42 @@ describe('getTextAtPoint', () => {
     assertTextResultEqual(result, 'あいうえお');
   });
 
+  it('should pull the text out of a title attribute on an image even when matchText is false', () => {
+    testDiv.innerHTML = '<img src="" title="あいうえお">';
+    const imgNode = testDiv.firstChild as HTMLImageElement;
+    imgNode.style.width = '200px';
+    imgNode.style.height = '200px';
+    const bbox = imgNode.getBoundingClientRect();
+
+    const result = getTextAtPoint({
+      point: {
+        x: bbox.left + bbox.width / 2,
+        y: bbox.top + bbox.height / 2,
+      },
+      matchText: false,
+      matchImages: true,
+    });
+
+    assertTextResultEqual(result, 'あいうえお');
+  });
+
+  it('should NOT pull the text out of a title attribute on a text node when matchText is false', () => {
+    testDiv.innerHTML = '<span title="あいうえお">Not Japanese text</span>';
+    const span = testDiv.firstChild as HTMLSpanElement;
+    const bbox = span.getBoundingClientRect();
+
+    const result = getTextAtPoint({
+      point: {
+        x: bbox.left + bbox.width / 2,
+        y: bbox.top + bbox.height / 2,
+      },
+      matchText: false,
+      matchImages: true,
+    });
+
+    assert.strictEqual(result, null);
+  });
+
   it("should use the last result if there's no result but we haven't moved far", () => {
     testDiv.append('abcdefあいうえお');
     const textNode = testDiv.firstChild as Text;