From 12ab332f4d6d339ae257d550247066c34dfd06fb Mon Sep 17 00:00:00 2001
From: Robert DeLuca <robertdeluca19@gmail.com>
Date: Thu, 18 Apr 2019 14:49:14 -0500
Subject: [PATCH] fix: Revert DOM clean up methods (#160)

* Revert DOM clean up methods

Given this JSBin: https://jsbin.com/?html,console,output

When we serialize the CSSOM into the `<style>` tag that the in-memory
styles own and then _remove_ those styles, it triggers `deleteRule` on
those styles in the CSSOM. Which means we're removing the text we
injected AND all of the styles that live in memory. This breaks their
app.

With the input cleaning, when we remove the value or
checked attribute on the element, it's possible those elements lose
their values entirely. For example, when we remove the `checked`
attribute from checkboxes in the customers DOM, this will make the
checkbox _no longer checked_, which is changing the state of their
application. It would be better, in this world without an AST, to
leave the mutations in place. Removing the mutations causes more
issues than it tries to solve.

* Update test cases
---
 src/percy-agent-client/percy-agent.ts         | 11 ++----
 src/percy-agent-client/serialize-cssom.ts     | 19 +---------
 src/percy-agent-client/serialize-input.ts     | 17 ---------
 test/integration/agent-integration.test.ts    | 35 ++++++++++---------
 .../testcases/stabilize-cssom.html            | 13 +++++++
 test/integration/testcases/stabilize-dom.html |  4 ---
 test/percy-agent-client/percy-agent.test.ts   | 16 ---------
 .../serialize-cssom.test.ts                   |  9 -----
 .../serialize-input.test.ts                   | 12 -------
 9 files changed, 35 insertions(+), 101 deletions(-)
 create mode 100644 test/integration/testcases/stabilize-cssom.html

diff --git a/src/percy-agent-client/percy-agent.ts b/src/percy-agent-client/percy-agent.ts
index 0c038d3b..49775dbd 100644
--- a/src/percy-agent-client/percy-agent.ts
+++ b/src/percy-agent-client/percy-agent.ts
@@ -1,8 +1,8 @@
 import Constants from '../services/constants'
 import {ClientOptions} from './client-options'
 import {PercyAgentClient} from './percy-agent-client'
-import {cleanSerializedCssOm, serializeCssOm} from './serialize-cssom'
-import {cleanSerializedInputElements, serializeInputElements} from './serialize-input'
+import {serializeCssOm} from './serialize-cssom'
+import {serializeInputElements} from './serialize-input'
 import {SnapshotOptions} from './snapshot-options'
 
 export default class PercyAgent {
@@ -71,13 +71,6 @@ export default class PercyAgent {
 
     const snapshotString = doctype + domClone.outerHTML
 
-    // Remove all the additions we've made to the original DOM.
-    // Ideally we would make a deep clone of the original DOM at the start of this
-    // method, and operate on that, but this turns out to be hard to do while
-    // retaining CSS OM and input element state. Instead, we carefully remove what we added.
-    cleanSerializedCssOm(documentObject)
-    cleanSerializedInputElements(documentObject)
-
     return snapshotString
   }
 
diff --git a/src/percy-agent-client/serialize-cssom.ts b/src/percy-agent-client/serialize-cssom.ts
index 8d093163..ffd17fc2 100644
--- a/src/percy-agent-client/serialize-cssom.ts
+++ b/src/percy-agent-client/serialize-cssom.ts
@@ -1,5 +1,4 @@
 const DATA_ATTRIBUTE = 'data-percy-cssom-serialized'
-const START_COMMENT = '/* Start of Percy serialized CSSOM */'
 
 // Take all the CSS created in the CSS Object Model (CSSOM), and inject it
 // into the DOM so Percy can render it safely in our browsers.
@@ -18,7 +17,7 @@ export function serializeCssOm(document: HTMLDocument) {
         .call(styleSheet.cssRules)
         .reduce((prev: string, cssRule: CSSRule) => {
           return prev + cssRule.cssText
-        }, `${START_COMMENT}\n`)
+        }, '')
 
       // Append the serialized styles to the styleSheet's ownerNode to minimize
       // the chances of messing up the cascade order.
@@ -29,19 +28,3 @@ export function serializeCssOm(document: HTMLDocument) {
 
   return document
 }
-
-export function cleanSerializedCssOm(document: HTMLDocument) {
-  // IMPORTANT: querySelectorAll(...) will not always work. In particular, in certain
-  // cases with malformed HTML (e.g. a <style> tag inside of another one), some of
-  // the elements we are looking for will not be returned. In that case, we will
-  // leave traces of ourselves in the underlying DOM.
-  const nodes = document.querySelectorAll(`[${DATA_ATTRIBUTE}]`)
-  Array.from(nodes).forEach((node: Element) => {
-    node.removeAttribute(DATA_ATTRIBUTE)
-    const startOfSerialized = node.innerHTML.indexOf(START_COMMENT)
-    if (startOfSerialized < 0) {
-      return
-    }
-    node.innerHTML = node.innerHTML.substring(0, startOfSerialized)
-  })
-}
diff --git a/src/percy-agent-client/serialize-input.ts b/src/percy-agent-client/serialize-input.ts
index 81ea15be..6834c218 100644
--- a/src/percy-agent-client/serialize-input.ts
+++ b/src/percy-agent-client/serialize-input.ts
@@ -32,20 +32,3 @@ export function serializeInputElements(doc: HTMLDocument): HTMLDocument {
 
   return doc
 }
-
-export function cleanSerializedInputElements(doc: HTMLDocument) {
-  doc.querySelectorAll(`[${DATA_ATTRIBUTE_CHECKED}]`).forEach((el: Element) => {
-    el.removeAttribute('checked')
-    el.removeAttribute(DATA_ATTRIBUTE_CHECKED)
-  })
-  doc.querySelectorAll(`[${DATA_ATTRIBUTE_TEXTAREA_INNERTEXT}]`).forEach((el: Element)  => {
-    const originalInnerText: string = el.getAttribute(DATA_ATTRIBUTE_TEXTAREA_INNERTEXT) || ''
-    const textArea = el as HTMLTextAreaElement
-    textArea.innerText = originalInnerText
-    el.removeAttribute(DATA_ATTRIBUTE_TEXTAREA_INNERTEXT)
-  })
-  doc.querySelectorAll(`[${DATA_ATTRIBUTE_VALUE}]`).forEach((el: Element) => {
-    el.removeAttribute('value')
-    el.removeAttribute(DATA_ATTRIBUTE_VALUE)
-  })
-}
diff --git a/test/integration/agent-integration.test.ts b/test/integration/agent-integration.test.ts
index df9289f5..02f07194 100644
--- a/test/integration/agent-integration.test.ts
+++ b/test/integration/agent-integration.test.ts
@@ -67,8 +67,8 @@ describe('Integration test', () => {
       const domSnapshot = await snapshot(page, 'Buildkite snapshot')
       expect(domSnapshot).contains('Buildkite')
     })
-  })
 
+  })
   describe('on local test cases', () => {
     const testCaseDir = `${__dirname}/testcases`
     const PORT = 8000
@@ -96,19 +96,9 @@ describe('Integration test', () => {
         await page.goto(`http://localhost:${PORT}/stabilize-dom.html`)
       })
 
-      it('serializes CSSOM', async () => {
-        await page.evaluate(() => {
-          const jsStyledDiv = document.getElementById('jsStyled') as HTMLInputElement
-          jsStyledDiv.style.background = 'mediumorchid'
-        })
-
-        const domSnapshot = await snapshot(page, 'Serialize CSSOM')
-        expect(domSnapshot).to.contain('data-percy-cssom-serialized')
-        expect(domSnapshot).to.contain('background: mediumorchid')
-      })
-
       it('serializes input elements', async () => {
         await page.type('#testInputText', 'test input value')
+        await page.type('#testTextarea', 'test textarea value')
         await page.click('#testCheckbox')
         await page.click('#testRadioButton')
 
@@ -116,13 +106,26 @@ describe('Integration test', () => {
         expect(domSnapshot).to.contain('test input value')
         expect(domSnapshot).to.contain('type="checkbox" checked')
         expect(domSnapshot).to.contain('type="radio" checked')
+        expect(domSnapshot).to.contain('test textarea value')
       })
+    })
 
-      it('serializes textarea elements', async () => {
-        await page.type('#testTextarea', 'test textarea value')
+    describe('stablizes CSSOM', () => {
+      before(async () => {
+        await page.goto(`http://localhost:${PORT}/stabilize-cssom.html`)
+      })
+
+      it('serializes the CSSOM', async () => {
+        const domSnapshot = await snapshot(page, 'Serialize CSSOM')
+
+        expect(domSnapshot).to.contain('data-percy-cssom-serialized')
+        expect(domSnapshot).to.contain('.box { height: 500px; width: 500px; background-color: green; }')
+
+        // we want to ensure mutiple snapshots are successful
+        const secondDomSnapshot = await snapshot(page, 'Serialize CSSOM twice')
+        expect(secondDomSnapshot).to.contain('data-percy-cssom-serialized')
+        expect(secondDomSnapshot).to.contain('.box { height: 500px; width: 500px; background-color: green; }')
 
-        const domSnapshot = await snapshot(page, 'Serialize textarea elements')
-        expect(domSnapshot).to.contain('test textarea value')
       })
     })
   })
diff --git a/test/integration/testcases/stabilize-cssom.html b/test/integration/testcases/stabilize-cssom.html
new file mode 100644
index 00000000..473b7990
--- /dev/null
+++ b/test/integration/testcases/stabilize-cssom.html
@@ -0,0 +1,13 @@
+<html>
+  <head>
+    <title>CSSOM testing</title>
+    <style type="text/css"></style>
+    <script>
+     let styleSheet = document.styleSheets[0];
+     styleSheet.insertRule('.box { height: 500px; width: 500px; background-color: green; }')
+    </script>
+  </head>
+  <body>
+    <div class="box"></div>
+  </body>
+</html>
diff --git a/test/integration/testcases/stabilize-dom.html b/test/integration/testcases/stabilize-dom.html
index f6cf6a2c..48bf984b 100644
--- a/test/integration/testcases/stabilize-dom.html
+++ b/test/integration/testcases/stabilize-dom.html
@@ -15,9 +15,5 @@
       <textarea id="testTextarea" name="testTextArea" rows="10"></textarea>
     </p>
     <!-- End of test elements for input element serialization. -->
-
-    <!-- Test element for CSSOM serialization. -->
-    <div id="jsStyled">Magically styled ✨</div>
-
   </body>
 </html>
diff --git a/test/percy-agent-client/percy-agent.test.ts b/test/percy-agent-client/percy-agent.test.ts
index 9fcca8b5..9cbee56d 100644
--- a/test/percy-agent-client/percy-agent.test.ts
+++ b/test/percy-agent-client/percy-agent.test.ts
@@ -58,21 +58,5 @@ describe('PercyAgent', () => {
       expect(requestBody.widths).to.eql([320, 1024])
       expect(requestBody.minHeight).to.equal(512)
     })
-
-    it('does not alter the DOM being snapshotted', () => {
-      const originalHTML = htmlWithoutSelector(document, '#mocha')
-
-      subject.snapshot('a snapshot')
-
-      const postSnapshotHTML = htmlWithoutSelector(document, '#mocha')
-      expect(postSnapshotHTML).to.eq(originalHTML)
-      expect(postSnapshotHTML).to.not.contain('data-percy')
-    })
-
-    it('multiple snapshots produce the same result', () => {
-      const firstDOMSnapshot = subject.snapshot('a snapshot')
-      const secondDOMSnapshot = subject.snapshot('a second snapshot')
-      expect(secondDOMSnapshot).to.eq(firstDOMSnapshot)
-    })
   })
 })
diff --git a/test/percy-agent-client/serialize-cssom.test.ts b/test/percy-agent-client/serialize-cssom.test.ts
index 16750925..2cc6d519 100644
--- a/test/percy-agent-client/serialize-cssom.test.ts
+++ b/test/percy-agent-client/serialize-cssom.test.ts
@@ -18,14 +18,5 @@ describe('serializeCssOm', () => {
       const parsedDoc = (new DOMParser()).parseFromString(domSnapshot, 'text/html')
       expect(parsedDoc.getElementById('jsStyled')!.style.background).to.contain('red')
     })
-
-    it('cleans up after itself', () => {
-      subject.snapshot('test snapshot')
-
-      const postSnapshotHTML = htmlWithoutSelector(document, '#mocha')
-
-      expect(postSnapshotHTML).to.not.contain('data-percy-cssom-serialized')
-      expect(postSnapshotHTML).to.not.contain('Start of Percy serialized CSSOM')
-    })
   })
 })
diff --git a/test/percy-agent-client/serialize-input.test.ts b/test/percy-agent-client/serialize-input.test.ts
index 13f4a20f..7bf47afb 100644
--- a/test/percy-agent-client/serialize-input.test.ts
+++ b/test/percy-agent-client/serialize-input.test.ts
@@ -32,16 +32,4 @@ describe('serializeInputElements', () => {
 
     expect(domSnapshot).to.contain('checked')
   })
-
-  it('cleans up after itself', () => {
-    const preSnapshotHTML = htmlWithoutSelector(document, '#mocha')
-
-    subject.snapshot('test snapshot')
-
-    const postSnapshotHTML = htmlWithoutSelector(document, '#mocha')
-
-    expect(postSnapshotHTML).to.eq(preSnapshotHTML)
-    expect(postSnapshotHTML).to.not.contain('data-percy-input-serialized')
-    expect(postSnapshotHTML).to.not.contain('checked')
-  })
 })