guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Fails to retain focus due to invalid range comparisons when in ShadowDOM #329

Closed bigdave closed 7 years ago

bigdave commented 9 years ago

While trying to get Scribe to work within a Polymer-based application, we ran into a problem. The proof-of-concept polymer element for Scribe keeps the editor in the light DOM, which is fine for that example, but when using the editor within a Polymer component's Shadow DOM, the editor fails to keep focus, and throws exceptions. It appears that it is attempting to compare ranges, assuming that the editor is present in window.document, when in fact is resides in the Shadow DOM document of the Polymer element.

This results in the error Uncaught WrongDocumentError: Failed to execute 'compareBoundaryPoints' on 'Range': The source range is in a different document than this range. selection.js:52.

I have setup a proof of concept for this error, which must be viewed in a browser which actually has a Web Components Shadow DOM implementation such as Chrome, Opera, or Vivaldi. The behavior will not appear in FireFox (unless you're using a fancy nightly with the toggle turned on).

bigdave commented 9 years ago

If you're looking at scribe.js as opposed to selection.js, the error is occurring on line 3844

hmgibson23 commented 9 years ago

Not sure but this could be mitigated by using ownerDocument to get the selection. However, when doing this we ran into some issues in FF.

shaunnetherby commented 9 years ago

This issue is affecting me as well. ownerDocument seems to be the same as window.document. In Chrome, if you grab the shadowRoot of the target element (el) and call getSelection on that root, the selection object has the correct nodes (which means it is in the correct document fragment). However when getRangeAt is called the range object has a common ancestor in the window.document instead of the shadowRoot. This results in the compareBoundaryPoints method throwing an error because this.range is in the window.document and scribeNodeRange is in the shadowRoot document.

  var selectionStartWithinScribeElementStart = this.range.compareBoundaryPoints(Range.START_TO_START, scribeNodeRange) >= 0;
brenthosie commented 9 years ago

I'm having this same problem. Can we get an ETA on a fix?

hmgibson23 commented 9 years ago

Is this an issue with Polymer?

miztroh-zz commented 9 years ago

Perhaps it's related to this Chrome bug: https://code.google.com/p/chromium/issues/detail?id=380690

hmgibson23 commented 9 years ago

It might be. If it's an issue with Polymer only, it's highly unlikely we'll provide a fix for this.

miztroh-zz commented 9 years ago

If it's related to the Chrome issue (very likely, as I've encountered similar issues with various content editors), then it would affect any Shadow DOM usage of Scribe (not just Polymer). Someone has already committed a fix for the Chrome issue, so it's just a matter of time before that fix is incorporated into a stable release.

cutandpastey commented 9 years ago

Its possible that the fix you mentioned won't fix this issue as the scribe.api.Selection creates a range from the global window. We could enable passing the shadow root into the constructor like:

var s = new scribe.api.Selection(myShadowRoot);

I haven't used shadow DOM much so this is just a hunch, @theefer?

miztroh-zz commented 9 years ago

window.getSelection() should (once the Chrome fix is released) return a valid selection even from Shadow DOM contexts.

shaunnetherby commented 9 years ago

I tried Chrome Canary to see if the bug fix for https://code.google.com/p/chromium/issues/detail?id=380690 would fix this issue. The bug fix doesn't change window.getSelection(); window.getSelection still returns the selection from the document context. It does fix selection.getRangeAt() to return the appropriate range. So even with that fix there seems like there would still be an issue in scribe.

I was trying my hand at fixing this issue in selection.js by looking up the root doc fragment but after performing a command (e.g., bolding) on the selection, the selection is reset so nothing is highlighted.

This is the code I used to lookup the root doc:

var rootDoc = document;

// find the parent document or document fragment
var currentElement = scribe.el.parentNode;
while(currentElement && currentElement.nodeType !== 11 && currentElement.nodeType !== 9) { // 11 = DOCUMENT_FRAGMENT_NODE, 9 = DOCUMENT
  currentElement = currentElement.parentNode;
}

// if we found a document fragment and it has a getSelection method, set it to the root doc
if (currentElement.nodeType === 11 && currentElement.getSelection) {
  rootDoc = currentElement;
}
this.selection = rootDoc.getSelection();
rrees commented 9 years ago

@bigdave Can you review in light of @shaunnetherby fixes?

bigdave commented 9 years ago

@shaunnetherby's fix looks good :+1:

Thanks a bunch!

nazar-pc commented 7 years ago

Should this be closed now?

bigdave commented 7 years ago

Yep - thanks for the support!