Fix TOC scroll-spy active state landing on the wrong entry (fixes #1255) - #1256
Open
georgestephanis wants to merge 1 commit into
Open
Fix TOC scroll-spy active state landing on the wrong entry (fixes #1255)#1256georgestephanis wants to merge 1 commit into
georgestephanis wants to merge 1 commit into
Conversation
…erver scrollToElement() used Math.floor(rect.top) - offset, which always slightly undershoots the required scroll distance. The landed position ends up a fraction of a pixel above the offset line, which fails the (former) Gumshoe active check of top <= offset, so a click leaves the *previous* TOC entry highlighted instead of the one just clicked. Fixes the rounding (Math.ceil instead of Math.floor) and replaces the bundled Gumshoe scroll-event-polling scrollspy with a native IntersectionObserver implementation scoped to the TOC block only (the navigation block's separate scrollspy/Gumshoe usage is untouched). IntersectionObserver has ~96% global browser support (caniuse.com) with a W3C polyfill available for the remaining IE/Opera Mini share. Also collapses the two near-duplicate enqueue branches in render_table_of_content() into one now that scroll spy no longer needs the gumshoe dependency, and removes the TOC block's now-unused gumshoe script registration.
Author
|
(also in case there's any concern over the swapping to intersectionobserver, it's already in use in kb in several other spots already) |
Author
|
cc: @pauloiankoski -- any chance of remediation here or should I just patch stuff downstream? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1255.
Problem
With Smooth Scroll + Scroll Spy both enabled on a Table of Contents block, clicking an entry scrolls to the right heading but the previous entry stays marked
activeinstead of the one just clicked.Root cause
scrollToElement()computed the scroll distance asMath.floor(rect.top) - offset.Math.floortruncates the fractional part ofrect.topbefore subtractingoffset, so the scroll always slightly undershoots — the heading'stoplands a fraction of a pixel aboveoffset.parseInt(bounds.top, 10) <= offset. Since the click-scroll landed just above that line, the check fails for the just-clicked heading right after the scroll settles, so the previous entry stays highlighted until the user scrolls manually and Gumshoe's own poll catches up.What this PR does
scrollToElement()—Math.ceil(rect.top - offset)instead ofMath.floor(rect.top) - offset, guaranteeing the scroll distance is at least what's needed so the landed position satisfies the<= offsetcheck on the very next check.IntersectionObserverfor this block's scroll spy (initScrollSpy()insrc/assets/js/kb-table-of-contents.js). Same selection semantics as before (last heading in document order whosetop <= offset, with the existing "scrolled to the very bottom" fallback for a short final section), sameactive/active-parentclass behavior on<li>and the heading itself — just triggered by intersection callbacks instead of polling everyscrollevent. IntersectionObserver has ~96% global browser support (basically everything except IE11 and Opera Mini), with a W3C polyfill available for the remainder if that's a concern.src/assets/js/kb-navigation-block.jshas its own separatenew Gumshoe(...)call for the navigation block's scroll-spy feature and is untouched. (Could be a good follow-up if there's interest, since it's the same underlying pattern — happy to open a separate PR for that.)gumshoescript as a dependency, this also collapses the two near-duplicatewp_enqueue_script('kadence-blocks-table-of-contents', ...)branches inrender_table_of_content()into one, and removes the TOC block's now-unusedwp_register_script('kadence-blocks-gumshoe', ...)call inclass-kadence-blocks-table-of-contents-block.php. As a side effect this also removes a latent bug where the scroll-spy-disabled branch calledwp_enqueue_script('kadence-blocks-table-of-contents')with no args, relying on it having been registered elsewhere first.Testing
Manually reasoned through / traced the fix against the current
masterbehavior and confirmed it resolves the reported symptom: clicking any TOC entry now marks that entry (and only that entry) active immediately once the scroll settles, and scrolling manually (no click) still updates the active entry correctly as headings cross the offset line.I wasn't able to run the full
bun install/bun run build-wppipeline locally (private@kadence/*package auth isn't available to me), so I haven't regeneratedincludes/assets/js/kb-table-of-contents.min.jshere — happy to do that if someone can point me at credentials, or CI/a maintainer can regenerate it from this source change.AI disclosure
This PR was drafted with the assistance of Claude (Anthropic's AI coding assistant), operating under my direction and review. Claude did the investigation (tracing the bug across
kb-table-of-contents.jsandgumshoe.js, and confirming it against the currentmaster), wrote the diff, and drafted this description. I reviewed the diff and reasoning before opening the PR. As noted above, I was not able to run this project's build/lint pipeline (bun install/bun run build-wp/bun run lint-js) locally, so beyond a plainnode --checksyntax pass and manual review of the diff, the change hasn't been exercised against a live WordPress install with this exact patch — I'd appreciate a second look from a maintainer or CI before merge.