Skip to content

Disable link annotations during text selection#18481

Merged
timvandermeij merged 1 commit into
mozilla:masterfrom
nicolo-ribaudo:fix-selection-with-links
Jul 28, 2024
Merged

Disable link annotations during text selection#18481
timvandermeij merged 1 commit into
mozilla:masterfrom
nicolo-ribaudo:fix-selection-with-links

Conversation

@nicolo-ribaudo

@nicolo-ribaudo nicolo-ribaudo commented Jul 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Commit message:

Disable link annotations during text selection

When selecting text, hovering over an element causes all the text between (according the the dom order) the current selection and that element to be selected.

This means that when, while selecting, the cursor moves over a link, all the text in the page gets selected. Setting user-select: none on the link annotations would improve the situation, but it still makes it impossible to extend the selection within a link without using Shift+arrows keys on the keyboard.

This commit fixes the problem by setting pointer-events: none on the <section>s in the annotation layer while selecting some text. This way, they are ignored for hit-testing and do not affect selection.

It is still impossible to start a selection inside a link, as the link text is covered by the link annotation.

Fixes #18266

A lot of changes are just indentation: the selectionchange event handler has a single changed line, and the pre-existing tests none. I recommend reviewing with whitespace diff disabled.

Snuffleupagus
Snuffleupagus reviewed Jul 22, 2024
View reviewed changes
Comment thread web/text_layer_builder.js Outdated
@nicolo-ribaudo nicolo-ribaudo force-pushed the fix-selection-with-links branch from 0faa2c1 to 2b8cdce Compare July 22, 2024 14:50
@nicolo-ribaudo

nicolo-ribaudo commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator Author

@timvandermeij Could you also label this as text-selection? :)

timvandermeij
timvandermeij reviewed Jul 22, 2024
View reviewed changes

@timvandermeij timvandermeij left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll trigger a run of the tests, but these are my comments based on a first look at the diff.

Comment thread test/integration/text_layer_spec.mjs Outdated
Comment thread web/text_layer_builder.js Outdated
@timvandermeij

timvandermeij commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator

/botio integrationtest

@moz-tools-bot

moz-tools-bot commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/95a65e9243e5107/output.txt

@moz-tools-bot

moz-tools-bot commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/2b461cbf77de893/output.txt

@moz-tools-bot

moz-tools-bot commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/95a65e9243e5107/output.txt

Total script time: 8.65 mins

  • Integration Tests: Passed

@moz-tools-bot

moz-tools-bot commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/2b461cbf77de893/output.txt

Total script time: 18.87 mins

  • Integration Tests: Passed

@nicolo-ribaudo nicolo-ribaudo force-pushed the fix-selection-with-links branch 2 times, most recently from 6222dea to d2ac0f7 Compare July 22, 2024 16:44
Snuffleupagus
Snuffleupagus reviewed Jul 23, 2024
View reviewed changes
Comment thread test/integration/text_layer_spec.mjs
Comment thread test/integration/text_layer_spec.mjs Outdated
@nicolo-ribaudo
Disable link annotations during text selection
64a0e59 
When selecting text, hovering over an element
causes all the text between (according the the dom
order) the current selection and that element to
be selected.

This means that when, while selecting, the cursor
moves over a link, all the text in the page gets
selected. Setting `user-select: none` on the link
annotations would improve the situation, but it
still makes it impossible to extend the selection
within a link without using Shift+arrows keys on
the keyboard.

This commit fixes the problem by setting
`pointer-events: none` on the `<section>`s in the
annotation layer while selecting some text. This
way, they are ignored for hit-testing and do not
affect selection.

It is still impossible to _start_ a selection
inside a link, as the link text is covered by the
link annotation.

Fixes mozilla#18266
@nicolo-ribaudo nicolo-ribaudo force-pushed the fix-selection-with-links branch from d2ac0f7 to 64a0e59 Compare July 23, 2024 08:42
@timvandermeij

timvandermeij commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

/botio-linux preview

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/ce1a97100ab0be4/output.txt

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/ce1a97100ab0be4/output.txt

Total script time: 1.03 mins

Published

@timvandermeij

timvandermeij commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

/botio integrationtest

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/a53bd2105eb47b2/output.txt

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/0f67645c2a5e003/output.txt

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/0f67645c2a5e003/output.txt

Total script time: 8.70 mins

  • Integration Tests: Passed

@moz-tools-bot

moz-tools-bot commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/a53bd2105eb47b2/output.txt

Total script time: 18.88 mins

  • Integration Tests: Passed

@timvandermeij timvandermeij merged commit 6f9fc70 into mozilla:master Jul 28, 2024
@timvandermeij

timvandermeij commented Jul 28, 2024

Copy link
Copy Markdown
Collaborator

Thank you for improving this!

@nicolo-ribaudo nicolo-ribaudo deleted the fix-selection-with-links branch July 28, 2024 13:41
@Snuffleupagus Snuffleupagus mentioned this pull request Aug 23, 2024
Closed
@YuvrajKaushal YuvrajKaushal mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus left review comments
@timvandermeij timvandermeij timvandermeij approved these changes

Assignees

No one assigned

Labels

annotations text-selection viewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link text could not be selected

4 participants

@nicolo-ribaudo @timvandermeij @moz-tools-bot @Snuffleupagus