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
| cooked.classList.add("post-text-selection-active"); | ||
| this.disableTextSelection = true; |
There was a problem hiding this comment.
These lines do:
- disable text selection for the whole page
- force enable current text post selection
| } | ||
| } | ||
|
|
||
| html.mobile-device { |
There was a problem hiding this comment.
This doesn't seem related to viewport-width, so I don't think we should be using .mobile-device here?
There was a problem hiding this comment.
how do I target android?
There was a problem hiding this comment.
Could we only apply the -has-post-text-selection class for affected devices?
| &: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
| if (touch) { | ||
| if (isAndroid) { | ||
| // fires when user finishes adjusting text seleciton on android | ||
| // ios has no such event |
There was a problem hiding this comment.
Could we just add the event listeners anyway? Or do they cause issues on other devices?
(Reducing code differences between devices makes things a lot easier to test/debug IMO)
| @@ -737,8 +737,32 @@ aside.quote { | |||
| opacity: 0.4; | |||
| } | |||
|
|
|||
| .fk-d-menu[data-identifier="post-text-selection-toolbar"] { | |||
| // disabling user select on the whole page while selecting a post text | |||
| body.-disable-global-text-selection * { | |||
There was a problem hiding this comment.
This is a very expensive selector... shows up as the 4th-most-expensive in the whole of Discourse in chrome dev tools. (browsers evaluate selectors right-to-left, so this is a big cost even if -disable-global-text-selection is not set)
Not sure there's anything we can do about it... but wanted to note it
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.