Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Pasting image with data URL source #1101

Closed
erykpiast opened this issue Mar 29, 2017 · 5 comments
Closed

Pasting image with data URL source #1101

erykpiast opened this issue Mar 29, 2017 · 5 comments
Assignees
@aadsm @flarnie
Labels
bug good first issue

Comments

@erykpiast
Copy link

erykpiast commented Mar 29, 2017 •
edited
Loading

Do you want to request a feature or report a bug? It's hard to say, I'd say missing feature or not handled case.

What is the current behavior? When image with data: URL is pasted to the editor, that huge, meaningless (for the user) blob of text becomes a content of newly created block. I cannot say it's not expected behavior (it does make sense for regular URLs, usually not so long and much more readable), but for big images (or even not so big, like 800 x 600 JPG) it causes very poor performance because of amount of data to process. 65K long string itself is shared under the hood, so that's not a problem, but CharacterList of such length may be. As result of that, editor freezes for a moment when image is pasted. Look at example here: https://jsfiddle.net/erykpiast/jwsdokwz/

Once I altered this line to node.textContent = '_' (it cannot be empty or white-space only for some reason), there is no slow down experienced.

What is the expected behavior? I'm not sure. There may be naive check for data: URLs, so in such case node text content is set to something like data:image/jpeg;base64, not a full URL. For "regular" URLs behavior may stay as it is now. Do you think it'd be breaking change that cannot be introduced as a patch?

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js? Draft 0.10.0, any browser. It's a "bug" introduced in v0.10 as previous versions didn't care about pasted images.
@hellendag
Copy link

Ha, we definitely shouldn't be treating data as the text content fallback.

You've got the right idea here -- we should use some dummy placeholder in the data case.

A PR would be welcome! :)

@erykpiast
Copy link
Author

erykpiast commented Jun 16, 2017 •
edited
Loading

Hi @hellendag, thank you for your response! I'll make the PR with pleasure.

I just want to discuss some details beforehand. What do you think, what pattern is enough to check: data:* (like url.startsWith('data:')) or something more specific like /^data:\w+\/\w+;*/? I'd say it shouldn't be too specific, on the other hand I can imagine there is thousands of people using the library in many different ways, so it may be one relies on value of the first kind (let's say data:myCustomFormat) and too general pattern will break her code. And well, premature generalisation is just dangerous thing.

The second thing, what should be pasted instead of full URL? I'd say data:MIME_TYPE may be still quite poorly chosen default from UX point of view, I have no better idea though. I guess we'd like to avoid arbitrary strings like "Inline image" (maybe not exactly this, what even "inline" means to non-technical people), even if it may be the most readable and understandable thing we might ever display.

@flarnie flarnie self-assigned this Jul 22, 2017
@flarnie
Copy link
Contributor

flarnie commented Jul 22, 2017

Hi again @erykpiast - thanks for reporting this issue.

tldr; either url.startsWith('data:') or /^data:\w+\/\w+;*/ would be fine, and I'd actually prefer we stop setting text content in the node at all. I would +1 any of those options if you still want to make a PR. :)

More details: I was working with the folks who added img support and the main use case we were considering was an editor that has a decorator implemented for img entity types. Looking at other text editor programs, the only example I found that does not support copy-pasting images is quip, and in that case if you try to paste an image it outputs nothing. You don't get the image url. I think that is the more expected behavior - it seems a bit weird from a UX perspective for the image url to show up.

@tylercraft please chime in if you have strong reasons for preferring we paste the image url. But it seems to me the simplest thing is to skip setting the node text content at all.

@flarnie
Copy link
Contributor

flarnie commented Aug 29, 2017

It sounds like @aadsm might pick this up - heads up in case anyone else was looking at working on it.

@aadsm aadsm self-assigned this Aug 30, 2017
facebook-github-bot pushed a commit that referenced this issue Sep 1, 2017
@aadsm @facebook-github-bot
Stop setting the url src as the textContent of the img
0b22d71 
Summary:
WARNING: This diff introduces a **BREAKING change** from current Draft: when you paste an image it will no longer render the url of the image (it renders nothing).

= Main Issue =
This diff addresses the issue raised on #1101

tl;dr of this issue is that by setting the url src as the textContent it creates a significant delay in parsing data: urls since they can easily be ~60KB. This happens because the content of the img will be parsed by draft and for each character there will be an entity reference and entity style entry.

= Solutions =
In the comments of the issue 2 solutions were proposed:
1) Ignore data: urls from being parsed from HTML.
2) Stop setting the textContent of the img node when parsing HTML.

In this diff I'm going with solution **2)** becase as flarnie points out in her comment, support to parse img tags was added with the decorator usecase in mind (and ignoring data: urls would make a decorator not render in this case), also, as an example, quip doesn't render anything when an image is pasted so we think this behaviour is reasonable.

The main downside of this approach is the introduction of a breaking change in the framework, but I think it's acceptable and I'm not expecting any big issue because of this (please correct me if I'm wrong).

= Implementation =
Ideally I wouldn't set anything on textContent but I'm forced too because otherwise Draft won't create an entity for it. Entity creation is only done if the current node being parsed has children, otherwise skipped: diffusion/E/browse/tfb/trunk/www/html/shared/draft-js/model/encoding/convertFromHTMLToContentBlocks.js;3261725$483.
My current solution is to set it to a single space, looks a bit clowny though.

Is the children restriction something we might want to change? The only use case I can see though is when we want to create entities just for decorators to act upon (which is this case).

= A Tale of Two URI implementations =
While fixing and testing I noticed that on our internal version of Draft it crashes when you try to paste a data url.
I looked into it and found that internally we have a version of URI that has plenty of checks: diffusion/E/browse/tfb/trunk/www/html/shared/core/URIBase.js;3262904$242 and it fails because `data:` is not an allowed protocol: diffusion/E/browse/tfb/trunk/www/html/shared/core/URISchemes.js;3263158$11. While on our public version we have a shim based implementation: https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/__forks__/URI.js.

I'm not expecting pasting data urls to be a common behaviour so not a big deal but we do use URI to check other src attributes in HTML. I recommend to add 'data' to the list of supported protocols. Any reason not to?

Is there any chance we could open source our version of URI? It's tricky testing Draft if we have two different implementations for this.

= Breaking Change =
Any advice on how to deal with this?
The CRMComposer tests explicitly tested the URL rendering, adding tylercraft to the diff to make sure this is fine.

Reviewed By: flarnie

Differential Revision: D5733880

fbshipit-source-id: 2825a083d261b55e469fd73d23257778a13d609e
@flarnie
Copy link
Contributor

flarnie commented Sep 5, 2017

Thanks again for fixing this @aadsm !

@flarnie flarnie closed this as completed Sep 5, 2017
0on pushed a commit to pofigizm/draft-js that referenced this issue Sep 19, 2017
@aadsm @0on
Stop setting the url src as the textContent of the img
f948dcb 
Summary:
WARNING: This diff introduces a **BREAKING change** from current Draft: when you paste an image it will no longer render the url of the image (it renders nothing).

= Main Issue =
This diff addresses the issue raised on facebookarchive#1101

tl;dr of this issue is that by setting the url src as the textContent it creates a significant delay in parsing data: urls since they can easily be ~60KB. This happens because the content of the img will be parsed by draft and for each character there will be an entity reference and entity style entry.

= Solutions =
In the comments of the issue 2 solutions were proposed:
1) Ignore data: urls from being parsed from HTML.
2) Stop setting the textContent of the img node when parsing HTML.

In this diff I'm going with solution **2)** becase as flarnie points out in her comment, support to parse img tags was added with the decorator usecase in mind (and ignoring data: urls would make a decorator not render in this case), also, as an example, quip doesn't render anything when an image is pasted so we think this behaviour is reasonable.

The main downside of this approach is the introduction of a breaking change in the framework, but I think it's acceptable and I'm not expecting any big issue because of this (please correct me if I'm wrong).

= Implementation =
Ideally I wouldn't set anything on textContent but I'm forced too because otherwise Draft won't create an entity for it. Entity creation is only done if the current node being parsed has children, otherwise skipped: diffusion/E/browse/tfb/trunk/www/html/shared/draft-js/model/encoding/convertFromHTMLToContentBlocks.js;3261725$483.
My current solution is to set it to a single space, looks a bit clowny though.

Is the children restriction something we might want to change? The only use case I can see though is when we want to create entities just for decorators to act upon (which is this case).

= A Tale of Two URI implementations =
While fixing and testing I noticed that on our internal version of Draft it crashes when you try to paste a data url.
I looked into it and found that internally we have a version of URI that has plenty of checks: diffusion/E/browse/tfb/trunk/www/html/shared/core/URIBase.js;3262904$242 and it fails because `data:` is not an allowed protocol: diffusion/E/browse/tfb/trunk/www/html/shared/core/URISchemes.js;3263158$11. While on our public version we have a shim based implementation: https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/__forks__/URI.js.

I'm not expecting pasting data urls to be a common behaviour so not a big deal but we do use URI to check other src attributes in HTML. I recommend to add 'data' to the list of supported protocols. Any reason not to?

Is there any chance we could open source our version of URI? It's tricky testing Draft if we have two different implementations for this.

= Breaking Change =
Any advice on how to deal with this?
The CRMComposer tests explicitly tested the URL rendering, adding tylercraft to the diff to make sure this is fine.

Reviewed By: flarnie

Differential Revision: D5733880

fbshipit-source-id: 2825a083d261b55e469fd73d23257778a13d609e
midas19910709 added a commit to midas19910709/draft-js that referenced this issue Mar 30, 2022
@midas19910709
Stop setting the url src as the textContent of the img
0160965 
Summary:
WARNING: This diff introduces a **BREAKING change** from current Draft: when you paste an image it will no longer render the url of the image (it renders nothing).

= Main Issue =
This diff addresses the issue raised on facebookarchive/draft-js#1101

tl;dr of this issue is that by setting the url src as the textContent it creates a significant delay in parsing data: urls since they can easily be ~60KB. This happens because the content of the img will be parsed by draft and for each character there will be an entity reference and entity style entry.

= Solutions =
In the comments of the issue 2 solutions were proposed:
1) Ignore data: urls from being parsed from HTML.
2) Stop setting the textContent of the img node when parsing HTML.

In this diff I'm going with solution **2)** becase as flarnie points out in her comment, support to parse img tags was added with the decorator usecase in mind (and ignoring data: urls would make a decorator not render in this case), also, as an example, quip doesn't render anything when an image is pasted so we think this behaviour is reasonable.

The main downside of this approach is the introduction of a breaking change in the framework, but I think it's acceptable and I'm not expecting any big issue because of this (please correct me if I'm wrong).

= Implementation =
Ideally I wouldn't set anything on textContent but I'm forced too because otherwise Draft won't create an entity for it. Entity creation is only done if the current node being parsed has children, otherwise skipped: diffusion/E/browse/tfb/trunk/www/html/shared/draft-js/model/encoding/convertFromHTMLToContentBlocks.js;3261725$483.
My current solution is to set it to a single space, looks a bit clowny though.

Is the children restriction something we might want to change? The only use case I can see though is when we want to create entities just for decorators to act upon (which is this case).

= A Tale of Two URI implementations =
While fixing and testing I noticed that on our internal version of Draft it crashes when you try to paste a data url.
I looked into it and found that internally we have a version of URI that has plenty of checks: diffusion/E/browse/tfb/trunk/www/html/shared/core/URIBase.js;3262904$242 and it fails because `data:` is not an allowed protocol: diffusion/E/browse/tfb/trunk/www/html/shared/core/URISchemes.js;3263158$11. While on our public version we have a shim based implementation: https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/__forks__/URI.js.

I'm not expecting pasting data urls to be a common behaviour so not a big deal but we do use URI to check other src attributes in HTML. I recommend to add 'data' to the list of supported protocols. Any reason not to?

Is there any chance we could open source our version of URI? It's tricky testing Draft if we have two different implementations for this.

= Breaking Change =
Any advice on how to deal with this?
The CRMComposer tests explicitly tested the URL rendering, adding tylercraft to the diff to make sure this is fine.

Reviewed By: flarnie

Differential Revision: D5733880

fbshipit-source-id: 2825a083d261b55e469fd73d23257778a13d609e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@aadsm aadsm

@flarnie flarnie

Labels
bug good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@aadsm @flarnie @erykpiast @hellendag