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

convertFromHTML breaks after converting \n string #1822

Closed
yury-sannikov opened this issue Jul 25, 2018 · 4 comments
Closed

convertFromHTML breaks after converting \n string #1822

yury-sannikov opened this issue Jul 25, 2018 · 4 comments

Comments

@yury-sannikov
Copy link
Contributor

yury-sannikov commented Jul 25, 2018

Do you want to request a feature or report a bug?
I want to report a bug

What is the current behavior?
After a call to the convertFromHTML('\n') another call to the convertFromHTML, for instance convertFromHTML('<h1>H</h1>') return unstyled block type.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
https://jsfiddle.net/m6z0xn4r/1190/

What is the expected behavior?
Call to the convertFromHTML should be idempotent.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?
0.10.5 (latest)

@yury-sannikov
Copy link
Contributor Author

yury-sannikov commented Jul 25, 2018

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. :-(

@yury-sannikov
Copy link
Contributor Author

PR has been created: #1823

@MikeKoval
Copy link

I also faced the same issue, thank you @yury-sannikov

facebook-github-bot pushed a commit that referenced this issue Sep 19, 2019
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
@claudiopro
Copy link
Contributor

Issue already fixed with switch to the new HTML converter, test case was added with #1823

jdecked pushed a commit to twitter-forks/draft-js that referenced this issue Oct 9, 2019
…ive#1822 (facebookarchive#1823)

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: facebookarchive#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
mmissey pushed a commit to mmissey/draft-js that referenced this issue Mar 24, 2020
…ive#1822 (facebookarchive#1823)

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: facebookarchive#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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants