ScrollSpy for table of contents #1557

edemaine · 2018-08-06T22:44:46Z

Fix #1537 by adding automatic highlighting of currently visible topmost table of contents item.

~~This sometimes doesn't do an initial highlight, I guess because the table of contents isn't rendered at the beginning? Is there a way to fix this? But it works when scrolling and resizing.~~

Based on #1537 (comment) and https://github.com/makotot/scrollspy/blob/master/src/js/modules/scrollspy.js

ylemkimon · 2018-08-07T04:30:14Z

website/siteConfig.js

-    scripts: ['https://buttons.github.io/buttons.js'],
+    scripts: [
+        'https://buttons.github.io/buttons.js',
+        baseUrl + 'js/scrollspy.js',


You can use /js/scrollspy.js.

@edemaine Sorry, it turns out baseUrl is needed for scripts path. I'll open a PR.

ylemkimon · 2018-08-07T04:32:28Z

website/static/js/scrollspy.js

+document.addEventListener('scroll', onScroll);
+document.addEventListener('resize', onScroll);
+document.addEventListener('ready', onScroll);
+onScroll();


Wrap this in a closure (IIFE) to prevent polluting global scope. Or wrap in a function and attach to DOMContentLoaded to cache elements and positions (#1557 (comment)).

The script is included in the <head>, so calling onScroll here has no effect.

ylemkimon · 2018-08-07T04:35:45Z

website/static/js/scrollspy.js

+};
+document.addEventListener('scroll', onScroll);
+document.addEventListener('resize', onScroll);
+document.addEventListener('ready', onScroll);


ready is only available on jQuery. You should use DOMContentLoaded.

ylemkimon · 2018-08-07T04:39:53Z

website/static/js/scrollspy.js

@@ -0,0 +1,37 @@
+const epsilon = 10;


I think the name OFFSET is better. (All caps, as it's a constant)

ylemkimon · 2018-08-07T04:46:30Z

website/static/js/scrollspy.js

+    }
+    timer = setTimeout(() => {
+        let found = false;
+        const headings = document.querySelectorAll('.toc-headings > li > a');


You can cache (pre-calculate) this value after DOMContentLoaded. You can also probably cache positions of their headers, but I'm not sure DOMContentLoaded waits for fonts to be loaded and rendered.

I think we can cache positions of headers based on the scrollHeight. i.e., update cached positions if scrollHeight has been changed. (AFAIK, modern browsers support document.documentElement.scrollHeight)

I've added the query cache (though if we ever load pages via AJAX, this will break). I don't think we need to cache the positions of the headers -- these should be effectively cached by the web browser. This is also standard for scrollspy.

ylemkimon · 2018-08-07T04:51:32Z

website/static/js/scrollspy.js

+        // https://github.com/makotot/scrollspy/blob/master/src/js/modules/scrollspy.js
+        const scrollTop = document.documentElement.scrollTop ||
+            document.body.scrollTop;
+        const scrollBottom = scrollTop + window.innerHeight;


scrollBottom is not used.

ylemkimon · 2018-08-07T05:03:42Z

website/static/js/scrollspy.js

+        // Some computations based on
+        // https://github.com/makotot/scrollspy/blob/master/src/js/modules/scrollspy.js
+        const scrollTop = document.documentElement.scrollTop ||
+            document.body.scrollTop;


All browsers except IE<9, which Docusaurus and we don't support, support window.pageYOffset (https://developer.mozilla.org/en-US/docs/Web/API/Window/pageYOffset).

edemaine · 2018-08-07T13:50:48Z

@ylemkimon Thanks for all the comments and pointers. I've implemented them all, except for caching header positions.

ylemkimon · 2018-08-07T14:15:02Z

website/static/js/scrollspy.js

@@ -0,0 +1,44 @@
+// Based in part on
+// https://github.com/makotot/scrollspy/blob/master/src/js/modules/scrollspy.js


I think it is no longer based on it.

Maybe "inspired by"? I could remove the link and just mention that ScrollSpy is a thing. I also wonder whether this file should be called "scrollspy.js" or something else...

ylemkimon · 2018-08-07T14:18:37Z

website/static/js/scrollspy.js

+                    const next = headings[i + 1].href.split('#')[1];
+                    const nextHeader = document.getElementById(next);
+                    const top = nextHeader.getBoundingClientRect().top;
+                    current = top > OFFSET;


This should be current = top > scrollTop + OFFSET;

It used to be top = nextHeader.getBoundingClientRect().top + scrollTop and then current = top + scrollTop > scrollTop + OFFSET. The scrollTops cancel.

ylemkimon · 2018-08-07T14:23:30Z

website/static/js/scrollspy.js

+    let timer;
+    let headingsCache;
+    const findHeadings = () =>
+        document.querySelectorAll('.toc-headings > li > a');


I think we can check whether cache is available here, i.e., const findHeadings = () => headingsCache || document.querySelectorAll('.toc-headings > li > a');.

I don't think so. In my experience, it takes multiple seconds before the DOM content is fully loaded, and someone could easily scroll during that time. We don't want to accidentally cache at this time or we might miss some of the elements...

Alternatively, I could bind the scroll etc. events only after DOM content is fully loaded. But I like that the current code does its best effort while the page is still loading, in the case that someone scrolls early.

Oops, I see now that you were suggesting the cache could be read here, not written. I'll do that.

ylemkimon

~~Please fix lint errors.~~ I'll fix them on my end.

codecov · 2018-08-08T00:00:52Z

Codecov Report

Merging #1557 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1557   +/-   ##
=======================================
  Coverage   84.49%   84.49%           
=======================================
  Files          79       79           
  Lines        4619     4619           
  Branches      807      807           
=======================================
  Hits         3903     3903           
  Misses        619      619           
  Partials       97       97

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48e6d9d...96b02a7. Read the comment docs.

ylemkimon · 2018-08-08T00:22:01Z

website/static/js/scrollspy.js

+        if (timer) {  // throttle
+            clearTimeout(timer);
+        }
+        timer = setTimeout(() => {


I think throttling is more suitable than debounce. (This will debounce, i.e., run only once after repeated scrolling)

OK, I've switched back to throttling. Thanks for the lint fixes! I also tweaked the comment now that scrollTop is no longer defined.

edemaine added 2 commits August 6, 2018 18:41

ScrollSpy for table of contents

1265cc5

Add reference

bb48965

ylemkimon reviewed Aug 7, 2018

View reviewed changes

edemaine added 2 commits August 7, 2018 09:45

Incorporate comments

d826154

Merge remote-tracking branch 'upstream/master' into scrollspy

d204206

ylemkimon reviewed Aug 7, 2018

View reviewed changes

edemaine added 2 commits August 7, 2018 14:10

Simplify headingsCache and change ScrollSpy reference

720583e

Add comment

5ce44e3

ylemkimon approved these changes Aug 7, 2018

View reviewed changes

Fix lint errors

20c921d

Merge branch 'master' into scrollspy

802157e

ylemkimon requested changes Aug 8, 2018

View reviewed changes

Switch back to throlling, update comments

69ffb99

ylemkimon approved these changes Aug 8, 2018

View reviewed changes

Merge branch 'master' into scrollspy

96b02a7

ylemkimon merged commit 05dc921 into KaTeX:master Aug 8, 2018

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScrollSpy for table of contents #1557

ScrollSpy for table of contents #1557

edemaine commented Aug 6, 2018 •

edited

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 8, 2018

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 7, 2018

ylemkimon Aug 7, 2018

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 7, 2018

ylemkimon Aug 7, 2018

edemaine Aug 7, 2018

ylemkimon Aug 7, 2018

ylemkimon Aug 7, 2018 •

edited

edemaine commented Aug 7, 2018

ylemkimon Aug 7, 2018

edemaine Aug 7, 2018

ylemkimon Aug 7, 2018

edemaine Aug 7, 2018

ylemkimon Aug 7, 2018

edemaine Aug 7, 2018

edemaine Aug 7, 2018

ylemkimon left a comment •

edited

codecov bot commented Aug 8, 2018 •

edited

ylemkimon Aug 8, 2018 •

edited

edemaine Aug 8, 2018

		@@ -0,0 +1,44 @@
		// Based in part on
		// https://github.com/makotot/scrollspy/blob/master/src/js/modules/scrollspy.js

ScrollSpy for table of contents #1557

ScrollSpy for table of contents #1557

Conversation

edemaine commented Aug 6, 2018 • edited

ylemkimon Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon Aug 7, 2018 • edited

Choose a reason for hiding this comment

edemaine commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon left a comment • edited

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2018 • edited

Codecov Report

ylemkimon Aug 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine commented Aug 6, 2018 •

edited

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 7, 2018 •

edited

ylemkimon Aug 7, 2018 •

edited

ylemkimon left a comment •

edited

codecov bot commented Aug 8, 2018 •

edited

ylemkimon Aug 8, 2018 •

edited