Add shadow dom support #1805

43081j · 2017-11-03T09:34:26Z

This is still WIP.

Trying to add support for quill being used inside a shadow root. This involves:

Referencing the root node rather than document wherever necessary (e.g. when getting selections)
Simplifying event handling to avoid having to search the DOM for editors (as querySelector won't penetrate through shadow boundaries)
Searching for containers (if given a selector) from the root node rather than the document

The primary issue here is that anywhere we try find editor nodes or get selections will not work if the editor is in a shadow root as none of these operations penetrate the boundaries of shadow DOM.

TODO & request for help/remarks:

Squash commits
Figure out why, when running the full test suite inside a shadow root, the size of a <br> (via getBoundingClientRect) is off by 10 pixels in height
Consider using a weakmap instead of a set somehow, for tracking emitters (for GC)
Consider reverting the mouseup/down changes if we meant to listen on the document for a reason

43081j · 2017-11-03T09:35:21Z

core/emitter.js

@@ -4,15 +4,13 @@ import logger from './logger';
 let debug = logger('quill:events');

 const EVENTS = ['selectionchange', 'mousedown', 'mouseup', 'click'];
+const EMITTERS = new Set();


should we use a weakmap for this? not sure what the value would be though..

43081j · 2017-11-03T09:36:07Z

core/selection.js

-    this.emitter.listenDOM('mousedown', document.body, () => {
+    const node = (this.rootDocument === document ? document.body : this.rootNode);
+
+    this.emitter.listenDOM('mousedown', node, () => {


should these be reverted to document? why do we listen on document for mouse events? shouldn't we only listen on the editor container?

We want to know if you drag outside of the boundaries of the editor container. See the lines below this one.

ah fair enough, this can probably be reverted then as its specific to the body rather than the container.

jhchen · 2017-11-05T23:29:08Z

Some general thoughts:

I would like to see some tests to guide this development. There are lots of ways to implement this but there's no point in debating the best way without a starting point of what even works.

Second we cannot use any features IE11 does not support (like Set) which is why the emitter was implemented the way it is.

43081j · 2017-11-07T09:16:49Z

ah fair enough, it can be changed to an array.

as for tests, the "WIP" part of this PR was that i need to add tests, don't worry 👍

so far i have been testing by placing the container for all tests in a shadow root. to ensure all tests which pass outside of one, pass inside of one too.

i will add some tests soon as i can, though.

i did find that a couple of tests fail in a shadow root because a <br> is somehow taller..

43081j · 2017-11-07T09:30:36Z

@jhchen something weird definitely happens with those br tags...

in chrome, if i make a simple element:

<div>
    #shadow-root (open)
      <br>
    <br>
</div>

The outer <br> has all zero values for its bounding rect.
The inner (shadow) <br> has correct values.

ShaiUX · 2017-11-22T11:59:21Z

any update on this? any help needed?

43081j · 2017-11-22T12:02:08Z

sorry for the slight delay, haven't been around recently.

i'll check this out tonight and see if i can get the tests added. current failures seem to be due to the size of a line break differing in shadow DOM.

43081j · 2017-11-28T19:06:01Z

@jhchen i just ran the full test suite inside a shadow DOM container and all tests pass in chrome now (css was not being loaded the first time i tried so p tags were taller in shadow roots).

So at least this means now the aim of this PR has been met.

I'll try put some work in on writing additional tests next

jhchen · 2017-11-29T06:20:09Z

core/emitter.js


 EVENTS.forEach(function(eventName) {
  document.addEventListener(eventName, (...args) => {
-    [].slice.call(document.querySelectorAll('.ql-container')).forEach((node) => {


This was added as a memory leak way to add event listeners on global objects. If WeakMap were available it would be used like the comment mentions but since it is not it does this which by the way is how WeakMaps work under the hook.

The old method won't work with shadow DOM due to the fact that querySelectorAll will not penetrate through shadow roots.

Though I can see how EMITTERS could keep growing if you repeatedly make new instances (and they never be released). Needs some thought..

Why couldn't emitters be passed as an initialization argument? Making this global is a recipe for leaks and also makes encapsulating the events for ShadowDOM very hard. It seems we might be able to make emitter into an interface, then have a GlobalEventEmitter and a ShadownDOMEventEmitter where Global does the current global things and the ShadowDOMVersion is initialized with the ShadowRoot or editor root.

I'd be down to prototype this, but maybe there is some other consideration. I'd like to get perspective from yall before working on it. What do you think?

jhchen · 2017-11-29T06:24:48Z

core/emitter.js

@@ -30,8 +29,10 @@ class Emitter extends EventEmitter {
  }

  handleDOM(event, ...args) {
+    const target = (event.composedPath ? event.composedPath()[0] : event.target);


Why is this needed?

If a listener exists above a shadow root, but the event fires from within said shadow root, the target will be the host of the shadow root rather than the element which fired the event.

composedPath will handle this, though it looks like support for it isn't well documented.

jhchen · 2017-11-29T06:32:32Z

core/selection.js

@@ -22,12 +22,13 @@ class Selection {
    this.composing = false;
    this.mouseDown = false;
    this.root = this.scroll.domNode;
+    this.rootDocument = (this.root.getRootNode ? this.root.getRootNode() : document);


I'm not sure I like this approach as it creates an extra state and scope dependency. A generalized solution would be much preferred which would also cut down on code duplication in any other modules. Can we use ownerDocument instead?

afaik ownerDocument will always be the owning document, not the shadow root (a document fragment)

A possible solution for now:

this.rootDocument = this.getDomRoot(this.root)

getDomRoot(ctx) { if (ctx === document) { return ctx } if (HTMLElement.prototype.attachShadow) { if (ctx instanceof ShadowRoot) { if (typeof ctx.getSelection === 'function') { return ctx } return document } } return this.getDomRoot(ctx.parentNode) }

A modified variation from here.

jhchen · 2017-11-29T06:33:08Z

core/selection.js

-    this.emitter.listenDOM('mousedown', document.body, () => {
+    const node = (this.rootDocument === document ? document.body : this.rootNode);
+
+    this.emitter.listenDOM('mousedown', node, () => {


We want to know if you drag outside of the boundaries of the editor container. See the lines below this one.

yorrd · 2017-12-22T21:36:18Z

Let me know if I can help you run tests or figure out a specific issue! Very interested in this as well!

43081j · 2017-12-23T17:59:04Z

it just needs tests writing now i think.

there's still some comments @jhchen hasn't responded to but the code is fine either way.

i won't be massively available during christmas but will try get the tests written soon.

jhchen · 2017-12-25T19:12:10Z

Which comments require my response?

43081j · 2017-12-25T19:14:42Z

just whether we are ok with the EMITTERS array. if you keep making instances, of course the array will just grow until we can use Set. im fine with that because i dont see why anyone would ever make so many instances, though.

43081j · 2017-12-25T19:49:37Z

i've added some tests now. just the basics.

to be honest we could test literally anything we already test in non-shadow dom. so ive just tested that the common things work.

jhchen · 2017-12-26T16:11:35Z

Sorry from your comment I thought you were going to think about it some more and come back and it is tricky to solve both needs. But to answer your followup no it is not okay to use the memory leaky EMITTERS array. A huge portion of users use Quill in a single page app where memory leaks are a big concern. There are some Github Issues on the subject of SPAs and memory leaks if you want more details.

43081j · 2017-12-26T16:30:19Z

yup my mistake, should have updated you. i did have a think about it.

the old method is now impossible and was a little unfortunate its self too, because of having to arbitrarily attach instances to dom nodes.

the current method is correct IMO other than the fact we have no way of knowing when an instance has been disposed of...

a memory leak is important to everyone, whether there are a lot of SPAs concerned with it or not isn't relevant. this leak will occur when you repeatedly make new instances over time (which is something people should not be doing, but we know it will happen). the solution to that is to have teardown somewhere, possibly in the constructor we can check if old instances are still in use.

43081j · 2017-12-26T16:41:46Z

core/emitter.js

@@ -21,6 +19,7 @@ class Emitter extends EventEmitter {
  constructor() {
    super();
    this.listeners = {};
+    EMITTERS.push(this);


@jhchen we could possible iterate through EMITTERS here, looking for any with a container which is no longer in the DOM tree. this isn't straight forward, though, because document.body.contains(node) is false for nodes in shadow trees. we would have to go up the tree (getRootNode) until we reach a node which is in document , if ever.

ah this won't work, we don't have any idea what container is at this point

43081j · 2017-12-26T16:52:20Z

test/unit/core/selection.js

@@ -37,6 +37,42 @@ describe('Selection', function() {
    });
  });

+  describe('shadow root', function() {


@jhchen am i ok to just disable these where attachShadow isn't supported? firefox tests will fail until i do as they're unusually far behind in implementing the shadow dom spec.

giona69 · 2018-01-04T15:16:52Z

I tried this fork in a real application in Polymer 2 and I found two issues:

1- no event is handled: as it was said,

if (target === node || node.contains(target))

will always be false, as node.contains do not work across shadow root boundaries; hence no event will be targeted, hence no toolbar will be displayed (if bubble) and no action on the selection done.

writing

if (target === node || true)

of course will work

2- if the event works, I found a bug on Safari where we have this problem with .getSelection(), as the API DocumentOrShadowRoot.getSelection() is not yet implemented ... there is a bug open on Safari, here all the docs:

ProseMirror/prosemirror#476
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/getSelection
https://bugs.webkit.org/show_bug.cgi?id=163921

for Safari the only way to make it work, currently, is to force shadydom on loading

43081j · 2018-01-04T15:36:49Z

ah much appreciated @giona69 , its helpful to have someone try it in a real world project.

your first issue, im surprised i didn't notice the contains call. i had commented about that exact issue elsewhere in the codebase recently. ill check it out soon as im able to

the second one, we can't really start shipping polyfills etc. we should probably either not support shadow roots in safari for now or find some workaround when we detect that getSelection doesn't exist.

giona69 · 2018-01-04T17:37:47Z

@43081j I will go on developing on this and test it ... forcing shadydom on safari

for getSelection I do not see any workaround unfortunately, but will go on searching for an alterntive.

jhchen · 2018-01-22T06:38:41Z

So no Firefox support and major issue in Safari (though Webkit is used by Chrome too?). Is Chrome the only browser ready for this?

43081j · 2018-01-22T08:27:26Z

@jhchen shadow dom v1 is shipped in chrome, safari and firefox AFAIK.

im not sure what the best way for us to work around the safari bug is. if not for that, it would work fine there too.

firefox should work as they shipped much of shadow dom v1 recently.

we also have a bug with event handlers according to giona's post. ill get on that today if i can

43081j · 2018-01-22T18:25:02Z

@giona69 can you give the latest branch a try with your project if you don't mind?

i added a small helper for the contains problem. so we now traverse the DOM tree using getRootNode (if available) and check contains of each host element.

petecarapetyan · 2018-01-24T03:20:18Z

@jhchen
With polyfills I believe all browsers are shadow dom supported?
https://www.polymer-project.org/1.0/docs/browsers
This ^ is an older report but it has better visuals.
More recent report is here but it mixes unrelated concerns such as Templates
https://www.polymer-project.org/2.0/docs/browsers
Or, not considering polyfills, this
https://caniuse.com/#feat=shadowdomv1

web-padawan · 2018-10-02T13:34:46Z

@jhchen As of now, Shadow DOM v1 is supported by both Chrome and Safari, and will land in Firefox 63 (can be already tested in beta).

Any chance for this PR to be revisited?

mrigdon-zz · 2020-02-26T18:29:54Z

What needs to happen to move this PR along? I'm happy to help

43081j · 2020-02-26T18:51:32Z

ive rebased onto latest master, ill have another read through the diff and see if i can remember where we got to.

mrigdon-zz · 2020-02-26T18:57:37Z

cool @43081j , thanks for the quick response

43081j · 2020-02-26T20:20:45Z

@mrigdon summary:

build fails because getSelection on shadow roots isn't supported by safari yet (as with most modern things...)
there's a possible memory leak with EMITTERS in that adding a quill instance and removing it many times will leave references in the EMITTERS array. haven't had chance to think about how to prevent this
most changes are that we query from a 'root' now instead of document, which is found through getRootNode()
events are now on the root node rather than document

i guess to get this fixed up, we need a solution for safari. that could be to just not support shadow dom, i.e. default to document as a root if safari is used, at least for selection.

and to check the latest codebase for any new occurrences of accessing document so we can scope those too.

mrigdon-zz · 2020-02-27T17:00:55Z

sounds good. i'll play around with it @43081j

mfreed7 · 2021-11-11T01:07:48Z

Hello, I just wanted to check in on this issue, and point out a new proposal for a Shadow DOM compatible Selection API. This proposal has been being discussed on WICG/webcomponents#79. In particular, this comment gives a quick TL;DR summary of the proposal.

In particular, I'd be curious to hear the opinions of the Quill maintainers as to whether this proposal will suit your needs:

Is the proposal understandable to you?
Does the proposed getComposedRange() API work for your use case?
In particular, do the two parameters, selectionRoot and shadowRoots make sense? And would they allow you to easily make use of cross-scope selection information?
Do the changes to the "setter" APIs like setBaseAndExtent() seem like they'll work for your use case?
Do you have any need to get selection information "anywhere on the page", i.e. within any shadow root? This would likely require another parameter to make this easy, if so.
Do you have any other concerns or feedback?

I'd love to hear your feedback either here in this issue, or (better) on the Selection API thread. These changes are still very much in development, so your feedback can have an impact. Thanks!

43081j commented Nov 3, 2017

View reviewed changes

43081j force-pushed the selection-document branch from 21a05e9 to 1927ef3 Compare November 4, 2017 13:27

benbro mentioned this pull request Nov 20, 2017

editor in shadowroot #1826

Closed

jhchen reviewed Nov 29, 2017

View reviewed changes

benbro mentioned this pull request Dec 15, 2017

Delegate listener pattern breaks quill in (open) shadow dom #1811

Closed

43081j commented Dec 26, 2017

View reviewed changes

giona69 mentioned this pull request Jan 9, 2018

This is a very good component. Do you have any plans to migrate to Polymer 2.0-rc? chuckh/polymer-quill#13

Open

vanooo mentioned this pull request Mar 27, 2018

Cursor moves back when typing first character on new row? #1916

Closed

This was referenced Oct 2, 2018

Wrap Quill and add value property vaadin/vaadin-rich-text-editor#2

Closed

Shadow DOM support and tests #2337

Closed

KillerCodeMonkey mentioned this pull request May 8, 2019

Problem using ngx-quill with ViewEncapsulation.ShadowDom KillerCodeMonkey/ngx-quill#381

Closed

43081j force-pushed the selection-document branch from b27ccb9 to 24e1e00 Compare October 24, 2019 11:57

43081j added 3 commits February 26, 2020 18:50

add shadow dom support

22e5110

require rootnode support

074b897

require composedpath support

ce39202

43081j force-pushed the selection-document branch from 0b748a7 to ce39202 Compare February 26, 2020 18:50

boazsender mentioned this pull request Oct 19, 2021

Selection APIs for Shadow DOM WICG/webcomponents#79

Closed

mfreed7 mentioned this pull request Dec 3, 2021

Modification of selection APIs to account for shadow dom w3ctag/design-reviews#694

Closed

1 task

robrez mentioned this pull request Jan 7, 2022

Feature/shadow dom robrez/quill#1

Open

oleq mentioned this pull request Feb 11, 2022

WebComponent for CKEditor 5 ckeditor/ckeditor5#1483

Open

7 tasks

yuzuquats mentioned this pull request May 11, 2022

Bug: Binding to webcomponent shadow root fails facebook/lexical#2119

Closed

hunterloftis mentioned this pull request Oct 13, 2022

Composed Selection (Selection APIs for Shadow DOM) web-platform-tests/interop#216

Closed

web-padawan mentioned this pull request Apr 25, 2024

Support Shadow DOM v1 #2021

Open

web-padawan mentioned this pull request Jul 15, 2024

fix: implement custom toolbar formats click logic vaadin/web-components#7547

Closed

Add shadow dom support #1805

Are you sure you want to change the base?

Add shadow dom support #1805

Conversation

43081j commented Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhchen commented Nov 5, 2017

43081j commented Nov 7, 2017

43081j commented Nov 7, 2017

ShaiUX commented Nov 22, 2017 • edited Loading

43081j commented Nov 22, 2017

43081j commented Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorrd commented Dec 22, 2017

43081j commented Dec 23, 2017

jhchen commented Dec 25, 2017

43081j commented Dec 25, 2017

43081j commented Dec 25, 2017

jhchen commented Dec 26, 2017

43081j commented Dec 26, 2017

Choose a reason for hiding this comment

43081j Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giona69 commented Jan 4, 2018

43081j commented Jan 4, 2018

giona69 commented Jan 4, 2018

jhchen commented Jan 22, 2018

43081j commented Jan 22, 2018 • edited Loading

43081j commented Jan 22, 2018

petecarapetyan commented Jan 24, 2018 • edited Loading

web-padawan commented Oct 2, 2018 • edited Loading

mrigdon-zz commented Feb 26, 2020

43081j commented Feb 26, 2020

mrigdon-zz commented Feb 26, 2020

43081j commented Feb 26, 2020

mrigdon-zz commented Feb 27, 2020

mfreed7 commented Nov 11, 2021

43081j commented Nov 3, 2017 •

edited

Loading

ShaiUX commented Nov 22, 2017 •

edited

Loading

43081j commented Nov 28, 2017 •

edited

Loading

43081j Dec 26, 2017 •

edited

Loading

43081j commented Jan 22, 2018 •

edited

Loading

petecarapetyan commented Jan 24, 2018 •

edited

Loading

web-padawan commented Oct 2, 2018 •

edited

Loading