search

readthedocs / addons

JavaScript client to integrate with Read the Docs nicely
https://readthedocs-addons.readthedocs.io/
MIT License
20 stars 6 forks source link

docdiff: highlights unchanged tables, not actual changes #348

Open stevepiercy opened 3 months ago

stevepiercy commented 3 months ago

Example PR preview: https://plone6--1679.org.readthedocs.build/install/containers/images/backend.html#arbitrary-user-and-persistent-data

When I use d to toggle between docdiff views, the changes from the PR do not appear as a diff, but instead all tables in the page display diff highlighting even though there were no changes to them.

humitos commented 3 months ago

Thanks for reporting this issue. I've seen this problem when using docdiff on pages with tables, but I think I never created the issue and then forgot about this 😅

Since the content for the original version could change over time, I created a small GIF to visually document the issue:

Peek 2024-07-18 11-47

I also downloaded both of the versions for that file, and it's clear there is no changes in the tables at all:

$ diff -u original.html pr.html 
--- original.html   2024-07-18 03:22:43.000000000 +0200
+++ pr.html 2024-07-17 12:42:39.000000000 +0200
@@ -80,7 +80,7 @@
 </script>
 <!-- End Matomo Tag Manager -->

-  <script async type="text/javascript" src="/_/static/javascript/readthedocs-addons.js"></script><meta name="readthedocs-project-slug" content="plone6" /><meta name="readthedocs-version-slug" content="6.0" /><meta name="readthedocs-resolver-filename" content="/install/containers/images/backend.html" /></head>
+  <script async type="text/javascript" src="/_/static/javascript/readthedocs-addons.js"></script><meta name="readthedocs-project-slug" content="plone6" /><meta name="readthedocs-version-slug" content="1679" /><meta name="readthedocs-resolver-filename" content="/install/containers/images/backend.html" /></head>
   <body data-spy="scroll" data-target="#bd-toc-nav" data-offset="100">
 <!-- Checkboxes to toggle the left sidebar -->
 <input type="checkbox" class="sidebar-toggle" name="__navigation" id="__navigation" aria-label="Toggle navigation sidebar">
@@ -519,7 +519,7 @@
      </li>
      <li class="toctree-l3">
       <a class="reference internal" href="../../../volto/configuration/volto-config-js.html">
-       Programmatically define the active add-ons and theme
+       Programatically define the active add-ons and theme
       </a>
      </li>
      <li class="toctree-l3 has-children">

I suppose the underlying JavaScript library we use is performing in a weird way here.

humitos commented 3 months ago

It seems this issue may be related to https://github.com/Teamwork/visual-dom-diff/issues/7

agjohnson commented 3 months ago

I've noticed this happening on sites using sphinx-rtd-theme, and I believe it's because the theme dynamically adds a class to tables after loading -- this is enough for the DOM node to be considered different. This is my theory though, I haven't tested this.

humitos commented 3 months ago

I commented on the upstream issue with an example and it seems it's not the case you are describing. There is something else going on here

agjohnson commented 3 months ago

Also, note: though this library worked as an initial proof of concept, it was clear from the start that we might have to take on some maintenance of the library to make it work like we need -- it hasn't seen any commits in a years now. Unfortunately, it's Typescript, which will be an added hurdle to this.

This library and a couple of the small bugs like this are one of the reasons I've been hesitant to promote docdiff just yet. It's close enough that it works for many cases at least, but it could work really nice with some more polish.