FIX: improves text selection of posts #33143
Conversation
| // ensure we selected content inside 1 post *only* | ||
| let postId; | ||
| for (let r = 0; r < selection.rangeCount; r++) { | ||
| const range = selection.getRangeAt(r); | ||
| const selectionStart = getElement(range.startContainer); | ||
| const ancestor = getElement(range.commonAncestorContainer); | ||
|
|
||
| if (!selectionStart.closest(".cooked")) { | ||
| return await this.hideToolbar(); | ||
| } | ||
|
|
||
| postId ||= ancestor.closest(".boxed, .reply")?.dataset?.postId; | ||
|
|
||
| if (!ancestor.closest(".contents") || !postId) { | ||
| return await this.hideToolbar(); | ||
| } |
There was a problem hiding this comment.
we now receive the cooked from the onSelectionChanged event
| const _selectedElement = getElement(selectedNode()); | ||
| const cooked = | ||
| _selectedElement.querySelector(".cooked") || | ||
| _selectedElement.closest(".cooked"); | ||
|
|
There was a problem hiding this comment.
cooked is received from onSelectionChange event
| @@ -220,7 +232,8 @@ export default class PostTextSelection extends Component { | |||
| // so we need more space | |||
| // - the end of the selection is not in viewport, in this case our menu will be shown at the top | |||
| // of the screen, so we need more space to avoid overlapping with the native menu | |||
| offset = 70; | |||
| const { isAndroid } = this.capabilities; | |||
| offset = isAndroid ? 90 : 70; | |||
There was a problem hiding this comment.
THe menu needs more spacing on android
There was a problem hiding this comment.
Should we just increase it everywhere for consistency?
There was a problem hiding this comment.
It's just the native toolbar on android is bigger, but we can try the same if you think that's better
app/assets/javascripts/discourse/app/components/post-text-selection.gjs
Outdated
Show resolved
Hide resolved
| &:not(.ios-device) body.-disable-global-text-selection { | ||
| // prevents hovering toolbar menu to confuse text selection on android |
There was a problem hiding this comment.
Can we just do the same thing on iOS for consistency? Or does it cause issues there?
There was a problem hiding this comment.
it does nothing on iOS I think so yes I can probably just add it too
app/assets/javascripts/discourse/app/components/post-text-selection.gjs
Outdated
Show resolved
Hide resolved
This commit is applying different techniques to make selecting text of a post less error prone: - disables text selection of all the page BUT the current post - ensures the selection menu is not interfering with text selection - ensures the d-header is not interfering with text selection The situation was very bad on android but it should also improve other situations.
fe2750f to
a929410
Compare
| // ensure we never quote text in the post menu area | ||
| const postMenuArea = ancestor.querySelector(".post-menu-area"); | ||
| if (postMenuArea) { | ||
| range.setEndBefore(postMenuArea); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should decide this outside of this helper, setEndBefore is messing with browser behavior and can end up creating weird behaviors where the text selection is changing after cursor release, which is a very janky feeling.
| const parent = | ||
| range.commonAncestorContainer.nodeType === Node.TEXT_NODE | ||
| ? range.commonAncestorContainer.parentNode | ||
| : range.commonAncestorContainer; | ||
|
|
||
| const cooked = parent.closest(".cooked"); |
There was a problem hiding this comment.
This is mostly my solution to ensure we don't show the toolbar when selecting outside of one post, if the common ancestor to a range selection is not a cooked, it means we are selecting outside of a post, or two posts, or a post and something else...
This is needed for discourse/discourse#33143 as we now rely on this pointerup event.
|
|
||
| const { isIOS, isWinphone, isAndroid } = this.capabilities; | ||
| const wait = isIOS || isWinphone || isAndroid ? INPUT_DELAY : 25; | ||
| this.selectionChangeHandler = discourseDebounce( |
There was a problem hiding this comment.
now that we rely on pointerup and contextmenu and not selectionchange, we don't need debouncing anymore.
This reverts commit 51c9bb4.
| } | ||
|
|
||
| // most importantly for android to prevent the quote menu to interfere with the text selection | ||
| [data-identifier="post-text-selection-toolbar"].-disable-pointer-events, |
There was a problem hiding this comment.
Would it be easy to make a class for this element? Selecting by a data attribute feels a little weird
|
|
||
| // most importantly for android to prevent the quote menu to interfere with the text selection | ||
| [data-identifier="post-text-selection-toolbar"].-disable-pointer-events, | ||
| [data-identifier="post-text-selection-toolbar"].-disable-pointer-events * { |
There was a problem hiding this comment.
Does it work if we do this?
| [data-identifier="post-text-selection-toolbar"].-disable-pointer-events * { | |
| [data-identifier="post-text-selection-toolbar"].-disable-pointer-events { |
If I'm understanding MDN correctly, pointer-events:none applies to children as long as they don't explicitly set pointer-events: auto 👀
| @@ -323,6 +323,16 @@ blockquote { | |||
| margin-right: 0; | |||
| } | |||
|
|
|||
| .d-header-wrap * { | |||
There was a problem hiding this comment.
Does it work without *? MDN page on user-select says:
the initial auto value makes it behave like it is inherited most of the time
This is needed for discourse/discourse#33143 as we now rely on this pointerup event.
This is needed for discourse/discourse#33143 Ideally we should move all these helpers to core, or improve the ones in core.
This is needed for discourse/discourse#33143 Ideally we should move all these helpers to core, or improve the ones in core.
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/i-can-only-quote-one-word/369911/1 |
This commit is applying different techniques to make selecting text of a post less error prone:
The situation was very bad on android but it should also improve other situations.