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

Wrap Quill and add value property #2

Closed
tomivirkki opened this issue Sep 24, 2018 · 9 comments · Fixed by #14
Closed

Wrap Quill and add value property #2

tomivirkki opened this issue Sep 24, 2018 · 9 comments · Fixed by #14
Assignees
Milestone

Comments

@tomivirkki
Copy link
Member

tomivirkki commented Sep 24, 2018

Plans here

  • value should represent the Quill's Delta
@tomivirkki tomivirkki added this to the 1.0.0 milestone Sep 24, 2018
@web-padawan web-padawan self-assigned this Oct 2, 2018
@web-padawan
Copy link
Member

There is a pending PR introducing shadow DOM support: slab/quill#1805

I'm in favour of creating our own fork for now, and re-using the work from there.

@web-padawan
Copy link
Member

Submitted a PR rebased on top of 1.3.6 release: slab/quill#2337

Tests are failing in Safari because not implemented getSelection() API.
Potential fix would be using https://github.com/GoogleChromeLabs/shadow-selection-polyfill

@web-padawan
Copy link
Member

web-padawan commented Oct 3, 2018

Short summary

Only Chrome supports getSelection() on the Shadow Root. Neither Firefox 63 beta, nor Safari have it enabled. We need to invest some time in development of our own polyfill to make it work (especially relevant for Safari, as in Firefox 63 global API still works).

Long Story

Original issue is being discussed here: WICG/webcomponents#79
There is no final agreement around it yet, although we might hear some news in the end of October: WICG/webcomponents#763 (comment)

There is an unofficial polyfill by one guy from Google but in only covers part of functionality:
https://github.com/GoogleChromeLabs/shadow-selection-polyfill

Especially, it does not polyfill the whole getSelection() properly, but rather getSelection().getRangeAt(0) which is something we could start with, but it needs to be properly tested, preferably covered by automatic tests.

This issue is not only about the Quill itself, as it will also affect any editor relying on window.getSelection() (or document.getSelection() which is the same), when placed inside of the shadow root.

Solution options

  1. it might be that Google has some kind of working polyfill internally, we should ask (at least from what I understand they are upgrading from Polymer 1 now and should face this issue, too)

  2. implement our own polyfill, e.g. based on https://github.com/timdown/rangy/wiki/Rangy-Selection
    This would mean quite extra work and testing effort. Then we would have to somehow integrate it into the Quill editor, which is a separate task (we might need our own fork for that).

UPD: testing window.getSelection() API in the browsers lacking it on shadow root:

Firefox 63 beta

https://bugzilla.mozilla.org/show_bug.cgi?id=1430308

  • getSelection() not implemented on shadow root
  • window.getSelection() returns nodes from shadow trees (leaking as if they were not in shadow)
  • window.getSelection().addRange() applies the selection when passing range with start and end nodes from the inside of shadow tree
  • window.getSelection().removeAllRanges() works

Summary: Firefox 63 beta could be using global API for now but this might break in future

Safari 11

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/dom/DocumentOrShadowRoot.idl#L30

  • getSelection() is not implemented on shadow root
  • window.getSelection() does not leak nodes from shadow trees (reporting node as body)
  • window.getSelection().addRange() works when passing range with start and end nodes from shadow tree (so using global API seems to work here)
  • window.getSelection().removeAllRanges() works

Other Editors

Related issues in the other WYSIWYG editors:

@platosha
Copy link

platosha commented Oct 3, 2018

Could “Make Quill work on light DOM” be an option?

@web-padawan
Copy link
Member

@platosha the users would be still placing Quill inside of the some shadow root (either view or app-shell components), and in Safari the window.getSelection() will no longer report correct nodes.

@web-padawan
Copy link
Member

web-padawan commented Oct 4, 2018

Early prototype pushed as https://github.com/vaadin/vaadin-rich-text-editor/tree/proto/wrap-quill

Problems identified:

  1. shadow selection polyfill only works partially in Safari. Example:
  • type some text
  • select it
  • choose "Header" in the formatting select
  • nothing happens

This line gets empty range although works in Chrome: https://github.com/web-padawan/quill/blob/303d73fdd4bdf50a678877caf24927311305d91a/modules/toolbar.js#L87

  1. shadow selection polyfill produces lots of log output in Safari while cursor is inside of the editor, causing a performance bottleneck and even handing browser tab at some point (investigating)

  2. ShadyCSS scoping is not fully applied to select (Firefox 62, Edge):

screen shot 2018-10-04 at 9 31 32

  1. HierarchyRequestError happens in Edge when focusing and typing (also looks related to the shadow selection polyfill, needs checking without it):

screen shot 2018-10-04 at 9 35 44

  1. Quill styles not applying at all in IE11. Keyboard shortcut still work, e.g. Ctrl + B makes the text bold. Some errors in the console, related to shadow DOM support patch:

screen shot 2018-10-04 at 9 40 07

@web-padawan
Copy link
Member

Update:

  1. Shadow selection polyfill must be only invoked in Safari, it will break in IE11, Edge and Firefox

  2. Shadow selection polyfill does not work properly with Quill out of the box because of this line: https://github.com/quilljs/quill/blob/96e38e92637b75b907579d0cc1b201920aebe38c/core/selection.js#L32

See the listener in the polyfill where setTimeout is also used and the timeout is 0: https://github.com/GoogleChromeLabs/shadow-selection-polyfill/blob/9eff233c765a685bbf488b2f8eba0d3d75f44cbd/shadow.js#L74

@aadamsx
Copy link

aadamsx commented Oct 8, 2018

Are there plans to wrap the ckeditor too? I've used both, and ckeditor has always been full featured.

@web-padawan
Copy link
Member

@aadamsx we have decided to build initial version on top of Quill. In the future we might consider changing the internal implementation based e.g. on customer requests, but no such plans for now.

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 a pull request may close this issue.

4 participants