HTML diff: Maintain spacing in text/tags (#40)

Mr0grog · web-flow · commit bc1df2f1d6d0 · 2020-11-23T23:11:53.000-08:00
It turns out that some of the display breakage we've had with `html_render_diff` is happening because we are nicely reformatting the source on pages that use `white-space: pre;` or similar CSS styling to treat the spacing in the source code as significant. For example, the following will display poorly, but it's what we were outputting:  <div style="white-space: pre;">Some text</div>  <div style="white-space: pre;"></div>  <div style="white-space: pre;"> <del> Some text </del> </div> Solve this by working harder to maintain the original spacing and not unintentionally reformatting the source, so we now output:  <div style="white-space: pre;"><del>Some text</del></div> Fixes #36.
diff --git a/docs/source/release-history.rst b/docs/source/release-history.rst
@@ -7,6 +7,8 @@ In Development
 
 - Fixes an issue where the diffing server could reset the process pool that manages the actual diffs multiple times unnecessarily, leading to wasted memory and CPU. If you are tracking logs and errors, this will also make error messages about the diffing server clearer — you’ll see “BrokenProcessPool” instead of “'NoneType' object does not support item assignment.” (`#38 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/38>`_)
 
+- Fixes :func:`web_monitoring_diff.html_diff_render` to make sure the spacing of text and tags in the HTML source code of the diff matches the original. This resolves display issues on pages where CSS is used to treat spacing as significant. (`#36 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/36>`_)
+
 
 Version 0.1.0
 -------------
diff --git a/web_monitoring_diff/html_render_diff.py b/web_monitoring_diff/html_render_diff.py
@@ -435,18 +435,18 @@ def html_diff_render(a_text, b_text, a_headers=None, b_headers=None,
         soup.head.append(change_styles)
 
         soup.body.replace_with(diff_body)
-        # The method we use above to append HTML strings (the diffs) to the soup
-        # results in a non-navigable soup. So we serialize and re-parse :(
-        # (Note we use no formatter for this because proper encoding escapes
-        # the tags our differ generated.)
-        soup = html5_parser.parse(soup.prettify(formatter=None),
-                                  treebuilder='soup', return_root=False)
         runtime_scripts = soup.new_tag('script', id='wm-diff-script')
         runtime_scripts.string = UPDATE_CONTRAST_SCRIPT
         soup.body.append(runtime_scripts)
         if diff_type == 'combined':
             _deactivate_deleted_active_elements(soup)
-        results[diff_type] = soup.prettify(formatter='minimal')
+        # Convert to a string instead of prettifying. `prettify()` will always
+        # add extra space around non-inline elements, even if `formatter` is
+        # "minimal" or None. This is a problem because the page may use pre-
+        # formatted text or use CSS to make block elements display as inline,
+        # etc. There are lots of situations where the spacing really matters,
+        # so we want to make sure not to alter it.
+        results[diff_type] = str(soup)
 
     return results
 
@@ -506,7 +506,18 @@ def diff_elements(old, new, comparator, include='all'):
     def fill_element(element, diff):
         result_element = copy.copy(element)
         result_element.clear()
-        result_element.append(diff)
+        # At this point, `diff` is an HTML *string*, so we need to parse it
+        # before we can safely insert it into a soup. (We used to insert it as
+        # a string and do some funny tricks, but this led to other issues.)
+        # TODO: _htmldiff() should return a tree of soup tags rather than a
+        # list of strings, so we don't need to re-parse here.
+        parsed_diff = html5_parser.parse(
+            f'<!doctype html>\n<html><body>{diff}</body></html>',
+            treebuilder='soup', return_root=False)
+        # `.contents/.children` are *live*, so cache their output into a new
+        # list before moving them to the new container -- otherwise we'll miss
+        # some because the contents change in the middle of moving.
+        result_element.extend(list(parsed_diff.body.children))
         return result_element
 
     results = {}
diff --git a/web_monitoring_diff/tests/test_html_diff_validity.py b/web_monitoring_diff/tests/test_html_diff_validity.py
@@ -15,6 +15,21 @@
 from web_monitoring_diff.html_render_diff import html_diff_render
 
 
+# Helpers
+HEAD_PATTERN = re.compile(r'<head>(.*?)</head>', re.DOTALL)
+BODY_PATTERN = re.compile(r'<body.*?>(.*)</body>', re.DOTALL)
+EXTRAS_PATTERN = re.compile(r'<(style|script) id="wm-diff-.*?</\1>', re.DOTALL)
+
+def get_head(html):
+    return HEAD_PATTERN.search(html).group(1)
+
+def get_body(html):
+    return BODY_PATTERN.search(html).group(1)
+
+def remove_extras(html):
+    return EXTRAS_PATTERN.sub('', html)
+
+
 # TODO: extend these to other html differs via parameterization, a la
 # `test_html_diff.py`. Most of these are written generically enough they could
 # feasibly work with any visual HTML diff routine.
@@ -26,6 +41,30 @@ def test_html_diff_render_works_on_pages_with_no_head():
     assert result
 
 
+def test_html_diff_render_maintains_existing_content_spacing():
+    """
+    The diff should not add spaces to make the HTML source look nicely
+    formatted, since this can change how the page is laid out and styled -- any
+    space in the doc may be significant if an element has CSS like:
+        white-space: pre;
+    """
+    result = html_diff_render(
+        '<html><body>Hello <div>Alice</div></body></html>',
+        '<html><body>Hello <div>Sally</div></body></html>',
+        include='all')
+
+    # Just look at the body (the head can have nice space) and remove extra
+    # styling/scripting, which can have space inside it because it doesn't
+    # contribute to the content.
+    deletions = remove_extras(get_body(result['deletions']))
+    insertions = remove_extras(get_body(result['insertions']))
+    combined = remove_extras(get_body(result['combined']))
+
+    assert deletions == 'Hello <div><del class="wm-diff">Alice</del></div>'
+    assert insertions == 'Hello <div><ins class="wm-diff">Sally</ins></div>'
+    assert combined == 'Hello <div><del class="wm-diff">Alice</del><ins class="wm-diff">Sally</ins></div>'
+
+
 def test_html_diff_render_does_not_encode_embedded_content():
     html = '<script>console.log("uhoh");</script> ok ' \
            '<style>body {font-family: "arial";}</style>'
