From 0b22d713d556ccc4820850099ad6231493b3f7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Afonso?= Date: Fri, 1 Sep 2017 11:37:51 -0700 Subject: [PATCH] Stop setting the url src as the textContent of the img 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 https://github.com/facebook/draft-js/issues/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 --- .../convertFromHTMLToContentBlocks-test.js | 19 +++++++++++++++++++ .../convertFromHTMLToContentBlocks.js | 5 +++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js b/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js index d0d6f6ece1..db7cd1baab 100644 --- a/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js +++ b/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js @@ -15,6 +15,9 @@ jest.disableAutomock(); const convertFromHTMLToContentBlocks = require('convertFromHTMLToContentBlocks'); +const IMAGE_DATA_URL = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///' + + 'yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'; + function testConvertingAdjacentHtmlElementsToContentBlocks( tag: string, ) { @@ -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( + '', + ); + expect(blocks.contentBlocks[0].text).toBe(' '); + }); + + test('img with data protocol should be correctly parsed', function() { + const blocks = convertFromHTMLToContentBlocks( + ``, + ); + expect(blocks.contentBlocks[0].text).toBe(' '); + }); + }); }); diff --git a/src/model/encoding/convertFromHTMLToContentBlocks.js b/src/model/encoding/convertFromHTMLToContentBlocks.js index 2ddafa78ac..d92283d953 100644 --- a/src/model/encoding/convertFromHTMLToContentBlocks.js +++ b/src/model/encoding/convertFromHTMLToContentBlocks.js @@ -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(