fix: fix handling of hold-to-show key for text elements with alt/titl…

…e attributes

Fixes #946.

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
 

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,17 +851,24 @@ 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.
   if (elem.nodeName === 'IFRAME') {
     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;
   }
 

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;