feat(theme-classic): reintroduce autoscrolling TOC #6666

mjvbz · 2022-02-13T06:34:36Z

#6317 Reimagined. Reverts #6748

#6317 breaks pages using scroll-behavior: smooth as both JS and CSS are trying to smoothly scroll the page. For sites like about.supabase.com which uses scroll-behavior: smooth (reasonable thing to add IMO), clicking on the TOCs doesn't bring you to the right section of the page. ( the issue on #6620 )

This is Pull Request in order to fix this issue and bring the website a better user experience.

This PR:

Delays the auto-scroll of TOC after the page has finished scrolling, to prevent concurrent scrolls
Makes the scroll only happen if a vertical scroll can bring the link into view
Adds an autoScrollTOC config option
Adds scroll-behavior: smooth to the website, both for dogfooding and better UX

netlify · 2022-02-13T06:40:40Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`d7ece94`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6242d9f9c4e8dd00084f65f4
😎 Deploy Preview	https://deploy-preview-6666--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-02-13T06:41:11Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	50
🟢 Accessibility	100
🟠 Best practices	83
🟢 SEO	100
🟠 PWA	80

Lighthouse ran on https://deploy-preview-6666--docusaurus-2.netlify.app/

Josh-Cena · 2022-02-13T08:42:14Z

@yangshun I think this fixes it. You can test here: https://deploy-preview-6666--docusaurus-2.netlify.app/tests/docs/more-test/ or on any other page

Josh-Cena · 2022-02-13T10:12:21Z

packages/docusaurus-theme-classic/src/validateThemeConfig.ts

@@ -46,6 +46,7 @@ const DEFAULT_CONFIG = {
    minHeadingLevel: 2,
    maxHeadingLevel: 3,
  },
+  autoScrollTOC: true,


We can keep this undocumented for now, probably not useful for most people, as their sites don't work because of broken CSS in the first place...

yangshun

Looks great!

website/src/css/custom.css

Josh-Cena · 2022-02-13T12:11:17Z

Oops, not entirely good. The back-to-top button doesn't work

Josh-Cena · 2022-02-13T12:18:28Z

Ah, it's caused by our smooth scrolling polyfill, which supabase gets around by removing the BTT button altogether.

@slorber What do you think about here? Should we use scroll-behavior: smooth for our own website? At least I don't think the BTT button should break when the user has scroll-behavior: smooth in userland.

mjvbz · 2022-02-13T13:16:32Z

Looks Great on the new changes!

Josh-Cena · 2022-02-14T06:47:24Z

Okay, BTT button fixed with some hacks...

mjvbz · 2022-02-14T10:08:53Z

Okay, BTT button fixed with some hacks...

Great! Everything is working fine and the documents have no bugs.

duanwilliam · 2022-02-15T03:19:16Z

smooth scrolling is still sometimes interrupted on lower performance machines (dropped frames?). using a higher value on the timeout e.g. 100, 250 doesn't seem to visibly affect the perceived display of the active toc item (at least in my opinion), while significantly lowering the chance of interruption on slower machines

Josh-Cena · 2022-02-15T03:35:51Z

@duanwilliam Good point. Turned the timeout up to 200, and the behavior is fine.

mjvbz · 2022-02-16T02:51:20Z

Is it sure that this will be working on all browsers and systems, Does any system or browser will find bugs? @Josh-Cena

slorber

this looks to me quite complex, hard to review/test and with a lot of moving parts for such a tiny toc highlighting feature 🤷‍♂️

I'd rather plainly disable this feature (or make it opt-in, default to false) until we can find a good implementation for it, as implementing and reviewing this properly prevents us to focus on 2.0 which is way more important.

This new implementation breaks TOC navigation.

This is what I get when clicking on "Crowdin Overview" in the TOC while at the bottom of this page: https://deploy-preview-6666--docusaurus-2.netlify.app/docs/i18n/crowdin/#example-configuration

The clicked TOC heading now appears under the navbar and is not visible anymore

slorber · 2022-02-17T14:01:16Z

packages/docusaurus-theme-classic/src/theme/BackToTopButton/index.tsx

@@ -37,18 +37,29 @@ function smoothScrollTopNative(): CancelScrollTop {

 function smoothScrollTopPolyfill(): CancelScrollTop {
  let raf: number | null = null;
+  // Coercing to auto scroll to prevent conflicts with scroll-behavior.
+  // This is a polyfill for browsers without smooth scrolling anyways.
+  document.documentElement.style.scrollBehavior = 'auto';


if a previous value is already set, will it be properly restored?

What's the impact of not restoring it?

There should be no other case where a style is inlined in the DOM. I don't believe any userland code would do that instead of using :root selector. We need to force using scroll-behavior: auto, because otherwise CSS and JS would scroll the page simultaneously and cancel each other, as is the entire purpose of sending this PR.

slorber · 2022-02-17T14:04:30Z

packages/docusaurus/src/client/PendingNavigation.tsx

            window.scrollTo(0, 0);
+            document.documentElement.style.scrollBehavior = '';


same here, restore previous value?

There should be no previous value.

slorber · 2022-02-17T14:08:22Z

packages/docusaurus-theme-common/src/utils/useTOCHighlight.ts

+        ) {
+          link.scrollIntoView({block: 'nearest', behavior: 'smooth'});
+        }
+      }, 200);


magic timeout values like that are a recipe for troubles 😅 I'd like to avoid it at all cost

What about waiting for the end of scroll events using debounce + trailing events?

Well, even if you are debouncing, you have to debounce within a certain time range, right? This is essentially a debounce.

slorber · 2022-02-17T14:18:52Z

packages/docusaurus-theme-common/src/utils/useTOCHighlight.ts

      }
+      const handle = setTimeout(() => {
+        const linkRect = link.getBoundingClientRect();


all this code is quite low-level, please extract it under a meaningful function name that convey intent, and allowing us to more easily migrate to the official "if-needed" feature later?

scrollIntoView(node, { scrollMode: 'if-needed', block: 'nearest', behavior: 'smooth' })

Maybe using this package? https://github.com/stipsan/scroll-into-view-if-needed

This is not if-needed. It's the exact opposite: we don't scroll if we have to do a horizontal scroll. We don't care if we do a moot scroll, but we don't want the scroll to be horizontal.

Josh-Cena · 2022-02-17T14:50:22Z

This new implementation breaks TOC navigation.

It has nothing to do with this fix. It's because of the scroll-behavior: smooth CSS. Our implementation assumes that TOC navigation would always hide the navbar, but now, an anchor link navigation can actually scroll up and make the navbar visible. If we revert to scroll-behavior: auto on our website, we won't observe this anymore.

We are mainly just interested in fixing the JS and empowering users to use scroll-behavior: smooth in userland. (And in such case they probably won't use the auto-hiding navbar)

Josh-Cena · 2022-02-17T14:53:29Z

It started as a quick fix for the autoscrolling TOC with scroll-behavior: smooth CSS, but we quickly noticed much more incompatibilities, such as the back-to-top button. The ultimate aim of this PR is to make the user freely enable smooth scroll behavior without doing other work to maintain a good UX.

slorber · 2022-02-17T15:07:16Z

Can we split this into separate PRs so that we can understand exactly what is the purpose and impact of each piece of code?

It will help me review it because in current state 🤷‍♂️ Remember I want to focus on 2.0 so I can't spend too much time on this PR

Can we have the most important fix (#6620) merged immediately, and figure it out later for the smooth scroll which is useful but less important?

mjvbz · 2022-03-01T03:14:38Z

@slorber, @Josh-Cena
Should I resolve the conflicts errors?
packages/docusaurus-theme-common/src/utils/useTOCHighlight.ts

Fix Smooth Scrolling

2f786a7

mjvbz requested review from slorber, lex111 and Josh-Cena as code owners February 13, 2022 06:34

facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 13, 2022

Josh-Cena changed the title ~~Fix Smooth Scrolling~~ fix: allow auto-scrolling TOC with smooth scrolling Feb 13, 2022

Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Feb 13, 2022

Josh-Cena and others added 6 commits February 13, 2022 14:43

add dogfooding cases

a23fdfb

revert scroll behavior change

6cea769

repro

0f72316

Fixing Bug

dbd09ca

try fixing

76681dd

maybe?

a356080

Josh-Cena requested a review from yangshun February 13, 2022 08:41

Josh-Cena changed the title ~~fix: allow auto-scrolling TOC with smooth scrolling~~ fix: various auto-scrolling TOC fixes Feb 13, 2022

Josh-Cena added 6 commits February 13, 2022 16:53

refactor

c242738

fix

0394d5b

fix?

e03cbc9

fix horizontal scroll

849ae7b

tiny refactor

554047a

add autoScrollTOC option

49e3052

Josh-Cena reviewed Feb 13, 2022

View reviewed changes

Josh-Cena added 2 commits February 13, 2022 18:26

add another condition

200ea3f

refactor

7dd0574

Josh-Cena mentioned this pull request Feb 13, 2022

Scrolling issue since beta.15 #6587

Closed

7 tasks

refactor

cff4337

yangshun approved these changes Feb 13, 2022

View reviewed changes

yangshun reviewed Feb 13, 2022

View reviewed changes

website/src/css/custom.css Outdated Show resolved Hide resolved

move CSS

24b04ea

Josh-Cena added 2 commits February 14, 2022 14:27

Fix BTT button

d5ebe74

actually fix..

e8819fb

Josh-Cena added 2 commits February 15, 2022 11:27

relax timeout

ce382c9

fix page jump

eba3e5b

Josh-Cena changed the title ~~fix: various auto-scrolling TOC fixes~~ fix: various auto-scrolling TOC fixes; better support for smooth scroll-behavior Feb 15, 2022

slorber requested changes Feb 17, 2022

View reviewed changes

slorber mentioned this pull request Feb 23, 2022

fix(theme-classic): temporarily disable toc heading autoscrolling #6748

Merged

slorber marked this pull request as draft February 23, 2022 16:09

duanwilliam mentioned this pull request Feb 24, 2022

Possible scroll issue in latest version of Chrome ( v98 ) #6754

Closed

7 tasks

mjvbz mentioned this pull request Mar 2, 2022

Website scrolling issue near/NEPs#318

Open

Josh-Cena mentioned this pull request Mar 29, 2022

fix(theme-common): use native scrolling when smooth behavior set in CSS #7057

Merged

Merge branch 'main' into patch-1

d7ece94

Josh-Cena changed the title ~~fix: various auto-scrolling TOC fixes; better support for smooth scroll-behavior~~ feat(theme-classic): reintroduce autoscrolling TOC Mar 29, 2022

Josh-Cena added pr: new feature This PR adds a new API or behavior. and removed pr: bug fix This PR fixes a bug in a past release. labels Apr 9, 2022

Josh-Cena mentioned this pull request Aug 6, 2023

Persist sidebar state (categories expanded state, hidden sidebar) #6319

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme-classic): reintroduce autoscrolling TOC #6666

feat(theme-classic): reintroduce autoscrolling TOC #6666

mjvbz commented Feb 13, 2022 •

edited by Josh-Cena

Loading

netlify bot commented Feb 13, 2022 •

edited

Loading

github-actions bot commented Feb 13, 2022 •

edited

Loading

Josh-Cena commented Feb 13, 2022

Josh-Cena Feb 13, 2022

yangshun left a comment

Josh-Cena commented Feb 13, 2022

Josh-Cena commented Feb 13, 2022

mjvbz commented Feb 13, 2022 •

edited

Loading

Josh-Cena commented Feb 14, 2022

mjvbz commented Feb 14, 2022

duanwilliam commented Feb 15, 2022

Josh-Cena commented Feb 15, 2022

mjvbz commented Feb 16, 2022 •

edited

Loading

slorber left a comment •

edited

Loading

slorber Feb 17, 2022

Josh-Cena Feb 17, 2022

slorber Feb 17, 2022

Josh-Cena Feb 17, 2022

slorber Feb 17, 2022

Josh-Cena Feb 17, 2022 •

edited

Loading

slorber Feb 17, 2022

Josh-Cena Feb 17, 2022

Josh-Cena commented Feb 17, 2022

Josh-Cena commented Feb 17, 2022 •

edited

Loading

slorber commented Feb 17, 2022

mjvbz commented Mar 1, 2022

		window.scrollTo(0, 0);
		document.documentElement.style.scrollBehavior = '';

feat(theme-classic): reintroduce autoscrolling TOC #6666

Are you sure you want to change the base?

feat(theme-classic): reintroduce autoscrolling TOC #6666

Conversation

mjvbz commented Feb 13, 2022 • edited by Josh-Cena Loading

netlify bot commented Feb 13, 2022 • edited Loading

✅ [V2]

github-actions bot commented Feb 13, 2022 • edited Loading

Josh-Cena commented Feb 13, 2022

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

Josh-Cena commented Feb 13, 2022

Josh-Cena commented Feb 13, 2022

mjvbz commented Feb 13, 2022 • edited Loading

Josh-Cena commented Feb 14, 2022

mjvbz commented Feb 14, 2022

duanwilliam commented Feb 15, 2022

Josh-Cena commented Feb 15, 2022

mjvbz commented Feb 16, 2022 • edited Loading

slorber left a comment • edited Loading

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

Josh-Cena Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Feb 17, 2022

Josh-Cena commented Feb 17, 2022 • edited Loading

slorber commented Feb 17, 2022

mjvbz commented Mar 1, 2022

mjvbz commented Feb 13, 2022 •

edited by Josh-Cena

Loading

netlify bot commented Feb 13, 2022 •

edited

Loading

github-actions bot commented Feb 13, 2022 •

edited

Loading

mjvbz commented Feb 13, 2022 •

edited

Loading

mjvbz commented Feb 16, 2022 •

edited

Loading

slorber left a comment •

edited

Loading

Josh-Cena Feb 17, 2022 •

edited

Loading

Josh-Cena commented Feb 17, 2022 •

edited

Loading