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.

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

Fix triple-click selection on chromium #753

Merged
merged 3 commits into from
Oct 25, 2021
Merged

Fix triple-click selection on chromium #753

merged 3 commits into from
Oct 25, 2021

Conversation

acywatson
Copy link
Contributor

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2021
const selection = view.getSelection();
if (selection !== null) {
selection.focus.key = selection.anchor.key;
selection.focus.offset = selection.anchor.getNode().getTextContentSize();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't selection.anchor.getNode() be a block node too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also you should never really mutate the focus or anchor points like this, instead prefer using the set method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also you should never really mutate the focus or anchor points like this, instead prefer using the set method.

I was curious about this - decided to just wait for your comments in the PR instead of asking. Good to know.

@trueadm
Copy link
Collaborator

trueadm commented Oct 22, 2021

I'm curious what bug the triple click brings? It seems to occur in other editors too. Why does patching it matter? I guess there are two things:

  • if we are trying to patch Outline's internal model so that it doesn't select both blocks, this change should maybe live in OutlineSelection, like I mentioned on work chat.
  • if you want to update Outline and also physically move the browser native selection into the previous node, then it shouldn't live in OutlineSelection, as that trigger a reconcileSelection.

@acywatson
Copy link
Contributor Author

I'm curious what bug the triple click brings? It seems to occur in other editors too. Why does patching it matter? I guess there are two things:

  • if we are trying to patch Outline's internal model so that it doesn't select both blocks, this change should maybe live in OutlineSelection, like I mentioned on work chat.
  • if you want to update Outline and also physically move the browser native selection into the previous node, then it shouldn't live in OutlineSelection, as that trigger a reconcileSelection.

The bug occurs in a pretty basic use case. if I type two lines of text (two paragraphs) then I triple click to select the entire first one and try to format it to Bold or something, that style gets applied to the entire second paragraph as well. I don't think this can be considered "sane" behavior, from a UX standpoint, even if other editors are doing it that way.

The Gmail composer doesn't have this behavior, neither does Quip, just from a cursory glance.

Something else we can consider is whether it makes sense to just patch it in the BizComposer. Maybe it doesn't need to be in Outline at all.

@trueadm
Copy link
Collaborator

trueadm commented Oct 22, 2021

Aha, Triple click here isn’t the bug. Rather there’s a bug with the format logic.

e. if I type two lines of text (two paragraphs) then I triple click to select the entire first one and try to format it to Bold or something, that style gets applied to the entire second paragraph as well.

if you’re selecting a text node at offset 0, we shouldn’t be applying any formatting or styling to that node, it should be skipped.

Copy link
Collaborator

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Awesome stuff! :)

Comment on lines 495 to 498
// if the entire last node isn't selected, split it
if (endOffset !== lastNodeTextLength) {
[lastNode] = lastNode.splitText(endOffset);
}
Copy link
Member

@zurfyx zurfyx Oct 25, 2021

Choose a reason for hiding this comment

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

Should we fix this here? AFAIK the problem is with the split text logic. Usually you want to handle index case 0, because otherwise instead of the expected [before, after] you get [after, undefined]. Which sounds like the logic you've introduced after to address this

Suggested change
// if the entire last node isn't selected, split it
if (endOffset !== lastNodeTextLength) {
[lastNode] = lastNode.splitText(endOffset);
}
// if the entire last node isn't selected, split it
if (endOffset !== 0) {
if (endOffset !== lastNodeTextLength) {
[lastNode] = lastNode.splitText(endOffset);
}
lastNode.setFormat(lastNextFormat);
}

@@ -357,8 +359,19 @@ export function getSelectionStyleValueForProperty(
): string {
let styleValue = null;
const nodes = selection.getNodes();
const anchor = selection.anchor;
const focus = selection.focus;
const isBefore = anchor.isBefore(focus);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isBefore = anchor.isBefore(focus);
const isBefore = selection.isBackward();

@acywatson acywatson merged commit 84e99aa into main Oct 25, 2021
@acywatson acywatson deleted the fix-triple-click branch October 25, 2021 14:36
acywatson added a commit that referenced this pull request Apr 9, 2022
* fix triple-click issue in formatting helper

* ignore triple-click e2e test in plain-text environment.

* implement suggested changes.

Co-authored-by: Acy Watson <acy@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants