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

Incorrect behavior of NodeView demo http://prosemirror.net/examples/codemirror/ #83

Open
Gozala opened this issue Jan 17, 2018 · 3 comments

Comments

@Gozala
Copy link

Gozala commented Jan 17, 2018

Hi,

I was not sure what was the right place to report issues for the the Codemirror NodeView demo, but since source for it is here I've decided to do it here.

Here is the list of things I found to not behave according to (at least my) expectations:

  1. If you place a cursor right after the editor view and hit backspace it moves text under the text editor into the editor itself. I am not entirely sure what would make sense here. I suspect one of the following:

    • Select an editor NodeView as it matches what happens if you had a "Horizontal rule" in the place of editor. If you do press backspace once again delete the whole editor view. I think even though this requires additional press on backspace it does provide user clear signal what's about to happen.
    • Select an editor NodeView when cursor is right in front or after the NodeView. I think this would clearly communicate that delete / backspace will get rid of NodeView without extra keypress.
    • Just move cursor into the NodeView, essentially do what ArrowLeft would have done. I'm not sure if that behavior would be intuitive but I suspect less surprising.
    • Move cursor into the NodeView and delete last character in it. I'm not really in favor of it, but that is certainly an option.

    I think it would also make sense to add Ctrl-Backspace keybinding to perform current behavior (move text after cursor into the editor view) and extend Ctrl-Enter keybinding to somewhat do what it already does but more closely mirror Ctrl-Backspace in other Ctrl modifier would allow you to move text in & out of the editor view.

  2. If you place cursor right before the editor view and hit delete it appends editor content to the previous line removing editor if line of the cursor isn't empty, or it remove empty line the cursor is on and moves cursor into an editor.

    Not going to list all the options here but would expect option to just mirror choice above. I also would expect that behavior to be the same regardless of whether line of cursor is empty or not.

  3. If you delete all the content of the editor and keep pressing backspace nothing happens. In fact only way to remove editor is to delete all the content then place cursor in front of the editor and press delete. Here's options I think would provide more intuitive behavior:

    • If highlighting an NodeView on first backspace / delete is a chosen solution, then there's nothing to do it would actually cover this.
    • If editor content is emptied and backspace is pressed just delete NodeView. Same for delete if editor content is empty it should probably just delete editor node.
  4. If you have text selected before the editor and keep extending selection with Shift-ArrowRight / Shift-ArrowDown keys nothing happens. On the other hand if you have text selected right after the editor and use Shift-ArrowLeft it extends selection with in the editor but only per line basis and Shift-ArrowUp does nothing. Also note that extending selection from with-in the editor to the outside of it does not work at all.

    • I'm not sure that extending selection with in the editor makes much sense, maybe treating as NodeView as atomic would make more sense here.
  5. If you have selection with-in the editor and you click outside of the editor text remains selected. Not saying it's incorrect but I wanted to point this out as well as it would help in the summary below.

Summary

I think my general observation is that there must be two different types of NodeView:

Atomic NodeView (kind of a Portal)

Such NodeView could be treated be treated from the outside as a single characters, except when cursor moves across it (cursor is moved across it or user clicks it) NodeView hook could onEnter(dir) is invoked which NodeView could cancel to take over the ProseMirror's events and perform all the range manipulation internally. It can also possibly exit itself whenever that makes sense (like when cursor is moved passed it's own content or Ctrl-Enter etc...)

all the events are from the ProseMirror are delegated to it. NodeView could also be responsible of triggering exiting itself (moving cursor past the NodeView).

Inline NodeView (just a alternate rendering of text range)

I need to look into more details to write something up, but my intuition is that it might be useful to have a NodeView which does just a rendering of a text range, meaning that all of the actual event handling (cursor positioning, selection, etc...) is taken care of by a ProseMirror while the rendering of the state is delegated to this NodeView.

As I don't know nearly enough about ProseMirror I might be drawing wrong conclusions, but it seems to me there would be a value in some of the cursor handling out to the NodeView interface so that NodeView implementer does not need to handle all of the edge cases for entering / deleting NodeView or care about details of keymap that ProseMirror has being setup with (for example I tend to use ctrl-f / ctr-b for navigation which works as expected in ProseMirror except it won't actually enter the the embedded editor as no mapping was setup).

@marijnh
Copy link
Member

marijnh commented Jan 18, 2018

To do a trick like this in a production-quality way, yes, more scripting will be required than the minimum I did in the example. I do not think this warrants having multiple types of node views. I've set the code_block node to isolating in the example, which solves some of this. The other I feel can be handled by adding more custom key and event handlers.

@WesleyKapow
Copy link

Definitely struggling with number 5 from above. I can't pin down exactly the issue but I believe it has something to do with forwardSelection and setSelection. I've removed forwardSelection and I'm not noticing any drawbacks but I still get issues with setSelection which I definitely need.

@marijnh
Copy link
Member

marijnh commented Sep 1, 2018

This is how CodeMirror works—it'll just style its selection differently when it's unfocused, not actually hide it. You could give it a blur handler that collapses the internal selection to work around it.

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

No branches or pull requests

3 participants