-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Support rendering Draft.js into iframes (nested browsing contexts) #765
Conversation
ca12190
to
cac9f0a
Compare
Just rebased with master and resolved conflicts in npm / yarn "draft-js": "brandcast/draft-js-built#333bf90",
"fbjs": "brandcast/fbjs-built#3961252",
"react": "brandcast/react-built#ab31c46",
"react-dom": "brandcast/react-dom-built#6cd1db3", in the browser <script src="https://rawgit.com/brandcast/react-built/15-stable/dist/react.js"></script>
<script src="https://rawgit.com/brandcast/react-dom-built/15-stable/dist/react-dom.js"></script>
<script src="https://cdn.jsdelivr.net/immutable.js/latest/immutable.min.js"></script>
<script src="https://cdn.jsdelivr.net/es6.shim/0.35.1/es6-shim.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/babel-core/5.8.34/browser.js"></script>
<script src="https://rawgit.com/brandcast/draft-js-built/master/dist/Draft.js"></script> |
There seems to be a problem with this patch when selecting text (
|
@philipp-spiess Glad you gave it a try, and thanks for reporting back! Can you share the set up you put together (maybe a small repo, or a fiddle / codepen thing) that led to getting that error after select all + backspace, as well as what browser you were using? And do you get the error with the small demo I made? See http://codepen.io/acusti/pen/RGEJZE?editors=0011 Would love to track down that issue if possible, but I can’t reproduce it in my demo or in the app we built off of the forks I mention in #765 (comment) |
@acusti argh never mind. After setting up the fiddle and frustrating why it works there I read your comments again and found out I forgot to add the other patches ( Sooo. How about we get someone to merge this and the PR on react? 😎 |
Right on. Let's do it 👍! |
Is the fix merged to all the libraries? |
@rogyvoje Hasn’t been any movement on this that I know of in React or here in Draft.js. |
@acusti Well, is there anything that we can do to help them merge faster or something like that? 😕 |
cac9f0a
to
c8760c4
Compare
Hey together, is this request integrated anytime soon? I also need to render a draftjs component into an iFrame! It would be awesome, if you could integrate it! Thanks! @spicyj maybe? |
Would love to see this merged in! Any updates? |
Guys, are there any updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi – sorry for the very slow response here. Added a few inline comments; if you have time to update and rebase then this would be good for merge. For some of the instanceof checks you may need to work around Flow. Something more like:
if (node.nodeType === 1) {
var element: Element = (node: any);
}
@@ -28,8 +28,12 @@ function editOnBlur(editor: DraftEditor, e: SyntheticEvent): void { | |||
// issue to be certain, checking whether the active element is `body` | |||
// to force it when blurring occurs within the window (as opposed to | |||
// clicking to another tab or window). | |||
if (isWebKit && getActiveElement() === document.body) { | |||
global.getSelection().removeAllRanges(); | |||
var doc = e.target && e.target.ownerDocument instanceof Document ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this check? I'd expect e.target.ownerDocument would always be a document. Also, you need e.target.ownerDocument.nodeType === 9
or else this won't work in multiple realms – exactly the case this is trying to support.
@@ -20,7 +20,7 @@ var getSelectionOffsetKeyForNode = require('getSelectionOffsetKeyForNode'); | |||
*/ | |||
function findAncestorOffsetKey(node: Node): ?string { | |||
let searchNode = node; | |||
while (searchNode && searchNode !== document.documentElement) { | |||
while (searchNode && searchNode !== searchNode.ownerDocument.documentElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just: searchNode.nodeName !== 'HTML'
?
invariant( | ||
node instanceof Element && node.getAttribute('data-contents') === 'true', | ||
win && node instanceof win.Element && node.getAttribute('data-contents') === 'true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.nodeType === 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sophiebits To appease Flow, I needed to change it to:
node = node.firstChild;
invariant(
node && node.nodeType === 1 && typeof node.getAttribute === 'function' &&
node.getAttribute('data-contents') === 'true',
'Invalid DraftEditorContents structure.',
);
Does that seem reasonable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in this case, because of the invariant
expression, I can’t really cast node
as an Element
, at least not in any way that makes much sense.
@@ -18,7 +18,8 @@ | |||
* found on the DOM tree of given node. | |||
*/ | |||
function getSelectionOffsetKeyForNode(node: Node): ?string { | |||
if (node instanceof Element) { | |||
var win = node.ownerDocument.defaultView; | |||
if (win && node instanceof win.Element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.nodeType === 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, this needed typeof node.getAttribute === 'function'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the above reply is incorrect. Here, the issue is solved by casting node
as an Element
, as in var element: Element = (node: any);
if (styleToCheck) { | ||
currentStyle = currentStyle.add(styleToCheck).toOrderedSet(); | ||
} else if (node instanceof HTMLElement) { | ||
} else if (node instanceof win.HTMLElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.nodeType === 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, refining the node type to Element
is insufficient, because the style
object is a part of the HTMLElement
interface. I can make Flow be ok with that with:
const htmlElement: HTMLElement = (node: any);
But seems strictly speaking to be inaccurate?
invariant( | ||
link instanceof HTMLAnchorElement, | ||
link instanceof win.HTMLAnchorElement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link.nodeName === 'A'
// IMG tags | ||
if ( | ||
nodeName === 'img' && | ||
node instanceof HTMLImageElement && | ||
node instanceof win.HTMLImageElement && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.nodeName === 'IMG'
@@ -459,7 +463,7 @@ function genFragment( | |||
|
|||
while (child) { | |||
if ( | |||
child instanceof HTMLAnchorElement && | |||
child instanceof win.HTMLAnchorElement && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
@sophiebits Thanks so much for the review! It's been a while, but if I remember correctly, all the instanceof checks were specifically to appease flow, because checking nodeType didn't seem to be enough. But I will need to refamiliarize myself with the code and verify to see if that's the case (I may misremember, or the core DOM type defs might've changed). So happy to see this getting picked up! I'll try to carve out some time to revive this work in the next week. |
Hi - just came across this, and it looks like we just need a few tweaks to get this ready. I can try to fix it up if I have time, or @acusti if you still have interest. |
Is there any movement on this? bing able to render draft.js inside an iframe (using https://github.com/ryanseddon/react-frame-component) would be an absolute life saver |
@danwoodbury It’s being actively worked on (by me) |
this issue need to be solved so badly |
@danwoodbury same boat :) |
27564fd
to
e41f4ee
Compare
e41f4ee
to
99c5348
Compare
For anyone out there who has been looking for this functionality (rendering draft.js editors inside an iframe using https://github.com/ryanseddon/react-frame-component or similar), you can try it out by updating the "draft-js": "brandcast/draft-js-built#4cdfd28",
"react": "brandcast/react-built#a384c20",
"react-dom": "brandcast/react-dom-built#3804391", Important note: that particular draft.js build (not this PR) is intended to be used with immutable v4 (which is still only an RC and full of breaking changes). Also, those versions of React are based off of v16. |
c441b6a
to
197d481
Compare
Need for guard is a result of the fact that the code now gets DOM node’s owner window instead of global window. Previously, in the cases where the DOM node isn’t in a window, the code would run but not do anything. With this change, the function will early return and the rest of the code won’t be run.
@sophiebits @flarnie Happy 2018! I merged latest Hoping we can get this rolling again now that we’ve all returned from winter vacation. I believe I’ve addressed all of Sophie’s requested changes and I don’t know of any further work to do. This PR won’t accomplish the goal of making Draft work when rendered into an iframe on its own, because that will require landing facebook/react#9184. But it will make Draft independently compatible to operate across nested browsing contexts (iframes or separate browser windows opened from the parent), cleaning up some code along the way (-9 net change in # of lines). |
@acusti Thanks for updating this! Reviewing it now, and Happy New Year to you too! 🎉:) |
Since you mentioned the other PR on React, facebook/react#9184 , I looked at that and I think we should figure out whether that will be landed or not first. I'm really impressed with your ongoing work on both these PRs, and overall on the use case of React in iframes. Thanks for sticking with it. I'm adding it to the React team agenda, hopefully we can get an answer about this soon. |
@flarnie Thanks for the updates on the React side of things! I wanted to update to say that I realized that there is a valid use case for being able to render a Draft editor, even without the changes in React that this PR partially depends on: you can use Draft to render a I wanted to call out this use case because wanting to render the HTML of editor content into an iframe to preview it with CSS that shouldn’t apply to the rest of the app is realistic and useful on its own. Rendering an interactable editor still won’t work, of course, because of limitations in React’s |
@flarnie The React PR to support iframes for selection event handling and restoration has been merged facebook/react#12037 (comment) 🎉 I see we have 8 conflicts above for us to go through, but I’m hoping this means we can get this merged soon, I know that v0.11.x has been pending for a while. Could this be the kind of thing that could land in the 0.10.x range? |
Hi, how is this issue going on? need a stable version released soon, thanks @acusti , the brandcast branch is useful for now. my usage is using draft-js in iframe, |
Thanks for your great work @acusti I have worked for a while on this issue, and one pattern that i see frequently is accessing the iframe const domSelection = editorNode.ownerDocument.defaultView.getSelection(); invariant(
- node instanceof Element &&
+ node instanceof defaultView.Element &&
node.getAttribute('data-contents') === 'true',
'Invalid DraftEditorContents structure.',
); We can sometimes access it if we have a DOM node in the scope. But sometimes we can't. For example in component/handlers/edit/commands/keyCommandBackspaceToStartOfLine.js. We can add an optional prop called |
Is there a way that we can push this issue forward? Is there an area where help is needed as it seems to have gone a little dormant. |
Thanks @haikyuu. I've got that branch working with popout windows 👍 |
Summary
Removes Draft’s dependency on the global
window
anddocument
objects and makes it work across nested browsing contexts (iframes) by using the document and window relative to the browsing context in which Draft is being used. React itself makes similar assumptions about being able to use the globalwindow
anddocument
objects, as documented in facebook/react#427, so this PR depends on facebook/react#9184. All together, these changes make it possible to render Draft.js inside an iframe, which in turn makes it possible to have a UX where the page includes multiple visible selections, like in this demo. In that demo, I used built versions of React and Draft.js which include all the changes necessary to make it work across nested browsing contexts.Fixes #527
Test Plan
All of the unit tests are passing. For manual testing, I’ve focused on verifying the behavior of the editor when dealing with selections. That includes:
I then:
Finally, I manually tested all of those behaviors with:
In order to make it easy to try and test these behaviors, I created installable branches of fbjs and React that include the
lib/
files to make it installable and usable via npm as well as the built files to be usable directly in the browser.Try it out via npm
In your package.json, use:
Try it out in the browser
Include:
Rendering into an iframe
To know more about rendering into an iframe, see this explanation from @ryanseddon. Also, he turned his work into the React Frame Component, which makes it trivial to try out. And to understand use cases and the utility of enabling this functionality, the major benefits are:
window
).