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

Commit

Permalink
fix style handling in convertFromHTMLToContentBlocks
Browse files Browse the repository at this point in the history
Summary:
# The Problem

`convertFromHTMLToContentBlocks` currently mishandles styles pretty significantly. It has a `currentStyle` attribute that is mutated as the HTML is processed. When processing a node, it determines what styles that node should have (by unioning styes inferred from the tag and styles inferred from the style attribute) and adds them to `currentStyle`. After it's done processing the node, it removes whatever it added.

This has several problems:
## Styles are *never* removed before rendering the children of a node
For example, if you have the following content:
```
<span style="font-weight: bold">
  <span style="font-weight: normal">
    Not bold!
  </span>
</span>
```
then the "Not bold!" text will actually be given the `'BOLD'` style.

## Styles are sometimes removed when they shouldn't be
For example, if you have the following content:
```
<span style="font-weight: bold">
  <span style="font-weight: bold">
    Bold!
  </span>
  <span>
    Still bold!
  </span>
</span>
```
then the "Still bold!" text will *not* be given the `'BOLD'` style.

## Styles from tag override styles from the style attribute

For example, if you have the following content:
```
<b style="font-weight: normal">Not bold!</b>
```
then "Not bold!" will be given the `'BOLD'` style. (In all browsers I've tested, this content would render as the normal font weight, so I'm assuming this should not have the `'BOLD'` style.)

# The Fix
Using a single `OrderedSet` of styles that is mutated as you process nodes can't fix the second issue mentioned above, because it simply does not store enough info to be able to know what styles it should be using at any given time.

So instead of using this attribute, I'm adding a `style` param to `_toBlockConfigs`. When a node has some styling applied to it, we just make a copy of the style object (implicitly, since it's an immutable `OrderedSet`) with the appropriate styles added or removed. When we're done processing the children of that node, we don't need to do any cleanup because the updated style object is simply thrown away.

In addition, we add styles from looking at tags before adding/removing styles based on the actual style attribute. This allows `<b style="font-weight: normal">Not bold!</b>` to actually render as non-bold text.

Reviewed By: mrkev

Differential Revision: D19295374

fbshipit-source-id: 0ba040a9a19de36d716a4fcd6f55bf57077f001e
  • Loading branch information
Frank Thompson authored and facebook-github-bot committed Jan 13, 2020
1 parent e711230 commit ad8374d
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 81 deletions.
Expand Up @@ -1009,6 +1009,136 @@ Array [
]
`;

exports[`Should properly handle nested attribute styles 1`] = `
Array [
Object {
"characterList": Array [
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
Object {
"entity": null,
"style": Array [
"BOLD",
],
},
],
"data": Object {},
"depth": 0,
"key": "key0",
"text": "boldnot boldbold again",
"type": "unstyled",
},
]
`;

exports[`Should recognized and *not* override html structure when having known draft-js classname with nesting enabled 1`] = `
Array [
Object {
Expand Down
Expand Up @@ -436,6 +436,20 @@ test('Should scope attribute styles', () => {
});
});

test('Should properly handle nested attribute styles', () => {
const html_string = [
'<span style="font-weight: bold">',
'<span>bold</span>',
'<span style="font-weight: normal">not bold</span>',
'<span>bold again</span>',
'</span>',
].join('');

assertConvertFromHTMLToContentBlocks(html_string, {
experimentalTreeDataSupport: false,
});
});

test('Should recognized list deep nesting', () => {
const html_string = `
<ul>
Expand Down

0 comments on commit ad8374d

Please sign in to comment.