Skip to content
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.

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

Tooltips: sphinx-hoverxref popup implementation #239

Closed
agjohnson opened this issue Feb 1, 2024 · 12 comments · Fixed by #379, readthedocs/ext-theme#521 or readthedocs/readthedocs.org#11746
Assignees
Labels
Accepted Accepted issue on our roadmap Feature New feature

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Feb 1, 2024

I couldn't find an issue where we talked about this specifically, but it has come up in a few calls.

The problem we have currently with hoverxref is that the popup library implementation needs to be replaced as we port it to an addon. I believe the issue here was that the library was a jQuery library? @humitos is this correct?

Options we should weigh:

  • Load jQuery and library conditionally, via dynamic webpack import. We still use jQuery in this case, but only when the documentation project uses hoverxref
  • We replace the library with another implementation
  • We rely on upcoming HTML features that aren't yet fully standard 1

Footnotes

  1. The HTML popover attribute is upcoming, and is currently part of Interop 2024. Firefox is the outlier here currently, though there is an implementation in nightly. A hybrid approach would be to do a polyfill or something similar for unsupported browsers.

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Feb 1, 2024
@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Feb 1, 2024
@humitos
Copy link
Member

humitos commented Feb 5, 2024

I believe the issue here was that the library was a jQuery library? @humitos is this correct?

Yes, and it's because it's not maintained anymore. The last commit was 4 years ago: https://github.com/calebjacob/tooltipster

Load jQuery and library conditionally, via dynamic webpack import. We still use jQuery in this case, but only when the documentation project uses hoverxref

I would avoid this approach if possible because the underlying library is not well maintained anyways. We will either, replace the library now or in the future anyways. So, I wouldn't re-implement it using an old library at this point.

We replace the library with another implementation

This is the path I was following. I found popper.js which looked promising, and then found tippy.js which is another layer on top of popper.js that makes it easy to work (my POC lives at https://github.com/readthedocs/addons/blob/main/src/tooltips.js)

Today I found that popper.js was renamed to floating-ui (https://floating-ui.com/). So, it's worth to take another look at it. It's also worth to note that I didn't find any issue with these libraries and I had something that worked, but it wasn't just styled properly. Example:

Peek 2024-02-05 12-43

We rely on upcoming HTML features that aren't yet fully standard

Hrm, it doesn't seem this is gonna happen soon, tho.

@agjohnson
Copy link
Contributor Author

Hrm, it doesn't seem this is gonna happen soon, tho.

Most browsers already have this feature actually 1 -- Firefox is the outlier here, it's only in nightly right now.

I feel like our time frame for this feature is still a little ways out, perhaps after we get a wider distribution on the current work in addons? So, time frames might even align too.

That is, we might not have to drop the work we have here already, and instead could use our current dependency until we swap to native only. Or do some polyfill if the native feature is missing and back down to the current library.

My main concern is reworking our dependency here twice. I think we should ultimately land on using native features if we can. Investing in a secondary intermediate dependency doesn't feel super useful for that.

If floating-ui has plans to act as a popover polyfill, that would be exactly what we need. I'm not familiar with the library though.

Footnotes

  1. https://caniuse.com/?search=popover

@humitos humitos changed the title Hoverxref popup implementation Tooltips: sphinx-hoverxref popup implementation Feb 21, 2024
@humitos humitos removed the Needed: design decision A core team decision is required label Jun 18, 2024
@humitos humitos added Accepted Accepted issue on our roadmap Feature New feature labels Jul 9, 2024
@zanderle
Copy link
Collaborator

I've used popper.js/floating-ui before, and I have good experience with it. As far as I know it's a pretty well maintained library.

I haven't thought of the native implementation before, but I like the idea, so I'll take a look into that first (and probably check floating-ui if they've had any discussions/thoughts around that). My initial reaction is skeptical towards the native implementation, but since I haven't looked into it before, so I'm ready to be surprised :D

@agjohnson
Copy link
Contributor Author

I'm guessing that none of these libraries are operating as a polyfill for popover, but maybe I'm wrong.

Since my last comment, and since 2024-04, popover is now in Firefox and Firefox Mobile, and has been in all other browsers since early last year.

image

So I'm inclined to lean into this feature, rather than settle into a library that will naturally self-deprecate due to the overlap. But if there are strong reasons to use a third party library here, that might influence the decision.

@zanderle
Copy link
Collaborator

zanderle commented Aug 2, 2024

Ok so unless I'm misunderstanding this addon, a very strong argument against using native popover is positioning. The big advantage of using popper.js/floating-ui is that it deals with positioning really well. Native popover doesn't seem to do any of that, which makes me think that implementation using native popover will be much more time-consuming.
Looking closer at native popover, I think comparing popper.js and popover is not really fair. We should instead be comparing it to CSS anchor positioning which unfortunately is not at all ready to be used (not even close).

So my vote is definitely for popper.js/floating-ui for now. What do you think?

@humitos
Copy link
Member

humitos commented Aug 2, 2024

So my vote is definitely for popper.js/floating-ui for now. What do you think?

I'm fine going in that direction 👍

@zanderle
Copy link
Collaborator

Floating ui is a bit more low-level than tippy js, which is currently used in tooltips.js. So the implementation will be slightly different from what we have now. The idea is:

  1. User hovers over an a.internal link
  2. The provided url is fetched with "/_/api/v3/embed/?" prefix
  3. A div is added to the DOM as a sibling to the hovered a.internal
  4. This div is positioned using floating ui
  5. The div is populated with the fetched content using innerHTML

Is it ok to leave that div in the DOM, and just hide it. And then show it again once the link is hovered again?

@humitos
Copy link
Member

humitos commented Sep 10, 2024

User hovers over an a.internal link

I'm not sure about the a.internal CSS selector. I suppose that .internal class may not be used for all the doctools we support. Finding a selector more generic would be awesome here. Maybe [href~={{ window.location.hostname }}] or similar. Would that make sense?

In any case, the initial implementation could use a.internal for now since it's the Sphinx one 👍🏼

Is it ok to leave that div in the DOM, and just hide it. And then show it again once the link is hovered again?

I suppose there is no problem. Maybe hovering a lot of links could make the page to become slow, tho. We don't need to premature optimize it, so either is fine to me.

@zanderle
Copy link
Collaborator

I'm not sure about the a.internal CSS selector. I suppose that .internal class may not be used for all the doctools we support. Finding a selector more generic would be awesome here. Maybe [href~={{ window.location.hostname }}] or similar. Would that make sense?

In any case, the initial implementation could use a.internal for now since it's the Sphinx one 👍🏼

I just proposed what's already in tooltips.js and hoverxref.js. If there's a better selector to use, that's even better. For now I can go with the simple a.internal and then we can improve later?

@humitos
Copy link
Member

humitos commented Sep 10, 2024

For now I can go with the simple a.internal and then we can improve later?

Yeah, that's fine.

@zanderle
Copy link
Collaborator

@humitos how should styling be handled? Currently (the way sphinx-hoverxref is implemented), things are displayed using the styles that are already on the page (which might be completely different from the content in the link). I assume that's ok, because the intention is that the links are all under the same site, right?

@humitos
Copy link
Member

humitos commented Sep 10, 2024

things are displayed using the styles that are already on the page (which might be completely different from the content in the link). I assume that's ok, because the intention is that the links are all under the same site, right?

Yes, this is correct. Since we are using links from the same project, the style should match. However, in the current (Sphinx) implementation we are linking to external resources as well (e.g. using intersphinx) and if the external content is not using the exact same Sphinx theme, the look & feel is weird.

Unfortunately, we don't have a good answer for the external link use cases; so we can skip it for now. If you have ideas about how to implement this properly, feel free to share them here or open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature
Projects
Archived in project
3 participants