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

L.DomUtil.disableTextSelection should only disable text selection on the map container #9259

Open
4 tasks done
ventralnet opened this issue Feb 14, 2024 · 4 comments · May be fixed by #9271
Open
4 tasks done

L.DomUtil.disableTextSelection should only disable text selection on the map container #9259

ventralnet opened this issue Feb 14, 2024 · 4 comments · May be fixed by #9271
Labels
bug feature good first issue Good for newcomers

Comments

@ventralnet
Copy link

ventralnet commented Feb 14, 2024 •
edited

Checklist

  • I've looked at the documentation to make sure the behavior isn't documented and expected.
  • I'm sure this is an issue with Leaflet, not with my app or other dependencies (Angular, Cordova, React, etc.).
  • I've searched through the current issues to make sure this hasn't been reported yet.
  • I agree to follow the Code of Conduct that this project adheres to.

Steps to reproduce

I have a certain situation in an application where we have a content editable div as a search bar. The search bar is special in that we highlight special search terms as the user types. To keep track of cursor positions as the user types we deal with text selection of the document.

When you enable a draw tool and the hooks are added there is a line that L.DomUtil.disableTextSelection. This prevents the special content editable div from receiving text select start events.

I think this is a bug. I think text selection should only be disabled on the map itself and not the entire DOM document.

Expected behavior

Text select is disabled on the map

Current behavior

text selection is disabled on the entire window

Minimal example reproducing the issue

No response

Environment

  • Leaflet version:
  • Browser (with version):
  • OS/Platform (with version):
@ventralnet ventralnet added bug needs triage Triage pending labels Feb 14, 2024
@IvanSanchez IvanSanchez added feature good first issue Good for newcomers and removed needs triage Triage pending labels Feb 14, 2024
@IvanSanchez
Copy link
Member

This is certainly doable.

The disableTextSelection/enableTextSelection functions in DomUtil should receive a HTMLElement as a parameter, and disable/enable text selection in that element only. If the parameter is undefined, then fall back to the document (this should prevent plugins from misbehaving).

Then, send the map's container as a parameter on calls to disable/enableTextSelection

@ventralnet
Copy link
Author

ventralnet commented Feb 14, 2024 •
edited

@IvanSanchez I agree. I was surprised looking through the docs it did not. I could probably submit a pull request sometime this week when I have time.

@mourner
Copy link
Member

mourner commented Feb 14, 2024

I think it was designed this way because you're not meant to disable it permanently, only for the duration of a particular drag action. This is what Leaflet does, disabling it on the whole document because dragging cursor can go outside of the map container while continuing to affect the map, and we don't want text to be selected at the time. This seems like a bug in the draw tool you're using rather than a Leaflet issue — correct me if I'm wrong.

@ventralnet
Copy link
Author

@mourner This makes sense. I was conflicted if it were a leaflet bug or a Leaflet.draw bug actually. My work around is I cancel all leaflet draw tools when the leaflet map is blurred which in turn calls L.DomUtil.enableTextSelection

ventralnet added a commit to ventralnet/Leaflet that referenced this issue Feb 18, 2024
@ventralnet
Leaflet#9259 Support to pass a target element to enableTextSelection …
3287ba5 
…and disableTextSelection
ventralnet added a commit to ventralnet/Leaflet that referenced this issue Feb 20, 2024
@ventralnet
Leaflet#9259 Updated leaflet-doc for disableTextSelection
7d4460d
ventralnet added a commit to ventralnet/Leaflet that referenced this issue Feb 20, 2024
@ventralnet
Leaflet#9259 Moved tests to a when arguments provided describe
2a5ea8c
ventralnet added a commit to ventralnet/Leaflet that referenced this issue Feb 21, 2024
@ventralnet
Leaflet#9259 spec updated for new functionality
00b0548
@ventralnet ventralnet linked a pull request Feb 21, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug feature good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

9259 disable text selection ventralnet/Leaflet
3 participants
@mourner @ventralnet @IvanSanchez