BUG - stop using Scrollspy #2107
Conversation
Coverage report
This PR does not seem to contain any modification to coverable code. |
There was a problem hiding this comment.
Self review.
I'm not sure whether we should style the link after it's been clicked on not. On the one hand, it gives feedback to the user that the link was clicked, but it does not get updated later if the user scrolls away from that section.
The behavior in this PR matches what you get in the GitHub Actions docs.
But I wonder if we should match the Sphinx docs behavior. If you click a section link in the Sphinx docs, you are take to the corresponding page and the TOC link is highlighted (bolded). But if you click any of the in-page TOC links, you are taken to that section, but the TOC link is not bolded or highlighted or anything.
One argument for not doing anything is simplicity: we can remove most of the code in this pull request.
| * see https://github.com/twbs/bootstrap/issues/20086 | ||
| */ | ||
| function addTOCInteractivity() { | ||
| window.addEventListener("activate.bs.scrollspy", function () { |
There was a problem hiding this comment.
I somewhat kept this behavior here of expanding nested portions of the TOC.
Previously, the TOC would auto-expand as the user scrolled down the page into sections nested deeper and deeper. For example if the sidebar page TOC was configure to only show links up to level 2 headings, then as the user scrolled into level 3, level 4, level 5, the sidebar TOC would also expand to show increasingly nested and indented links to those sections.
Now, the TOC does not update as the user scroll, but if the user clicks a link in the TOC that has sub-entries, the sub-entries will be shown, and if the user clicks one of those sub-entries and those sub-entries have sub-entries, then it will expand one more level, and so on and so forth.
For more info, refer to show_toc_level.
| clickedLink.setAttribute("aria-current", "true"); | ||
| clickedLink.classList.add("active"); |
There was a problem hiding this comment.
This will add styles to the link that make it look like the current link. This matches the behavior of the GitHub Actions docs, for example see Understanding GitHub Actions.
Note that once the user clicks on the link, it keeps this styling even if they scroll to a completely different section. It only gets cleared and applied to a different TOC link if the user clicks on a different TOC link.
| {# set up with scrollspy to update the toc as we scroll #} | ||
| {# ref: https://getbootstrap.com/docs/4.0/components/scrollspy/ #} | ||
| <body data-bs-spy="scroll" data-bs-target=".bd-toc-nav" data-offset="180" data-bs-root-margin="0px 0px -60%" data-default-mode="{{ default_mode }}"> | ||
| <body data-default-mode="{{ default_mode }}"> |
There was a problem hiding this comment.
I moved the skip link out of the body_tag block for two reasons:
- I noticed that if consumers of this theme wanted to customize the scroll spy behavior they would need to override this block, but if they did that, there's a good chance they might nuke the skip link in the process.
- It's counterintuitive to put anything in a block named "body_tag" other than, well, the body tag.
gabalafou
added
kind: bug
gabalafou
force-pushed
the
stop-using-scrollspy
branch
from
c18b31f to
28560cb
Compare
There was a problem hiding this comment.
Thank you so much for this! I spent yesterday trying to debug ScrollSpy and came to the same conclusion - that there's something in ScrollSpy itself rather than our usage that is the root cause here.
I played with this for a while and discovered one small issue. Otherwise let's 🚢
Related:
|
Every other advanced theme that I'm aware of are implementing a scrollspy-like feature so I think it would be a mistake to drop it without a proper replacement. It's super useful when scrolling in a long api-doc generated page to know where you stand. I can add to the list if related issues #1207 that is bugging me for years. I received 0 answer from the Bootstrap team so I guess it will never be looked after unfortunately. |
|
@12rambau, agreed, in the long term. But I think that for now we should turn the feature off while it's buggy because I think that the frustration, confusion, lost time, and bug reports outweigh the benefits. |
gabalafou
added
needs: discussion
|
In the meantime, I'm marking this pull request as needs discussion and do not merge until we've come to agreement as a team :) |
|
I did a little reading:
So it seems there's no established convention about the best approach as far as I can tell. But there's a pretty nice example on the Intersection Observer API docs that we could follow pretty closely to implement page-level TOC highlighting, I think. Essentially you'd just observe every heading in the article, and highlight the appropriate section in the page's TOC whenever the intersection callback is fired. If nobody else is interested, I'd be happy to follow up this PR with an implementation - just let me know :) |
|
I'm in favor of letting @peytondmurray give it a shot. Absent any obvious consensus among the major implementations, doing what MDN does on their own site seems like a good choice to me. And depending on how quickly it gets done, it might make moot the question of whether we should ship a release that lacks any scrollspy implementation. |
Fixes #1965.
Scrollspy is broken. I tried tweaking some of its options but I could not get it to work. It's even broken in the most up-to-date Bootstrap docs. (By broken, I mean that I can easily reproduce the kind of buggy behavior described in #1965, e.g., I click on a link in the side nav but instead of highlighting the link I just clicked, it highlights the subsequent link. That's because the headings associated with both links have come into the viewport and Scrollspy's logic somehow considers the second heading to be the one I am currently engaging and therefore it highlights the second link rather than the one that I clicked.)
I poked around the Scrollspy source code, I tried tweaking the options that we pass to Scrollspy, I tried various suggestions that people had written in on the issues in the Bootstrap repo, but I could not get it to work.
The comments on the Bootstrap issue ScrollSpy not behaving correctly #36431 make me think that Bootstrap's current implementation of Scrollspy is not meeting people's expectations, so I think we should drop it.
In the future, we could look into using a different Scrollspy library, implementing our own, or seeing if Bootstrap fixes theirs.
But for now, I think that the functionality that Scrollspy provides is not critical, and it's better to not have it all than to have a version of it that upsets user's expectations and results in bug reports getting filed to our repo.