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/shadow dom #1

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Feature/shadow dom #1

wants to merge 12 commits into from

Conversation

robrez
Copy link
Owner

@robrez robrez commented Jan 7, 2022

Background

QuillJs does not have ShadowDOM support. It has been requested several times, most recently here: slab#2961

Many in the community are using a fork which contains some ShadowDOM support; The fork was originally maintained by web-padawan and was subsequently transferred to vaadin/quill.

This patched version of quill is based on v1.x.x (1.3.6 specifically at the time of writing). The maintainer of Quill has implemented a very large number of changes slated for quill 2.x.x, which had its first -dev release 4 years ago. The most recent release, 2.0.0-dev.4, was published 2 years ago.

Some in the community are thinking it may be a good idea to put some effort into applying the ShadowDOM support patch into develop/2.x.x.

Some prior art in this space can be found at yananym/quill

There is also an older PR over here that has some descent amount of discussion - slab#1805

This fork is another attempt at the same task effort, hopefully we can compare notes and get something merged into quill official.

Overview

Here are some notes on this effort to patch quill/develop...

Apply Shadow-Dom Commits

Cherry-picked commits from vaadin/quill, as-needed.

  • 2186896 - applied
  • 31253d6 - applied
  • 14f924c - applied, subsequently reverted (to eliminate vaadin-specific changes)
  • 3c9b972 - applied, subsequently reverted (to eliminate vaadin-specific changes)
  • 5331cb2 - applied
  • 56a960e - applied
  • 45ff970 - skipped (no diff after resolving merge conflicts)
  • 95b8374 - applied
  • 6604cab - skipped (no diff after resolving merge conflicts)
  • 1b51673 - applied
  • 1cfe249 - applied

Fix Linter Complaints

Once the commits were all applied, there were a number of minor changes related to errors/warning raised by the new linter requirements. This was done via slab@1442e3e

Current Status

task status notes
build success
test:unit success* 1 test does fail, but it also fails in the main quill repo. It is this one
manual testing cursory things seem to be working okay in chrome

Observations, Musings

Generally, things seem to have worked well!

While applying the commits, I did notice two particular areas I found noteworthy in the diff

First, there are a couple of places that attempt to determine whether a given node contains another node. One of these routines has been added to quill itself. This one is not shadowDom-capable. Does it need to be? Another was added via the vaadin patch: emitter.js. If needed, perhaps this can be extracted to a utility for use in each location.

Next, quill seems to have introduced a new WeakMap of "instances" which is used by emitter. IT seems to be hydrated by quill.js. The vaadin patch achieves something similar via its introduction of an array of emitters, hydrated here. Should quill's WeakMap be consulted? It doesn't seem to be used any longer outside of Quill.find which only appears to be referenced from within the documentation. In an n older MR.. it is noted that EMITTERS causes memory leaks and, at that time, there was a desire to use weakMaps -- #1

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 this pull request may close these issues.

3 participants