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

cursor wrapper in Firefox #688

Closed
eshvedai opened this issue Aug 11, 2017 · 15 comments

Comments

@eshvedai
Copy link

commented Aug 11, 2017

I'm trying to achieve the following behavior with inline code:

inline-code

The idea is to be able to put cursor inside inline code and outside just by moving the cursor. This is working pretty neat in Chrome.

This is how it works in Firefox:
firefox-inline-code

Jumping empty span is the cursor wrapper. It gets created by toggling inline code mark on arrowLeft/arrowRight at the edge of the mark. The problem that I have is that sometime the cursor is stuck just before the "\ufeff" - I'm not able to move the cursor to the left, only to the right. It also requires quite a bit of left-right-left-right movement to get to the "stuck" state.

My debugging led me to believe that the reason is hidden in skipIgnoredNodesLeft function that has the following hack:

 if (node.nodeType == 3 && node.nodeValue.charAt(offset - 1) == "\ufeff") {
          moveNode = node
          moveOffset = --offset
 } else break

The funniest part is that as soon as I add console.log inside the above if statement, the problem is gone. I assume there's a timing issue (setTimeout-related) and console.log takes browser some time to fire.

I would appreciate any hits regarding the possible fix and would be happy to provide any details of implementation if needed.

@eshvedai eshvedai changed the title cursor wrapper in Firefow cursor wrapper in Firefox Aug 11, 2017

@eshvedai

This comment has been minimized.

Copy link
Author

commented Aug 12, 2017

fixed the problem locally by removing the "\ufeff" on arrowLeft and Backspace:

  if (moveNode) {
    moveNode.parentNode.removeChild(moveNode);
  }
@marijnh

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

The funniest part is that as soon as I add console.log inside the above if statement, the problem is gone. I assume there's a timing issue (setTimeout-related) and console.log takes browser some time to fire.

Nice. Does the DOM or DOM selection that the skipping code acts on look different in the successful and unsuccessful case? (Maybe put innerHTML in a global variable and inspect it afterwards to avoid the timing issue.)

@eshvedai

This comment has been minimized.

Copy link
Author

commented Aug 25, 2017

sorry, forgot to answer. The behavior was strange: the cursor was stack, you press arrowLeft 10 times or so and it starts moving again. The DOM seemed to be the same despite the presence of console.log.

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 9, 2018

@eshvedai how did you managed to fix this? I'm having exactly the same issue and tracked it down to the same place but can't seem to nudge it into working!

@eshvedai

This comment has been minimized.

Copy link
Author

commented Mar 9, 2018

I refactored a bit skipIgnoredNodesLeft (https://github.com/ProseMirror/prosemirror-view/blob/0353426e46eebbdc01933263bd3cc8df72c5b507/src/capturekeys.js#L100)
plus added those lines at the bottom of the function

  if (moveNode) {
    moveNode.parentNode.removeChild(moveNode);
  }
};

and I fire it on Backspace:

    const { $cursor } = state.selection as TextSelection;
    if ($cursor && $cursor.nodeBefore) {
      skipIgnoredNodesLeft(view);
    }
@marijnh

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

So, apologies about letting this sit for so long. Is there some easy way to reproduce this? I tried adding a plugin that always sets stored marks when the cursor is on the boundary between code and non-code text, to force a cursor wrapper, but after moving the cursor back and forth for five minutes I still didn't see anything suspicious.

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 10, 2018

I’ll try and distill down the issue I’m having into a demo over the next couple of days.

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 12, 2018

@marijnh I managed to create a breaking example here. Cursoring left into the strong mark (from after it) will, quite often, end up maintaining the selection at the end of the mark. Specifically I can only reproduce this when inclusive is set to false for the mark. It also seems like a race condition as it doesn't always fail (you might need to spend 10-20 seconds or so going back and forth over the end of the mark). This is tested in both Firefox 58.0.2 and 60.0b2. I've had a couple of fixes that I thought worked here (inside the skipIgnoredNodesLeft function) that revolved around finding the highest parent that shared the same end position as the text node and setting the selection on that. It seemed to work in some cases but break in others (although I'll admit to not being 100% sure I follow every branch of that function, despite knowing pretty much what it does). Happy to have a longer look at this when I get some time but any insight would be appreciated!

@marijnh

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

I was able to reproduce it now—though only about one in ~50 tries. I managed to log the DOM selection and DOM at the point where the keydown happens, both for the normal case and for the case where the cursor doesn't move, but there don't appear to be any differences between them.

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 13, 2018

@marijnh I can't remember whether it was in the instance but I found that the DOMSelection was sometimes mutating in FF and that while it was being set the same, it was actually different if I logged the reference to the selection and toggled that open in the console. I think it was when testing that, although you may have already looked into that.

marijnh added a commit to ProseMirror/prosemirror-view that referenced this issue Mar 13, 2018

Fix race condition in pre-keypress selection update
FIX: Fixes an issue where, on Firefox, depending on a race condition,
the skipping over insignificant DOM nodes done at keypress was
canceled again before the keypress took effect.

Issue ProseMirror/prosemirror#688
@marijnh

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

I think I found it. Sometimes the selectionchange event would fire before the effect of the key press was processed, which caused ProseMirror's handler to reset the DOM selection and undo the work of skipIgnoredNodesLeft/Right. Attached patch should address this. (I'll release it soon, but I want to look at some other stuff first.)

Could you verify that it helps for you too?

(Also I think I just wore out my arrow keys.)

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 13, 2018

@marijnh I've given it the cursoring of a lifetime and can't get it to break - good catch 🎉 ! Is this why I was seeing some mutation because it was getting rewritten elsewhere?

@marijnh

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

Yes, SelectionReader.readFromDOM was the culprit. My change makes it ignore the (intentional) update made by the key handling code.

@marijnh marijnh closed this Mar 13, 2018

@RichieAHB

This comment has been minimized.

Copy link

commented Mar 13, 2018

Actually, it seems to work in all cases except the original issue from @eshvedai in Firefox (with pseudo elements) and inclusive: false where it now doesn't work at all.

https://codesandbox.io/s/rwp1j58npp

marijnh added a commit to ProseMirror/prosemirror-view that referenced this issue Mar 14, 2018

Fix selection reset causing a stuck cursor on a pseudo-element + wrapper
FIX: Fixes an issue where an `:after` pseudo-element on a
non-inclusive mark could block the cursor, making it impossible to
arrow past it.

Issue ProseMirror/prosemirror#688
@marijnh

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

That was somewhat similar problem. Patch 23c1416 should take care of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.