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

Comments

@haugstrup
Copy link
Contributor

commented Jan 11, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

Did you report it to the browser vendors?

@haugstrup

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

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

@haugstrup

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2018

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

@haugstrup

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2018

"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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

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.

jhchen added a commit that referenced this issue Jan 18, 2018
@jhchen jhchen closed this Jan 18, 2018
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.