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

Commit

Permalink
convertFromHTML breaks after converting \n string, issue #1822 (#1823)
Browse files Browse the repository at this point in the history
Summary:
This diff is commandeering a PR based on a previous baseline to rebase it against latest master. This diff discards the original fix which is no longer applicable, having the affected HTML converter been removed in favor of the new implementation by nicolasc. I am still keeping the test case as a guard against regression.

**Summary**
The core issue is here:
https://github.com/facebook/draft-js/blob/master/src/model/encoding/convertFromHTMLToContentBlocks.js#L376

A call to the `let chunk = {...EMPTY_CHUNK};` create a shallow copy of the `EMPTY_CHUNK` object. It means that `chunk.blocks` array (and others) share the same reference to the `EMPTY_CHUNK.blocks`.

If code hit this line https://github.com/facebook/draft-js/blob/master/src/model/encoding/convertFromHTMLToContentBlocks.js#L633 `EMPTY_CHUNK` object will end up having `block` array with one element of `{type: 'unstyled', depth: 0}` which caused the aforementioned issue.

Next call to the `convertFromHTML` will start with polluted `EMPTY_CHUNK` object. :-(

**Test Plan**
Open a https://jsfiddle.net/m6z0xn4r/1190/
Put a breakpoint on `convertFromHTML` and see 2 different results while converting headers

Another way:
Open chrome dev tools and:
- execute `convertFromHTML('\n')`
- execute `convertFromHTML('<h1>H</h1>')`
Observe that second call return a block with unstyled type.
Pull Request resolved: #1823

Test Plan: Adds a regression test case, run with `jest --watch`

Reviewed By: gmertk

Differential Revision: D17476663

Pulled By: claudiopro

fbshipit-source-id: 05da4d53114aa0d09de61ce55c27f5166b021138
  • Loading branch information
yury-sannikov authored and facebook-github-bot committed Sep 19, 2019
1 parent b858f43 commit 9246cc7
Showing 1 changed file with 16 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,22 @@ test('Should not create empty container blocks around ol and their list items wh
});
});

// Regression test for issue https://github.com/facebook/draft-js/issues/1822
test('Should convert heading block after converting new line string', () => {
// Convert an HTML string containing a newline
// This was previously altering the module's internal state
convertFromHTML('a\n');
// Convert again, and assert this is not affected by the previous conversion
const contentBlocks = convertFromHTML('<h1>heading</h1>')?.contentBlocks;
expect(contentBlocks?.length).toBe(1);
const contentBlock = contentBlocks?.[0];
// #FIXME: Flow does not yet support method or property calls in optional chains.
if (contentBlock != null) {
expect(contentBlock.getType()).toBe('header-one');
expect(contentBlock.getText()).toBe('heading');
}
});

test('Should preserve entities for whitespace-only content', () => {
const html_string = `
<a href="http://www.facebook.com">
Expand Down

0 comments on commit 9246cc7

Please sign in to comment.