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

Browser crashes with specific cursor movement around images #1910

Closed
haugstrup opened this issue Jan 11, 2018 · 10 comments
Closed

Browser crashes with specific cursor movement around images #1910

haugstrup opened this issue Jan 11, 2018 · 10 comments

Comments

@haugstrup
Copy link
Contributor

Given specific editor contents and specific cursor position Chrome and Safari will reliably crash if you hit option+left arrow.

Steps for Reproduction

  1. https://quilljs.com/playground/
  2. Insert a leading space
  3. Insert an image
  4. Insert a line break (just hit enter)
  5. Insert an image on the next line
  6. Insert another image (don't include a space between the two images)
  7. Place the cursor at the end of the first line
  8. Press option+left arrow

Expected behavior:
Cursor moves to the other side of the image

Actual behavior:
Browser crashes and browser tab has to be force quit

Platforms:

Mac. Chrome and Safari.

Version:

1.3.4

Illustration:

interactive_playground_-_quill_rich_text_editor

@benbro
Copy link
Contributor

benbro commented Jan 11, 2018

Did you report it to the browser vendors?

@haugstrup
Copy link
Contributor Author

I did not. This is not a problem with contenteditable in general, it's a problem with Quill in particular.

@haugstrup
Copy link
Contributor Author

haugstrup commented Jan 11, 2018

There are more scenarios that will cause the browser crash. The example above is the smallest one I could find. If there is any non-whitespace text after the images everything works fine.

@benbro
Copy link
Contributor

benbro commented Jan 11, 2018

A browser crash is always a problem with the browser. A web page shouldn't be able to crash a browser.
What do you mean by 'browser crash'? What exactly happens?

@jhchen
Copy link
Member

jhchen commented Jan 12, 2018

I think it is more accurate to say the browser locks up but it looks like it affects vanilla contenteditable:

  1. Visit https://jsfiddle.net/vtqjxm1r/.
  2. Place cursor right after first image
  3. Hit option+arrowleft

The image / base64 image does not appear to be a factor: https://jsfiddle.net/k1ssqq70/1/. It does appear to require all three images and the leading space precisely to reproduce though. It may be possible to detect this shortcut in Quill and preventDefault to circumvent.

@haugstrup
Copy link
Contributor Author

Strange, I could not repro with a vanilla element earlier today. I must've made a mistake. I've added a custom key binding in my Quill usage to catch this before it happens -- seems reasonable to do the same in Quill out of the box.

@jhchen
Copy link
Member

jhchen commented Jan 12, 2018

Yes agreed. Are you just disabling the shortcut or re-implementing the effects of option+left arrow?

@haugstrup
Copy link
Contributor Author

"Re-implemented" in the sense that I'm moving the cursor to the left of the image. I'm not 100% sure that's the correct behavior for option+left, but it seems better than doing nothing

@jhchen
Copy link
Member

jhchen commented Jan 12, 2018

Great thanks -- would you be willing to share the code for the keyboard shortcut handler? Also you mentioned there were other cases but this was the simplest. Did they all involve three images like this? It seems like a strange condition I wonder if the later images happen to always fulfill some other general case.

@haugstrup
Copy link
Contributor Author

altLeft: {
	key: 37,
	altKey: true,
	metaKey: false,
	shiftKey: false,
	handler: () => {
		const range = this.quill.getSelection();
		if (range && range.index > 0) {
			const delta = this.quill.getContents(range.index - 1, 1);

			// "emoji" is a custom embed format -- replace with "image" I think?

			if (delta && delta.ops.length && delta.ops[0].insert.emoji) {
				// Manually move the cursor to the other side of the emoji
				this.quill.setSelection(range.index - 1, 0);
				return false;
			}
		}
	},
},

There's a wide variety of combinations that will cause this problem:

  • It'll still happen if you add a line of all images (with any whitespace) above
  • It'll still happen if you add more images (without whitespace) before the leading whitespace
  • It'll still happen if you add more images to the last line
  • It'll still happen if you add typed text after the last line (but any text before the leading whitespace and you're fine)

Those were the cases I want through in my testing.

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