Fix Markdown table paste not working with GitHub code

In Chrome, copying a block of code GitHub includes an HTML table with
both the line number and the line itself. This caused the paste function
to be fooled that a table should be inserted as Markdown instead of
plain text.

Now, we increase the requirements to determine for pasting a table by
comparing the HTML and text data:

1. The number of rows have to match
2. The max number of columns have to match

Note that we use the max number of columns in the tab-delimited text
because it's possible that a spreadsheet omits empty columns.

Closes https://gitlab.com/gitlab-org/gitlab/issues/195479

app/assets/javascripts/behaviors/markdown/paste_markdown_table.js

+const maxColumnWidth = (rows, columnIndex) => Math.max(...rows.map(row => row[columnIndex].length));
+
 export default class PasteMarkdownTable {
   constructor(clipboardData) {
     this.data = clipboardData;
+    this.columnWidths = [];
+    this.rows = [];
+    this.tableFound = this.parseTable();
   }
 
-  static maxColumnWidth(rows, columnIndex) {
-    return Math.max.apply(null, rows.map(row => row[columnIndex].length));
+  isTable() {
+    return this.tableFound;
   }
 
+  convertToTableMarkdown() {
+    this.calculateColumnWidths();
+
+    const markdownRows = this.rows.map(
+      row =>
+        // | Name         | Title | Email Address  |
+        // |--------------|-------|----------------|
+        // | Jane Atler   | CEO   | jane@acme.com  |
+        // | John Doherty | CTO   | john@acme.com  |
+        // | Sally Smith  | CFO   | sally@acme.com |
+        `| ${row.map((column, index) => this.formatColumn(column, index)).join(' | ')} |`,
+    );
+
+    // Insert a header break (e.g. -----) to the second row
+    markdownRows.splice(1, 0, this.generateHeaderBreak());
+
+    return markdownRows.join('\n');
+  }
+
+  // Private methods below
+
   // To determine whether the cut data is a table, the following criteria
   // must be satisfied with the clipboard data:
   //
   // 1. MIME types "text/plain" and "text/html" exist
   // 2. The "text/html" data must have a single <table> element
-  static isTable(data) {
-    const types = new Set(data.types);
-
-    if (!types.has('text/html') || !types.has('text/plain')) {
+  // 3. The number of rows in the "text/plain" data matches that of the "text/html" data
+  // 4. The max number of columns in "text/plain" matches that of the "text/html" data
+  parseTable() {
+    if (!this.data.types.includes('text/html') || !this.data.types.includes('text/plain')) {
       return false;
     }
 
-    const htmlData = data.getData('text/html');
-    const doc = new DOMParser().parseFromString(htmlData, 'text/html');
+    const htmlData = this.data.getData('text/html');
+    this.doc = new DOMParser().parseFromString(htmlData, 'text/html');
+    const tables = this.doc.querySelectorAll('table');
 
     // We're only looking for exactly one table. If there happens to be
     // multiple tables, it's possible an application copied data into
     // the clipboard that is not related to a simple table. It may also be
     // complicated converting multiple tables into Markdown.
-    if (doc.querySelectorAll('table').length === 1) {
-      return true;
+    if (tables.length !== 1) {
+      return false;
     }
 
+    const text = this.data.getData('text/plain').trim();
+    const splitRows = text.split(/[\n\u0085\u2028\u2029]|\r\n?/g);
+
+    // Now check that the number of rows matches between HTML and text
+    if (this.doc.querySelectorAll('tr').length !== splitRows.length) {
       return false;
     }
 
-  convertToTableMarkdown() {
-    const text = this.data.getData('text/plain').trim();
-    this.rows = text.split(/[\n\u0085\u2028\u2029]|\r\n?/g).map(row => row.split('\t'));
+    this.rows = splitRows.map(row => row.split('\t'));
     this.normalizeRows();
-    this.calculateColumnWidths();
 
-    const markdownRows = this.rows.map(
-      row =>
-        // | Name         | Title | Email Address  |
-        // |--------------|-------|----------------|
-        // | Jane Atler   | CEO   | jane@acme.com  |
-        // | John Doherty | CTO   | john@acme.com  |
-        // | Sally Smith  | CFO   | sally@acme.com |
-        `| ${row.map((column, index) => this.formatColumn(column, index)).join(' | ')} |`,
-    );
-
-    // Insert a header break (e.g. -----) to the second row
-    markdownRows.splice(1, 0, this.generateHeaderBreak());
+    // Check that the max number of columns in the HTML matches the number of
+    // columns in the text. GitHub, for example, copies a line number and the
+    // line itself into the HTML data.
+    if (!this.columnCountsMatch()) {
+      return false;
+    }
 
-    return markdownRows.join('\n');
+    return true;
   }
 
   // Ensure each row has the same number of columns
@@ -69,10 +92,21 @@ export default class PasteMarkdownTable {
 
   calculateColumnWidths() {
     this.columnWidths = this.rows[0].map((_column, columnIndex) =>
-      PasteMarkdownTable.maxColumnWidth(this.rows, columnIndex),
+      maxColumnWidth(this.rows, columnIndex),
     );
   }
 
+  columnCountsMatch() {
+    const textColumnCount = this.rows[0].length;
+    let htmlColumnCount = 0;
+
+    this.doc.querySelectorAll('table tr').forEach(row => {
+      htmlColumnCount = Math.max(row.cells.length, htmlColumnCount);
+    });
+
+    return textColumnCount === htmlColumnCount;
+  }
+
   formatColumn(column, index) {
     const spaces = Array(this.columnWidths[index] - column.length + 1).join(' ');
     return column + spaces;

app/assets/javascripts/dropzone_input.js

@@ -176,11 +176,11 @@ export default function dropzoneInput(form) {
     const pasteEvent = event.originalEvent;
     const { clipboardData } = pasteEvent;
     if (clipboardData && clipboardData.items) {
+      const converter = new PasteMarkdownTable(clipboardData);
       // Apple Numbers copies a table as an image, HTML, and text, so
       // we need to check for the presence of a table first.
-      if (PasteMarkdownTable.isTable(clipboardData)) {
+      if (converter.isTable()) {
         event.preventDefault();
-        const converter = new PasteMarkdownTable(clipboardData);
         const text = converter.convertToTableMarkdown();
         pasteText(text);
       } else {

spec/frontend/behaviors/markdown/paste_markdown_table_spec.js

@@ -10,9 +10,9 @@ describe('PasteMarkdownTable', () => {
       value: {
         getData: jest.fn().mockImplementation(type => {
           if (type === 'text/html') {
-            return '<table><tr><td></td></tr></table>';
+            return '<table><tr><td>First</td><td>Second</td></tr></table>';
           }
-          return 'hello world';
+          return 'First\tSecond';
         }),
       },
     });
@@ -24,39 +24,48 @@ describe('PasteMarkdownTable', () => {
     it('return false when no HTML data is provided', () => {
       data.types = ['text/plain'];
 
-      expect(PasteMarkdownTable.isTable(data)).toBe(false);
+      expect(new PasteMarkdownTable(data).isTable()).toBe(false);
     });
 
     it('returns false when no text data is provided', () => {
       data.types = ['text/html'];
 
-      expect(PasteMarkdownTable.isTable(data)).toBe(false);
+      expect(new PasteMarkdownTable(data).isTable()).toBe(false);
     });
 
     it('returns true when a table is provided in both text and HTML', () => {
       data.types = ['text/html', 'text/plain'];
 
-      expect(PasteMarkdownTable.isTable(data)).toBe(true);
+      expect(new PasteMarkdownTable(data).isTable()).toBe(true);
     });
 
     it('returns false when no HTML table is included', () => {
       data.types = ['text/html', 'text/plain'];
       data.getData = jest.fn().mockImplementation(() => 'nothing');
 
-      expect(PasteMarkdownTable.isTable(data)).toBe(false);
-    });
+      expect(new PasteMarkdownTable(data).isTable()).toBe(false);
     });
 
-  describe('convertToTableMarkdown', () => {
-    let converter;
+    it('returns false when the number of rows are not consistent', () => {
+      data.types = ['text/html', 'text/plain'];
+      data.getData = jest.fn().mockImplementation(mimeType => {
+        if (mimeType === 'text/html') {
+          return '<table><tr><td>def test<td></tr></table>';
+        }
+        return "def test\n  'hello'\n";
+      });
 
-    beforeEach(() => {
-      converter = new PasteMarkdownTable(data);
+      expect(new PasteMarkdownTable(data).isTable()).toBe(false);
+    });
   });
 
+  describe('convertToTableMarkdown', () => {
     it('returns a Markdown table', () => {
+      data.types = ['text/html', 'text/plain'];
       data.getData = jest.fn().mockImplementation(type => {
-        if (type === 'text/plain') {
+        if (type === 'text/html') {
+          return '<table><tr><td>First</td><td>Last</td><tr><td>John</td><td>Doe</td><tr><td>Jane</td><td>Doe</td></table>';
+        } else if (type === 'text/plain') {
           return 'First\tLast\nJohn\tDoe\nJane\tDoe';
         }
 
@@ -70,12 +79,18 @@ describe('PasteMarkdownTable', () => {
         '| Jane  | Doe  |',
       ].join('\n');
 
+      const converter = new PasteMarkdownTable(data);
+
+      expect(converter.isTable()).toBe(true);
       expect(converter.convertToTableMarkdown()).toBe(expected);
     });
 
     it('returns a Markdown table with rows normalized', () => {
+      data.types = ['text/html', 'text/plain'];
       data.getData = jest.fn().mockImplementation(type => {
-        if (type === 'text/plain') {
+        if (type === 'text/html') {
+          return '<table><tr><td>First</td><td>Last</td><tr><td>John</td><td>Doe</td><tr><td>Jane</td><td>/td></table>';
+        } else if (type === 'text/plain') {
           return 'First\tLast\nJohn\tDoe\nJane';
         }
 
@@ -89,6 +104,9 @@ describe('PasteMarkdownTable', () => {
         '| Jane  |      |',
       ].join('\n');
 
+      const converter = new PasteMarkdownTable(data);
+
+      expect(converter.isTable()).toBe(true);
       expect(converter.convertToTableMarkdown()).toBe(expected);
     });
   });

spec/javascripts/dropzone_input_spec.js

@@ -39,17 +39,17 @@ describe('dropzone_input', () => {
       const event = $.Event('paste');
       const origEvent = new Event('paste');
       const pasteData = new DataTransfer();
-      pasteData.setData('text/plain', 'hello world');
-      pasteData.setData('text/html', '<table></table>');
+      pasteData.setData('text/plain', 'Hello World');
+      pasteData.setData('text/html', '<table><tr><td>Hello World</td></tr></table>');
       origEvent.clipboardData = pasteData;
       event.originalEvent = origEvent;
 
-      spyOn(PasteMarkdownTable, 'isTable').and.callThrough();
+      spyOn(PasteMarkdownTable.prototype, 'isTable').and.callThrough();
       spyOn(PasteMarkdownTable.prototype, 'convertToTableMarkdown').and.callThrough();
 
       $('.js-gfm-input').trigger(event);
 
-      expect(PasteMarkdownTable.isTable).toHaveBeenCalled();
+      expect(PasteMarkdownTable.prototype.isTable).toHaveBeenCalled();
       expect(PasteMarkdownTable.prototype.convertToTableMarkdown).toHaveBeenCalled();
     });
   });