Passing ?readthedocs-diff=true breaks Links Preview

[humitos]

Steps to reproduce:

1. Go to https://docs--11751.org.readthedocs.build/en/11751/user-defined-redirects.html?readthedocs-diff=true
2. Hover on "custom domains" under "Root URL redirect at /" section
3. No preview is shown, and no error is reported

If you go to https://docs--11751.org.readthedocs.build/en/11751/user-defined-redirects.html (without readthedocs-diff), links preview works properly.

[agjohnson]

Noting that passing around the URL param is not necessary either. The diff links could instead:

• Intercept the click event
• Save to local storge or session/etc that doc diff is enabled
• Navigate to original link
• Consume local storage/session/cookie etc

This would avoid passing around the url param. Maybe something to explore here instead.

[ericholscher]

This doesn't look like an issue with the GET arg, it's just docdiff in general? It's replacing the page content, so it's likely the new a elements don't have the events on them.

I don't know if we can really expect previews to work while doing a docdiff. If we want them to continue working, we probably need some way to retrigger the link selector logic after doing the replacement of the page content.

Looking at MDN, it seems we need a live nodelist, instead of a static one: https://developer.mozilla.org/en-US/docs/Web/API/NodeList#live_vs._static_nodelists

[humitos]

This doesn't look like an issue with the GET arg, it's just docdiff in general? It's replacing the page content, so it's likely the new a elements don't have the events on them.

Good point. However, the a node seems to be the one we modified already since it has the class link-preview on it and it shows the dot underlined

The problem seems to be the JavaScript event attached to each of these nodes being deleted somehow.

[humitos]

Probably, a first step here would be to disable links preview when the docdiff is enabled? At least, that will remove the confusion of having these elements with dot underline where I'm expecting to trigger links preview.

I'm not sure if that will add the confusion of "Why links preview is not enabled in this project now?" 🥲

[humitos]

Looking at MDN, it seems we need a live nodelist, instead of a static one: developer.mozilla.org/en-US/docs/Web/API/NodeList#live_vs._static_nodelists

I'm not sure to follow how this would help us here 🤔

[humitos]

If we want them to continue working, we probably need some way to retrigger the link selector logic after doing the replacement of the page content.

I suppose we will want DocDiff to trigger READTHEDOCS_DOCUMENT_BODY_CHANGED or similar, so other addons can subscribe to it and update themselves. In the case of links preview, re-run the setupTooltip function.

This will be similar to what we are implementing for SPA in #157. I think these issues are related, so we may want to wait for that other issue to set the pattern here and we can use it here.

[humitos]

This would avoid passing around the url param. Maybe something to explore here instead.

I found another issue when passing the URL param: #461. I opened #462 to fix it for now and to start thinking about the idea anything external that enables/disables an add should be done via an event, so all the other addons that needs to update an internal state can do it.