Skip to content

[chrome] Fix text selection with .markedContent #19785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Apr 7, 2025

Commit message:

The current text layer approach based on absolutely positioned <span> elements by default causes flickering with text selection, and we have browser-specific workarounds to solve that.

In Chrome, the workaround involves moving the .endOfContent element to right after the last element that contains some selected content. This works well in simple PDFs, but breaks when we have span.markedContent elements. Given a text layer structure like the following, rendered as four consecutive lines:

<span class="markedContent">
  <br>
  <span>development enter the construction phase (estimated at around</span>
</span>
<span class="markedContent">
  <br>
  <span>300 MEUR).</span>
</span>
<span class="markedContent">
  <br>
  <span>Kreate's EBITA increased to 2.8 MEUR (Q4'23: 2.7 MEUR) and the</span>
</span>
<span class="markedContent">
  <br>
  <span>margin rose to 3.7% (Q4'23: 3.4%). However, profitability was</span>
</span>

when starting to select from inside the first line and dragging down to the empty space after the second line, Chrome will anchor the selection at the beginning of either the <br> or the <span> inside the last .markedContent, depending on whether the selection is in "per-character mode" (i.e. click and drag) or "per-word mode" (i.e. double click and drag). This causes us to insert the .endOfContent element in the wrong place (one element too far), which causes one more line to be selected, which triggers another "selecctionchange" event, which causes us to move .endOfContent again, and so on, looping until when the whole page is selected.

This commit fixes the issue by making sure that when the end of the selection range points to the begining of an element, we walk back the dom finding the first non-empty element, and attatch .endOfContent to the end of that.

Note that this PR only touches Chrome-specific code. It's stripped away in MOZCENTRAL builds, and it is behind feature detection in GENERIC builds. Firefox already passes the new tests.

The test PDF is the second page of https://www.inderes.fi/files/42dd9cb4-1021-4f9a-ae29-cfb10ee7529c. This is the video of the current broken behavior:

Screen.Recording.2025-04-07.at.16.37.04.mov

The current text layer approach based on absolutely positioned
`<span>` elements by default causes flickering with text selection,
and we have browser-specific workarounds to solve that.

In Chrome, the workaround involves moving the `.endOfContent` element to
right after the last element that contains some selected content. This
works well in simple PDFs, but breaks when we have `span.markedContent`
elements. Given a text layer structure like the following, rendered
as four consecutive lines:
```html
<span class="markedContent">
  <br>
  <span>development enter the construction phase (estimated at around</span>
</span>
<span class="markedContent">
  <br>
  <span>300 MEUR).</span>
</span>
<span class="markedContent">
  <br>
  <span>Kreate's EBITA increased to 2.8 MEUR (Q4'23: 2.7 MEUR) and the</span>
</span>
<span class="markedContent">
  <br>
  <span>margin rose to 3.7% (Q4'23: 3.4%). However, profitability was</span>
</span>
```
when starting to select from inside the first line and dragging down
to the empty space after the second line, Chrome will anchor the
selection at the beginning of either the `<br>` or the `<span>` inside
the last `.markedContent`, depending on whether the selection is in
"per-character mode" (i.e. click and drag) or "per-word mode" (i.e.
double click and drag). This causes us to insert the `.endOfContent`
element in the wrong place (one element too far), which causes one
more line to be selected, which triggers another `"selecctionchange"`
event, which causes us to move `.endOfContent` again, and so on, looping
until when the whole page is selected.

This commit fixes the issue by making sure that when the end of the
selection range points to the _begining_ of an element, we walk back
the dom finding the first non-empty element, and attatch `.endOfContent`
to the end of that.
@nicolo-ribaudo
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @nicolo-ribaudo received. Current queue size: 0

Live output at: http://54.193.163.58:8877/04bc98056e78896/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @nicolo-ribaudo received. Current queue size: 0

Live output at: http://54.241.84.105:8877/6fb4259009665dd/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/6fb4259009665dd/output.txt

Total script time: 12.39 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/04bc98056e78896/output.txt

Total script time: 31.09 mins

  • Integration Tests: FAILED

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, thank you.

@Snuffleupagus Snuffleupagus merged commit 85e6f3c into mozilla:master Apr 8, 2025
7 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the chrome-selection-fix branch April 8, 2025 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants