Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disable link annotations during text selection #18481

Merged
merged 1 commit into from
Jul 28, 2024
Merged

Disable link annotations during text selection #18481

merged 1 commit into from
Jul 28, 2024

Conversation

nicolo-ribaudo
Copy link
Contributor

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

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.

@nicolo-ribaudo
Copy link
Contributor Author

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

timvandermeij
timvandermeij reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@timvandermeij timvandermeij left a comment

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.

test/integration/text_layer_spec.mjs Outdated Show resolved Hide resolved
web/text_layer_builder.js Outdated Show resolved Hide resolved
@timvandermeij
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
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
Copy link
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
Copy link
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
Copy link
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
@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
@timvandermeij
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
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
Copy link
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
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
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
Copy link
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
Copy link
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
Copy link
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
7 checks passed
@timvandermeij
Copy link
Contributor

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