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

Commit

Permalink
Stop setting the url src as the textContent of the img
Browse files Browse the repository at this point in the history
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
  • Loading branch information
aadsm authored and facebook-github-bot committed Sep 1, 2017
1 parent 0a89522 commit 0b22d71
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ jest.disableAutomock();

const convertFromHTMLToContentBlocks = require('convertFromHTMLToContentBlocks');

const IMAGE_DATA_URL = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///' +
'yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';

function testConvertingAdjacentHtmlElementsToContentBlocks(
tag: string,
) {
Expand Down Expand Up @@ -45,4 +48,20 @@ describe('convertFromHTMLToContentBlocks', () => {
'p',
'pre',
].forEach(tag => testConvertingAdjacentHtmlElementsToContentBlocks(tag));

describe('img tag', function() {
test('img with http protocol should have empty content', function() {
const blocks = convertFromHTMLToContentBlocks(
'<img src="http://www.facebook.com">',
);
expect(blocks.contentBlocks[0].text).toBe(' ');
});

test('img with data protocol should be correctly parsed', function() {
const blocks = convertFromHTMLToContentBlocks(
`<img src="${IMAGE_DATA_URL}">`,
);
expect(blocks.contentBlocks[0].text).toBe(' ');
});
});
});
5 changes: 3 additions & 2 deletions src/model/encoding/convertFromHTMLToContentBlocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,9 @@ function genFragment(
entityConfig[attr] = imageAttribute;
}
});
const imageURI = new URI(entityConfig.src).toString();
node.textContent = imageURI; // Output src if no decorator
// Forcing this node to have children because otherwise no entity will be
// created for this node.
node.textContent = ' ';

// TODO: update this when we remove DraftEntity entirely
inEntity = DraftEntity.__create(
Expand Down

0 comments on commit 0b22d71

Please sign in to comment.