1976869 - Copy to Highlight option is not available/disabled on specific selection

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Adrian Florinescu [:aflorinescu]]

Found in

• Nightly 142.0a1;

Affected versions

• Nightly 142.0a1;

Tested platforms

• Affected platforms: Windows 11, Mac 13, Ubuntu 24

Preconditions

• dom.text_fragments.enabled to TRUE
• dom.text_fragments.create_text_fragment.enabled set to TRUE

Steps to reproduce

1. Access https://en.wikipedia.org/wiki/Nuclear_weapon
2. Find the paragraph that starts with "The first nuclear weapons were developed by the Allied Manhattan Project "
3. Do a selection that starts with "weapons were developed by the Allied Manhattan Project ..." and ends at the end of the paragraph.
4. Right click/Context menu.

Expected result

• Copy to Highlight is available.

Actual result (see recording)

• Copy to Hightlight is disabled.

Comment 1

[Adrian Florinescu [:aflorinescu]]

Attachment: copy-highlight-inactive.mp4

Comment 2

[Jan Jaeschke [:jjaschke]]

This bug was very easy to fix, but is surprisingly hard to explain (and write a test case for).

The issue here lies in the algorithm that computes common string lengths, which compares a reference string with text at a given boundary point (ie., this is not a string, but points to a DOM node+offset, and the algorithm needs to traverse the DOM until there's a mismatch or a block boundary). The actual computation of the common length happens conditionally inside of the loop through the text nodes. If for some reason this loop is not executed, the function returns the length of the reference string (and this is wrong!).

This happened in the test case above, which uses range-based matching. Therefore, the algorithm searches for all occurrences of the first word of the range ("weapons") in the partial document in order to find out how much the start term would need to be extended. The search found 5 results (to reproduce: Open the page, use find-in-page to search for "weapons" and tab through the results). Result 3 is at the end of a block boundary (in the box on the right side of the document, just above the picture). Therefore, in the algorithm, the outer loop runs once (for the text node), but since the direction here is LTR, the offset is already at text->GetLength(), so that the inner loop is not run, and the length of the reference string is returned (which in this case was very long).

Coincidentally, a very similar issue happens in this case for the same match when looking at the other direction to expand the prefix. In that case, there is some text to the left of the boundary point until a block boundary is reached ("Nuclear"), which happens to be the identical to the prefix for the start term ("The first nuclear"). However, instead of returning the correct length (which is 7), the algorithm exited the inner loop early by exhausting textLength, again without running the actual computation part. So, here the returned "common length" was the length of aReferenceString.

Since the length of aReferenceString is the maximum possible length to which the text directive candidate can be extended, the algorithm later determined that it's not possible to find a combination for prefix and start, because to "eliminate" this match both prefix and start would have to be extended further than the maximum possible expansion length.

The fix for this is to move the computation part out of the loop to make sure that it's running in any case, and to only return the length of aReferenceString if actually aReferenceString.Length() characters are equal.

Comment 3

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1976869 - Text Fragments: Fixed edge cases when computing common string lengths. r=mccr8!,#dom-core

In some edge cases (eg. boundary point already at end of block), the algorithm would
return the length of the reference string as common substring length (instead of 0).

This is fixed by moving the actual common length computation part out of the loop
(which in some conditions may not run).

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/45a81587b0b7
https://hg.mozilla.org/integration/autoland/rev/98ab67029e11
Text Fragments: Fixed edge cases when computing common string lengths. r=mccr8,dom-core

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/98ab67029e11

Comment 6

[Adrian Florinescu [:aflorinescu]]

Verified during 143 Nightly testing on Mac and Ubuntu with additional manual verification for Windows.