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

[Feature] Support Shadow DOM v1 #1472

Closed
ghost opened this issue May 27, 2017 · 28 comments
Closed

[Feature] Support Shadow DOM v1 #1472

ghost opened this issue May 27, 2017 · 28 comments

Comments

@ghost
Copy link

ghost commented May 27, 2017

Native support for Shadow DOM v1 is hitting browsers, and Polymer v2 was just released. I am requesting support for Shadow DOM as a feature because there are just too many bugs to try to describe. When Quill is used on an element inside a ShadowRoot, it is just broken in so many ways.

Rather than try to have the community try to document and submit what may be dozens of separate behavior bugs, I suggest that the project team needs to take on the specific task of making all the existing demos/samples and tests work in Shadow DOM and issue a future release with support for Shadow DOM a specific feature.

@jhchen
Copy link
Member

jhchen commented May 28, 2017

Quill's development is driven by volunteers, like many other open source projects of its kind. Those interested http://caniuse.com/#feat=shadowdomv1 may be helpful to track specific browser support.

@ghost
Copy link
Author

ghost commented May 28, 2017

Hey Jason, I think I fixed the issues with supporting ShadowDOM. It only required a few small changes to selection.js. You can view the changes here:

https://github.com/arsnebula/quill/commit/a6489a626f0e5d186ea9114d64efd0a3e1c44186#diff-ae237139107d046c67dc8622d25bace9

Unfortunately, I am currently developing on windows and ran into some problems with building the project due to linter issues, and npm script syntax. Doing a proper pull request is probably going to be a problem for me.

@ghost
Copy link
Author

ghost commented May 29, 2017

If anyone is interested in using Quill in a Polymer web component, I published a package on webcomponents.org - https://www.webcomponents.org/element/arsnebula/polymer-quill

@jhchen
Copy link
Member

jhchen commented May 30, 2017

Thanks! I'll take a look at the code this weekend. How are you testing the shadow dom support?

@ghost
Copy link
Author

ghost commented May 30, 2017

I am developing an editor component for my application using Polymer which has its own testing framework Web Component Tester.

@ghost
Copy link
Author

ghost commented Jun 1, 2017

Found some more problems tonight. Indent doesn't work with blockquote or list. List and blockquote are working at the initial format, but calling format with indent +1 or using the TAB key doesn't do anything.

@jhchen
Copy link
Member

jhchen commented Jun 5, 2017

Looks like it will be harder than expected to support ShadowDOM. I'll take another look when the official spec is stable. In the meantime those interested feel free to suggest small, non-regression additions with automatable tests to Quill to aid in this eventuality.

@lastmjs
Copy link

lastmjs commented Jul 7, 2017

@chrismbeckett Does your polymer-quill component work well?

@lastmjs
Copy link

lastmjs commented Jul 7, 2017

@jhchen Isn't the spec stable at v1, with Safari and Chrome shipping native implementations?

@ghost
Copy link
Author

ghost commented Jul 8, 2017

@lastmjs So far everything works except TAB key doesn't tab, and lists don't indent, but that might just be my inexperience with the QuillJS API. For my app, I don't need TAB or indents so it is working well for me, and it works in the latest versions of Chrome/Safari (native) and Edge/Firefox with the WC polyfills.

@ergo
Copy link

ergo commented Aug 23, 2017

@jhchen I've tested the component with the changes that chris made under 1.0 spec, everything behaved correctly at glance, if this could be merged it would be awesome. The spec is stable for quite a while now.

The list's not indenting seem to be some kind of css issue, as the delta and html produced seem to have proper classes applied.

Note: I've tested the quilljs build from his webcomponent.

@jhchen
Copy link
Member

jhchen commented Oct 23, 2017

Browsers take a long time to implement specs and browser users care if the feature is actually available, not if a committee has approved it. Again from http://caniuse.com/#feat=shadowdomv1, only Chrome and Opera appears to have implemented this.

A glance is also not going to cut it for a project of Quill's reach. You can see from the Contributing Guide, tests are required and any addition must work for all browsers supported by Quill.

@ghost
Copy link
Author

ghost commented Oct 23, 2017

While I won't argue with needing a pull request that follows the contributing guide and test coverage, your logic regarding the current state of the spec, and browser support doesn't make much sense.

The code required to support web components is conditional logic/feature detection. You can have a browser that supports web components and Shadow DOM, but if you are not using Quill inside a Shadow DOM it doesn't matter, the current Quill will work fine.

The primary change that needs to be made to Quill is that if the Quill node is contained within Shadow DOM, then document.activeElement doesn't work to get the current selection (which is essential to Quill). You need to get the ShadowRoot parent of the Quill Node and use shadowRoot.activeElement.

So, from a unit testing standpoint, you only need a single browser that supports Shadow DOM to validate the alternative paths through the code. Since every major browser vendor has signed off on Shadow DOM v1 and committed to supporting it, there really isn't any reason to not start supporting Shadow DOM in QuillJS right now. I don't see how there is any specific benefit to waiting to support Shadow DOM, the spec isn't going to change at this point.

From an integration testing standpoint, then yes, as each browser adds support for Shadow DOM some attention will need to be paid to ensuring there are no browser quirks that break the existing tests. While it might be more efficient to wait for every browser to support it and make the change at a future point where you can validate for every browser in one development cycle, it means that, in general, browser libraries will lag years behind every emerging feature. IMHO, that just isn't practical anymore. Feature detection and progressive enhancement are pretty much the established norm right now for anyone working professionally in web development.

Having briefly looked at the tooling and test approach for this project however, I will say this. Although it only took me < 20 lines of code to get QuillJS to work with Shadow DOM, it was very difficult to test, even manually. To meet your contributing guidelines and test requirements is going to take a substantially larger effort to re-factor how tests are run to be able to change the conditional context of the test to use Shadow DOM, etc. Especially for anyone who is unfamiliar with the project tooling, standards and structure, the LOE is likely to be much greater than a few hours (more likely days).

@jhchen
Copy link
Member

jhchen commented Oct 24, 2017

In a general Javascript library I would agree browser quirks can be viewed as an exception, but with an editor and contenteditable, they are the expectation. Quill working well across browsers despite their quirks is major value add that earned it the reach it has today.

Although it only took me < 20 lines of code to get QuillJS to work with Shadow DOM, it was very difficult to test, even manually

  1. Manual testing by definition bypasses the tooling so I'm confused why that would slow you down. If you are saying the development environment is difficult to set up let me know which step you got stuck on: https://github.com/quilljs/quill/blob/develop/.github/DEVELOPMENT.md.

  2. Not that lines of code is a good metric but engineers in my experience are perpetually in an "almost done" state of mind. The implementation difficulty should be measured when it is actually complete.

  3. I'm not asking anyone to rewrite the test infrastructure. Just start with one: initializing the editor. If you get ambitious you can try making an API change and making a direct DOM change as well.

@ghost
Copy link
Author

ghost commented Oct 24, 2017

I am sorry if my response above seemed critical of the QuillJS tooling or code. In fact, quite the opposite. I found the project exceptionally well structured and the code architecture modular, and well designed.

Yes, I very much appreciate how difficult the contenteditable is to work with, and how many quirks must be embedded in the QuillJS code to mitigate them. That is hard won knowledge by whoever wrote the code, and not easily or immediately understood by someone new to the code base trying to find and make a small change.

From my own tiny experience, I found it very difficult to debug and trace the code to build a simple conceptual model of how state was being changed, and code paths executed. This is not a tooling issue, and I am sorry that you interpreted it as such. I am just saying that even a small change to QuillJS by someone unfamiliar with the code, the tooling, and the guidelines is a very, very time consuming effort for what can amount to a relatively tiny code change - not a very efficient model for potential casual contributors.

I am going to close this issue since I don't think anything more productive can come of it. I simply don't have the time it would take for me to prepare and submit an acceptable pull request that would meet your guidelines. Karma and Jasmine are not the testing tools I use for my own software, and I don't have the time to learn all new tools for such a tiny code change.

My fork with the changes I made to 1.2.4 will remain available for anyone that wants to review the changes I made - https://github.com/arsnebula/quill. I recently tried to merge in 1.3.2 to release an updated version of my Polymer component and it no longer worked for me. A quick glance at the code changes did not surface any immediate reason why my small patch was no longer working.

@ghost ghost closed this as completed Oct 24, 2017
@fasihrana
Copy link

I've given up on this library because of no ShadowDOM support.

@BennerGe
Copy link

@fasihrana Did you found another wysiwyg editor with shadow DOM support?

@ergo
Copy link

ergo commented Dec 30, 2017

I think prosemirror might support that, I think people used codemirror and it worked correctly.

@fasihrana
Copy link

No. I ended up making something on my own using div contenteditable and Pagedown. I don’t have the wysiwyg yet but if someone knows markdown, they can just write their own posts for now.

Unfortunately I haven’t had time to modularise/webcomponentise it yet and it’s very specific to my use case.

@giona69
Copy link

giona69 commented Dec 31, 2017

I've given up on this library too because of no ShadowDOM support.

@AndreasGalster
Copy link

AndreasGalster commented Feb 28, 2018

Just pointing out that Safari has shipped shadow DOM for a year and Firefox is actively working on custom elements and shadow DOM with a shipping intent in 2 months from now I believe :).

Also based on the discussion above... Edge is pulling another IE again, they haven't even begun work and based on the arguments above we'd be waiting for support for at least another year since Edge surely will take at least that much longer to implement 😔

Also, note that even UC browser and Samsung browser now support shadow DOM. Essentially in 2 months from now 6 out of 7 browsers are supporting shadow DOM (I did not count IE btw)

@kilgarenone
Copy link

Does this mean that Quill won't work with Lit-Element too?

@raphaelrauwolf
Copy link

@kilgarenone there are actually multiple solutions out there for Lit-Element. I encourage you to join the Polymer Slack, which you can find in the bottom of the lit element page. Besides of that, i got it to work, by rewriting the range and update related functions to actually work with a shadowRoot context. Feel free to ping me on Slack when you joined it and i will help you there :)

@timblack-NukeDigital
Copy link

Quill is working in the shadow DOM for me in this branch: https://github.com/timblack-NukeDigital/quill/commits/feature/shadow-dom-support.

@heavybeard
Copy link

Any updates on this?

@gleb-svechnikov
Copy link

gleb-svechnikov commented Sep 15, 2021

Hello got into similar problem, I'm trying to use quill inside custom Web Component and it renders only toolbar but not the editor box and not the content. However it is represented in shadow DOM, but not rendered.

<custom-html-block id="91" class="html-block ql-container ql-snow"><div class="ql-editor" data-gramm="false" contenteditable="true" data-placeholder="HTML block"><p>Test content</p></div><div class="ql-clipboard" contenteditable="true" tabindex="-1"></div><div class="ql-tooltip ql-hidden"><a class="ql-preview" rel="noopener noreferrer" target="_blank" href="about:blank"></a><input type="text" data-formula="e=mc^2" data-link="https://quilljs.com" data-video="Embed URL"><a class="ql-action"></a><a class="ql-remove"></a></div></custom-html-block>
I'm on version 1.3.7 in case if it is important. Outside web component quill works fine.
Any idea how to fix it?

@rotemgrim
Copy link

Quill is working in the shadow DOM for me in this branch: https://github.com/timblack-NukeDigital/quill/commits/feature/shadow-dom-support.

@timblack-NukeDigital is your branch working in safari too?

@gfodor
Copy link

gfodor commented Aug 15, 2022

FWIW, I have a branch working on this: https://github.com/jel-app/quill/tree/shadow-dom-support

(The branch by timblack is missing a number of things if you are using the bubble theme.)

This issue was closed.
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