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

[SPIKE] Add delete to start of line for MacOS #636

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevinansfield
Copy link
Collaborator

@kevinansfield kevinansfield commented Jul 31, 2018

refs #445

  • add Position.atStartOfLine method that takes an existing position from which to find the start of it's line
  • add line unit for use in editor.performDelete
  • update delete key event handler to pass line unit when Meta+Backspace is pressed

I had a quick stab at re-implementing the default MacOS Cmd+Backspace behaviour but it's highlighted a problem. macOS allows you to place the caret visually at the end of the line when it's actual position is at the start of the next line (either through clicking at the end of the line or using Cmd+Right). You can see a demonstration here:

delete-line-buggy

When using both mobiledoc-kit's internal position and the browser-provided document.getSelection().getRangeAt(0).getBoundingClientRect() the only position exposed to JS is the beginning of the next line. The reason this is a problem is because to faithfully replicate macOS' default behaviour the delete needs to happen from the visual caret position rather than the logical caret position.

I disabled the delete key event handling in event-manager.js to demonstrate the native macOS behaviour where you can see the delete happening from the visual rather than logical position:

delete-line-native

refs bustle#445
- add `Position.atStartOfLine` method that takes an existing position from which to find the start of it's line
- add `line` unit for use in `editor.performDelete`
- update delete key event handler to pass `line` unit when <kbd>Meta+Backspace</kbd> is pressed
@kevinansfield
Copy link
Collaborator Author

kevinansfield commented Aug 1, 2018

Unfortunately removing the delete key handler causes problems as soon as cards are involved - Cmd+Backspace can remove part of the card's DOM leaving the editor in a broken state - so it looks like a solution similar to this PR is needed at least in the short term.

Looking at other editor implementations I saw the following behaviour:

  • Dropbox Paper: as per this PR
  • Google Docs: works like native (they have their own canvas which re-implements everything related to cursor positioning so that’s expected)
  • Medium: deletes to the beginning of the paragraph 🤨
  • Quill: works like native (built around mutation observers and no complex cards to deal with so that’s expected)
  • TinyMCE: works like native but undo breaks

Copy link
Contributor

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

I think this is an excellent direction to take this @kevinansfield!

@@ -101,6 +135,24 @@ Position = class Position {
return Position.fromNode(_renderTree, node, offset);
}

static atStartOfLine(position, editor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this method should be able to become an instance method or getter.

editor being passed to atPoint is perhaps in error. It seems like the only thing read off from the editor is renderTree and element, and the root element can be read off the renderTree just as well. Several other methods are coupled to the renderTree so we should likely stop from using editor if we can get by with just the render tree and references to the AST (section).

@@ -62,6 +62,40 @@ function findOffsetInSection(section, node, offset) {
}
}

// TODO: expose as utility function, update cursor._findNodeForPosition
function findNodeForPosition(position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have been copied from cursor.js, the code should probably be de-duplicated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants