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

Use shadow dom if available #3749

Merged
merged 23 commits into from
Mar 31, 2021
Merged

Use shadow dom if available #3749

merged 23 commits into from
Mar 31, 2021

Conversation

davidruisinger
Copy link
Contributor

Is this adding or improving a feature or fixing a bug?

bug

What's the new behavior?

If <Editable /> is rendered within a shadowRoot, internal editor.selection is not updated correctly which (amongst other issues) leads to the cursor always jumping back to the beginning of the text while typing.

qbyHEGwORJ

How does this change work?

window.getSelection (which is internally used in slate-react) refers to the shadow host selection. Instead of always relying on window.document I've added a getDocumentOrShadowRoot util which either returns the ShadowRoot (if active Element is rendered within a ShadowDom) or Document.
So instead of window.getSelection() we use getDocumentOrShadowRoot ().getSelection()

Have you checked that...?

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)

@davidruisinger
Copy link
Contributor Author

@CameronAckermanSEL would be great if you could have a look. THX!

Copy link
Contributor

@AndrewScull AndrewScull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found myself facing the same problem so thanks for putting this PR together!

*/

export const getDocumentOrShadowRoot = (): Document | ShadowRoot => {
return window.document.activeElement?.shadowRoot ?? window.document
Copy link
Contributor

@AndrewScull AndrewScull Aug 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems work for one level of shadowRoot but not if they are nested. I think it needs to be done bottom up from the element in question to find the shadowRoot that contains that element.

I've only come across searches up the node tree to find the appropriate root with something like below (illustrative only). It's probably something that'd be cached on the editor for use when it's needed.

let n = editor;
for (let n = n.parentNode; n; n = n.parentNode) {
  if (n instanceof Document || n instanceof ShadowRoot) break;
}
const documentOrShadowRoot = n || window.document;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting. Could you provide a simple example codesandbox (or something similar)?
You can look at the example I created in this PR as a starting point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forked your branch of this pull request and updated the Shadow DOM example to also include a nested Shadow DOM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated that branch with a commit that gets the nested version working too. It's far from being PR-ready and there are some behaviours that I can't yet fully explain so if you're more familiar with these APIs it'd be great to hear what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another go at this (branch) after coming across getRootNode() to avoid the tree traversal. However, I'm still running into an issue with key events that seems to be a React issue with shadow DOM. On Chrome, the undo key combo doesn't trigger the event handler and so the undo doesn't happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. I merged your changes into this PR. I've also updated a few smaller things (sorry for the mess with all those commits. I was trying out a few things...)

@davidruisinger
Copy link
Contributor Author

@CameronAckermanSEL , @ianstormtaylor , @BrentFarese , @timbuckley could one of you guys please have a look at this PR?
I really need to get slate working within ShadowDom and it seems that I'm not the only one with this use-case. THX!

@ellenblaine
Copy link

Hey @flavordaaave thanks for putting this PR together! I also have this use case and am wondering what the status of this PR is. I've really enjoyed Slate in other projects and would like to use it again. Thank you!

@davidruisinger
Copy link
Contributor Author

davidruisinger commented Nov 3, 2020

Hey @flavordaaave thanks for putting this PR together! I also have this use case and am wondering what the status of this PR is. I've really enjoyed Slate in other projects and would like to use it again. Thank you!

Hey @ellenblaine unfortunately none of the maintainers seems to be interested in reviewing/merging this PR which is why we have created our own (internal) fork for now. We still hope that at some point slate will be compatible with beeing rendered within a ShadowDom.


if (!(root instanceof Document || root instanceof ShadowRoot))
throw new Error(
`Unable to find DocumentOrShadowRoot for editor element: ${el}`
Copy link

@ellenblaine ellenblaine Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed an interesting issue experimenting with this branch myself: The selectionchange event fires as the editor unmounts (we have a drawer component that contains the editor -- closing the drawer causes selectionchange to fire). When this happens, the selectionchange event fires and findDocumentOrShadowRoot is called. This error is hit like so:

// ... selectionchange event calls this function ...
const el = ReactEditor.toDOMNode(editor, editor) // el is the editor, which isn't currently in the DOM
const root = el.getRootNode() // root is another element that is not in the DOM or the shadow root
// error throws because root is not Document or ShadowRoot

Any ideas? No worries if not -- this could definitely be an issue that's unrelated to Slate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't reproduce this myself, yet. We are also unmounting our slate editor when the user navigates to a different section/page but so far I didn't get this error.
Could you setup a minimal example illustrating this issue?
One idea how to get around this would be to call blur() before the drawer is closed but obviously this would just be a temporally workaround.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'm seeing this same behavior with an unmounting editor... I tried bluring it with a useEffect but that's probably too late.. still errors, but this time in the ReactEditor.blur(editor).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to fix an issue related to this change, can you explain why we need to throw an exception here @davidruisinger? I believe I'm missing context. For reference here's my draft pr for fixing the iframe crash, would be good to get some 👀 on it before I add cypress tests

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, these instanceof checks should use getWindow the way other similar checks do

const window = ReactEditor.getWindow(editor)
if (data instanceof window.DataTransfer) {

const window = getDefaultView(value)
return !!window && value instanceof window.Node

@tomdng
Copy link
Contributor

tomdng commented Dec 8, 2020

Hey @flavordaaave thanks for putting this PR together! I also have this use case and am wondering what the status of this PR is. I've really enjoyed Slate in other projects and would like to use it again. Thank you!

Hey @ellenblaine unfortunately none of the maintainers seems to be interested in reviewing/merging this PR which is why we have created our own (internal) for for now. We still hope that at some point slate will be compatible with beeing rendered within a ShadowDom.

Hey @flavordaaave have you attempted to contact the maintainers through the Slate Slack channel by any chance? It looks like the maintainers have their hands full trying to establish a release system for Slate and dealing with issues with the default branch for this repo. Perhaps you might find better luck there raising an issue there since there's a lot of noise with the active issues/PRs here.

@davidruisinger
Copy link
Contributor Author

Hey @flavordaaave thanks for putting this PR together! I also have this use case and am wondering what the status of this PR is. I've really enjoyed Slate in other projects and would like to use it again. Thank you!

Hey @ellenblaine unfortunately none of the maintainers seems to be interested in reviewing/merging this PR which is why we have created our own (internal) for for now. We still hope that at some point slate will be compatible with beeing rendered within a ShadowDom.

Hey @flavordaaave have you attempted to contact the maintainers through the Slate Slack channel by any chance? It looks like the maintainers have their hands full trying to establish a release system for Slate and dealing with issues with the default branch for this repo. Perhaps you might find better luck there raising an issue there since there's a lot of noise with the active issues/PRs here.

@tomdng Yes, I posted the PR there as well (#contributing channel) but I didn't help either.
There might be a small chance that this is working out of the box with React 17 (with the changes to the event delegation). Unfortunately I haven't had time to validate this but will do asap

@cpmotion
Copy link

Can we make this fix happen? Regularly utilize Slate inside a shadow DOM. PR looks good.

tomdng and others added 19 commits January 25, 2021 14:17
* Moved getDirtyPaths() into the editor object so it can be customized via plugin
Co-authored-by: liuchengshuai001 <liuchengshuai001@ke.com>
Shadow DOM brings different behaviours for selection and active
elements. This adds an example where the editor is found within a shadow
DOM, in fact, the editor is two levels deep in nested shadow DOMs.

The handling of selections means that this editor doesn't work properly
so Slate will need to be made aware of the shadow DOM in order to fix
this.
If the editor is within a ShadowDom, the selections and active element
APIs are implemented on the ShadowRoot for Chrome. Other browsers still
use the Document's version of these APIs for the shadow DOM.

Instead of defaulting to `window.document`, find the appropriate root to
use for the editor in question.
Chrome will always return true for isCollapsed on a selection from the
shadow DOM. Work around this by instead computing this property on
Chrome.

https://bugs.chromium.org/p/chromium/issues/detail?id=447523
@davidruisinger
Copy link
Contributor Author

Hey @flavordaaave thanks for putting this PR together! I also have this use case and am wondering what the status of this PR is. I've really enjoyed Slate in other projects and would like to use it again. Thank you!

Hey @ellenblaine unfortunately none of the maintainers seems to be interested in reviewing/merging this PR which is why we have created our own (internal) for for now. We still hope that at some point slate will be compatible with beeing rendered within a ShadowDom.

Hey @flavordaaave have you attempted to contact the maintainers through the Slate Slack channel by any chance? It looks like the maintainers have their hands full trying to establish a release system for Slate and dealing with issues with the default branch for this repo. Perhaps you might find better luck there raising an issue there since there's a lot of noise with the active issues/PRs here.

@tomdng Yes, I posted the PR there as well (#contributing channel) but I didn't help either.
There might be a small chance that this is working out of the box with React 17 (with the changes to the event delegation). Unfortunately I haven't had time to validate this but will do asap

Unfortunately the issue still remains with React 17. So these changes are still needed to get slate working within a shadow DOM. I updated this PR to resolve all open conflicts with the master branch.

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2021

🦋 Changeset detected

Latest commit: dcf48dd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ianstormtaylor ianstormtaylor merged commit 0473d0b into ianstormtaylor:master Mar 31, 2021
@github-actions github-actions bot mentioned this pull request Mar 31, 2021
@davidruisinger davidruisinger deleted the use_shadow_dom_if_available branch March 31, 2021 20:44
@Mangatt
Copy link
Contributor

Mangatt commented Apr 5, 2021

Is it possible that this PR is causing crash in Chromium, @davidruisinger ? See #4170

@juliankrispel
Copy link
Collaborator

Is it possible that this PR is causing crash in Chromium, @davidruisinger ? See #4170

Indeed this brakes iframe support, here's is the code with the exception thrown:

if (!(root instanceof Document || root instanceof ShadowRoot))
  throw new Error(
    `Unable to find DocumentOrShadowRoot for editor element: ${el}`
  )

for context: gets thrown if root refers to a document element inside an iframe.

@ottworks
Copy link

I'm getting Unable to find DocumentOrShadowRoot for editor element: [object HTMLDivElement] when I unmount an editor (it's in a react-bootstrap modal). getRootNode returns the now-detached modal div.

@github-actions github-actions bot mentioned this pull request Aug 10, 2021
kevinansfield added a commit to kevinansfield/mobiledoc-kit that referenced this pull request Jul 26, 2022
- quick hack to test support for rendering inside of shadow dom
- based on similar support added to Slate in ianstormtaylor/slate#3749
kevinansfield added a commit to TryGhost/mobiledoc-kit that referenced this pull request Jul 27, 2022
no issue

- quick hack to test support for mobiledoc instances being rendered inside of shadow dom
- based on similar support added to Slate in ianstormtaylor/slate#3749
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.