The `mergeClientRects` resolution of the `block-contains` & `block-is-contained` edge case

[oleksandr-danylchenko]

TL;DR;

When the content contains elements with the vertical-align style - the mergeClientRects can unexpectedly preserve selection only over it, ignoring everything before.

Interactive playground

Skip to the question

App:

selection_dismissed_bug.mp4

@recogito/text-annotator sandbox:

selection_bug.mp4

Issue

I tracked down that the "dismissal" of the content before the "super" element happens in the mergeClientRects function.

It establishes the block-is-contained relation between the "super" element rect and the rects on the left:

Rects	Relation

text-annotator-js/packages/text-annotator/src/utils/mergeClientRects.ts

Lines 51 to 54 in 5b9abad

	} else if (a.top >= b.top && a.bottom <= b.bottom) {
	if (a.left >= b.left && a.right <= b.right) {
	return 'block-is-contained';
	}

After, that the merging code picks the rect with the smaller width:

So the whole left part rect gets replaced with the "super" element rect!

text-annotator-js/packages/text-annotator/src/utils/mergeClientRects.ts

Lines 94 to 101 in 5b9abad

	} else if (relation === 'block-contains' || relation === 'block-is-contained') {
	// Block containment - keep the element with smaller width
	if (rectA.width < rectB.width) {
	next = next.map(r => r === rectB ? rectA : r);
	}
	wasMerged = true;
	break;
	}

Reproduction

The issue can be reproduced in the @recogito/text-annotator sandbox (Interactive playground):

<p>In 2020, the U.S. unemployment rate rose from a longtime record low of 3.5 percent in February to 14.7 percent in April. This represents a loss of 30 million jobs or about 18 percent of the labor force prior to the COVID-19 
<span class="footnote-ref-tail">pandemic.
	<a class="footnote-ref" name="footnote-ref-1" href="#footnote-1">1</a>
</span> To put this total in context
</p>

.footnote-ref-tail {
  white-space: nowrap;
}

.footnote-ref {
  display: inline-block;
  min-width: 20px;
  background-clip: content-box;
  vertical-align: super;
  color: #5f01df;
  font-size: 14px;
  font-style: normal;
  font-weight: bold;
  line-height: 17px;
  text-align: center;
  text-decoration: none;
  cursor: pointer;
  position: relative;
  top: 5px;
}

.footnote-ref::before {
  content: '[';
}
.footnote-ref::after {
  content: ']';
}

Question

Should we consider using the wider rect instead of the smaller one as a replacement target? That way we'll pick the container rect instead of the one that's being contained. It sounds like a more sensible approach to me 🤔

[rsimon]

Sigh... mergeClientRects really is the oldest (and nastiest) construction site in the codebase. I've been meaning to replace it altogether, but didn't get a chance to think through the whole thing yet.

Using the wider rect might solve this problem. It's worth a shot, but I think it might kick us back to the case where all the individual lines would get replaced by the large block-level element boxes (p etc.)

To solve this cleanly, mergeClientRects really needs to go back to the drawing board. I'll try to think through it one the weekend. Meanwhile, if you have any ideas–do let me know. (Note that there's also this ill-fated attempt of mine from a while back which is still commented out in the source file, but also didn't properly work.)

[oleksandr-danylchenko]

Thanks for the feedback!
Yeah, I saw the draft but didn't investigate it yet. I tried using the wider rect on a limited set of examples, and it seems to make the selection work right for the vertical-align elements (or any elements that have a distinct small rect). I can take a look at your suggested algo and check how it works on the existing content sometime this or next week.

However, an alternative solution that I can apply on my app's level is to mark the footnote-ref or any similar elements as not-annotatable. In my case, that should also be acceptable and won't require changing the mergeClientRects that might backfire sometime later. The not-annotatable will force the selector split, so the left part rects won't be accidentally replaced with the smaller rect.

UPD: Here's the result of using the not-annotatable:

footnote_selection_fix.mp4

[oleksandr-danylchenko]

Hello, @rsimon!
I got a chance to take a look at the suggested mergeClientRects, and it makes sense to me! I only added a few stylistic suggestions to remove the argument mutations - #190.
Do you have a remembrance of the cases that made the new function break and led to the revert? At the first glance, it works well in the sandbox. And even handles the problematic selection case correctly.