Skip to content

Link preview: do not add elements inside headers #592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 3, 2025
Merged

Link preview: do not add elements inside headers #592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
deb7c20
Link preview: do not add elements inside headers
humitos Jun 2, 2025
a2e3bb5
Always add the tooltips before closing `body`
humitos Jun 2, 2025
4074bab
Remove all .tooltips after running each test
humitos Jun 3, 2025
172cf77
Format
humitos Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/linkpreviews.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ function setupTooltip(el, doctoolname, doctoolversion, selector) {
);
newTooltip.setAttribute(TOOLTIP_DATA_HREF, anchorElement.href);
newTooltip.classList.add("tooltip");
anchorElement.insertAdjacentElement("afterend", newTooltip);
document.body.insertAdjacentElement("beforeend", newTooltip);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering what effect this might have. I assumed looking at this code that the element was inserted adjacently for positioning purposes. For example, if the tooltip is positioned relatively, measuring from the document body would be different than the measurement from a sibling element to the tooltip trigger element.

This might be noticeable if the link was inside an element that had a large margin-left for example, as the document body would not have this.

But as far as I can tell this doesn't look like it should be the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positioning or similar shouldn't be an issue because it's based on the original anchor element: https://github.com/readthedocs/addons/blob/humitos%2Flp-headers/src/linkpreviews.js#L125-L155

As far as I can tell, all the tooltip previews I've tested are well positioned 👍🏼

// Let's add event listeners on the tooltip as well, to prevent hiding, when
// mouse moves away from the anchor element
newTooltip.addEventListener("mouseenter", cancelHideDelay);
Expand Down
2 changes: 2 additions & 0 deletions tests/linkpreviews.test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
afterEach(() => {
// Restore the fake server to its original state
server.restore();

document.querySelectorAll("div.tooltip").forEach((el) => el.remove());
});

describe("Link previews tests", () => {
Expand Down