From ad8374d278507ae73dceffd9bbbcede398f849e3 Mon Sep 17 00:00:00 2001
From: Frank Thompson <frankthompson@fb.com>
Date: Mon, 13 Jan 2020 12:54:21 -0800
Subject: [PATCH] fix style handling in convertFromHTMLToContentBlocks

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
---
 ...onvertFromHTMLToContentBlocks-test.js.snap | 130 ++++++++++++++++++
 .../convertFromHTMLToContentBlocks-test.js    |  14 ++
 .../convertFromHTMLToContentBlocks.js         | 105 +++++++-------
 .../DraftPasteProcessor-test.js.snap          |  40 ++----
 4 files changed, 208 insertions(+), 81 deletions(-)

diff --git a/src/model/encoding/__tests__/__snapshots__/convertFromHTMLToContentBlocks-test.js.snap b/src/model/encoding/__tests__/__snapshots__/convertFromHTMLToContentBlocks-test.js.snap
index 4363b4152f..b1540ac35c 100644
--- a/src/model/encoding/__tests__/__snapshots__/convertFromHTMLToContentBlocks-test.js.snap
+++ b/src/model/encoding/__tests__/__snapshots__/convertFromHTMLToContentBlocks-test.js.snap
@@ -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 {
diff --git a/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js b/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js
index 4ee5e7099e..95b14118a1 100644
--- a/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js
+++ b/src/model/encoding/__tests__/convertFromHTMLToContentBlocks-test.js
@@ -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>
diff --git a/src/model/encoding/convertFromHTMLToContentBlocks.js b/src/model/encoding/convertFromHTMLToContentBlocks.js
index 1e93d15197..bc16d72428 100644
--- a/src/model/encoding/convertFromHTMLToContentBlocks.js
+++ b/src/model/encoding/convertFromHTMLToContentBlocks.js
@@ -48,7 +48,15 @@ const REGEX_ZWS = new RegExp('&#8203;?', 'g');
 
 // https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight
 const boldValues = ['bold', 'bolder', '500', '600', '700', '800', '900'];
-const notBoldValues = ['light', 'lighter', '100', '200', '300', '400'];
+const notBoldValues = [
+  'light',
+  'lighter',
+  'normal',
+  '100',
+  '200',
+  '300',
+  '400',
+];
 
 const anchorAttr = ['className', 'href', 'rel', 'target', 'title'];
 const imgAttr = ['alt', 'className', 'height', 'src', 'width'];
@@ -169,9 +177,10 @@ const isValidImage = (node: Node): boolean => {
  * Try to guess the inline style of an HTML element based on its css
  * styles (font-weight, font-style and text-decoration).
  */
-const styleFromNodeAttributes = (node: Node): DraftInlineStyle => {
-  const style = OrderedSet();
-
+const styleFromNodeAttributes = (
+  node: Node,
+  style: DraftInlineStyle,
+): DraftInlineStyle => {
   if (!isHTMLElement(node)) {
     return style;
   }
@@ -262,7 +271,6 @@ class ContentBlocksBuilder {
   currentBlockType: string = 'unstyled';
   currentDepth: number = 0;
   currentEntity: ?string = null;
-  currentStyle: DraftInlineStyle = OrderedSet();
   currentText: string = '';
   wrapper: ?string = null;
 
@@ -297,7 +305,6 @@ class ContentBlocksBuilder {
     this.currentBlockType = 'unstyled';
     this.currentDepth = 0;
     this.currentEntity = null;
-    this.currentStyle = OrderedSet();
     this.currentText = '';
     this.entityMap = DraftEntity;
     this.wrapper = null;
@@ -311,7 +318,7 @@ class ContentBlocksBuilder {
     this.contentBlocks = [];
     this.currentDepth = 0;
     // Converts the HTML node to block config
-    this.blockConfigs.push(...this._toBlockConfigs([node]));
+    this.blockConfigs.push(...this._toBlockConfigs([node], OrderedSet()));
 
     // There might be some left over text in the builder's
     // internal state, if so make a ContentBlock out of it.
@@ -345,20 +352,6 @@ class ContentBlocksBuilder {
     };
   }
 
-  /**
-   * Add a new inline style to the upcoming nodes.
-   */
-  addStyle(inlineStyle: DraftInlineStyle): void {
-    this.currentStyle = this.currentStyle.union(inlineStyle);
-  }
-
-  /**
-   * Remove a currently applied inline style.
-   */
-  removeStyle(inlineStyle: DraftInlineStyle): void {
-    this.currentStyle = this.currentStyle.subtract(inlineStyle);
-  }
-
   // ***********************************WARNING******************************
   // The methods below this line are private - don't call them directly.
 
@@ -392,7 +385,10 @@ class ContentBlocksBuilder {
    * block configs. Some text content may be left in the builders internal
    * state to enable chaining sucessive calls.
    */
-  _toBlockConfigs(nodes: Array<Node>): Array<ContentBlockConfig> {
+  _toBlockConfigs(
+    nodes: Array<Node>,
+    style: DraftInlineStyle,
+  ): Array<ContentBlockConfig> {
     const blockConfigs = [];
     for (let i = 0; i < nodes.length; i++) {
       const node = nodes[i];
@@ -416,7 +412,9 @@ class ContentBlocksBuilder {
             this.currentDepth++;
           }
         }
-        blockConfigs.push(...this._toBlockConfigs(Array.from(node.childNodes)));
+        blockConfigs.push(
+          ...this._toBlockConfigs(Array.from(node.childNodes), style),
+        );
         this.currentDepth = wasCurrentDepth;
         this.wrapper = wasWrapper;
         continue;
@@ -453,7 +451,10 @@ class ContentBlocksBuilder {
         }
 
         const key = generateRandomKey();
-        const childConfigs = this._toBlockConfigs(Array.from(node.childNodes));
+        const childConfigs = this._toBlockConfigs(
+          Array.from(node.childNodes),
+          style,
+        );
         this._trimCurrentText();
         blockConfigs.push(
           this._makeBlockConfig({
@@ -469,37 +470,33 @@ class ContentBlocksBuilder {
       }
 
       if (nodeName === '#text') {
-        this._addTextNode(node);
+        this._addTextNode(node, style);
         continue;
       }
 
       if (nodeName === 'br') {
-        this._addBreakNode(node);
+        this._addBreakNode(node, style);
         continue;
       }
 
       if (isValidImage(node)) {
-        this._addImgNode(node);
+        this._addImgNode(node, style);
         continue;
       }
 
       if (isValidAnchor(node)) {
-        this._addAnchorNode(node, blockConfigs);
+        this._addAnchorNode(node, blockConfigs, style);
         continue;
       }
 
-      const inlineStyle = HTMLTagToRawInlineStyleMap.has(nodeName)
-        ? OrderedSet.of(HTMLTagToRawInlineStyleMap.get(nodeName))
-        : OrderedSet();
-      const attributesStyle = styleFromNodeAttributes(node);
-
-      this.addStyle(inlineStyle);
-      this.addStyle(attributesStyle);
-
-      blockConfigs.push(...this._toBlockConfigs(Array.from(node.childNodes)));
-
-      this.removeStyle(attributesStyle);
-      this.removeStyle(inlineStyle);
+      let newStyle = style;
+      if (HTMLTagToRawInlineStyleMap.has(nodeName)) {
+        newStyle = newStyle.add(HTMLTagToRawInlineStyleMap.get(nodeName));
+      }
+      newStyle = styleFromNodeAttributes(node, newStyle);
+      blockConfigs.push(
+        ...this._toBlockConfigs(Array.from(node.childNodes), newStyle),
+      );
     }
 
     return blockConfigs;
@@ -508,10 +505,10 @@ class ContentBlocksBuilder {
   /**
    * Append a string of text to the internal buffer.
    */
-  _appendText(text: string) {
+  _appendText(text: string, style: DraftInlineStyle) {
     this.currentText += text;
     const characterMetadata = CharacterMetadata.create({
-      style: this.currentStyle,
+      style,
       entity: this.currentEntity,
     });
     this.characterList = this.characterList.push(
@@ -550,7 +547,7 @@ class ContentBlocksBuilder {
   /**
    * Add the content of an HTML text node to the internal state
    */
-  _addTextNode(node: Node) {
+  _addTextNode(node: Node, style: DraftInlineStyle) {
     let text = node.textContent;
     const trimmedText = text.trim();
 
@@ -568,20 +565,20 @@ class ContentBlocksBuilder {
       text = text.replace(REGEX_LF, SPACE);
     }
 
-    this._appendText(text);
+    this._appendText(text, style);
   }
 
-  _addBreakNode(node: Node) {
+  _addBreakNode(node: Node, style: DraftInlineStyle) {
     if (!isHTMLBRElement(node)) {
       return;
     }
-    this._appendText('\n');
+    this._appendText('\n', style);
   }
 
   /**
    * Add the content of an HTML img node to the internal state
    */
-  _addImgNode(node: Node) {
+  _addImgNode(node: Node, style: DraftInlineStyle) {
     if (!isHTMLImageElement(node)) {
       return;
     }
@@ -607,10 +604,10 @@ class ContentBlocksBuilder {
     // See https://github.com/facebook/draft-js/issues/231 for some context.
     if (gkx('draftjs_fix_paste_for_img')) {
       if (image.getAttribute('role') !== 'presentation') {
-        this._appendText('\ud83d\udcf7');
+        this._appendText('\ud83d\udcf7', style);
       }
     } else {
-      this._appendText('\ud83d\udcf7');
+      this._appendText('\ud83d\udcf7', style);
     }
 
     this.currentEntity = null;
@@ -621,7 +618,11 @@ class ContentBlocksBuilder {
    * (if any) are converted to Block Configs and appended to the provided
    * blockConfig array.
    */
-  _addAnchorNode(node: Node, blockConfigs: Array<ContentBlockConfig>) {
+  _addAnchorNode(
+    node: Node,
+    blockConfigs: Array<ContentBlockConfig>,
+    style: DraftInlineStyle,
+  ) {
     // The check has already been made by isValidAnchor but
     // we have to do it again to keep flow happy.
     if (!isHTMLAnchorElement(node)) {
@@ -645,7 +646,9 @@ class ContentBlocksBuilder {
       entityConfig || {},
     );
 
-    blockConfigs.push(...this._toBlockConfigs(Array.from(node.childNodes)));
+    blockConfigs.push(
+      ...this._toBlockConfigs(Array.from(node.childNodes), style),
+    );
     this.currentEntity = null;
   }
 
diff --git a/src/model/paste/__tests__/__snapshots__/DraftPasteProcessor-test.js.snap b/src/model/paste/__tests__/__snapshots__/DraftPasteProcessor-test.js.snap
index 2fdbd78e12..8cc186d90a 100644
--- a/src/model/paste/__tests__/__snapshots__/DraftPasteProcessor-test.js.snap
+++ b/src/model/paste/__tests__/__snapshots__/DraftPasteProcessor-test.js.snap
@@ -36,33 +36,23 @@ Array [
       },
       Object {
         "entity": null,
-        "style": Array [
-          "BOLD",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "BOLD",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "BOLD",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "BOLD",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "BOLD",
-        ],
+        "style": Array [],
       },
     ],
     "data": Object {},
@@ -110,33 +100,23 @@ Array [
       },
       Object {
         "entity": null,
-        "style": Array [
-          "ITALIC",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "ITALIC",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "ITALIC",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "ITALIC",
-        ],
+        "style": Array [],
       },
       Object {
         "entity": null,
-        "style": Array [
-          "ITALIC",
-        ],
+        "style": Array [],
       },
     ],
     "data": Object {},