Scrolling behavior of Reference sidebar

[johannahoerrmann]

The sidebar of the Reference shows some strange / buggy behavior when clicking on an item (take "Blueprints" for example): https://nnnnext.getkirby.com/docs/reference/blueprints

On click, the sidebar is sometimes

• automatically scrolled (or rather "jumps") so that the selected item sticks to the top (right below the black area),
• automatically scrolled so that the selected item dissapears under the black header
• not scrolled at all.

Of all of these, I prefer the latter – no distraction, easy orientation.

[distantnative]

The latter would be the ideal solution, but since an actual page load happens, this is difficult to achieve across page loads. I added a small improvement, so that now it should scroll always to the current grouping in the sidebar, giving a bit more orientation.

[lukasbestle]

I've built a very similar sidebar autoscroll feature for my bachelor's thesis' literature page that works quite well:

import { throttle, localStorageGet, localStorageSet } from '../etc/helpers.js';

/**
 * Updates the current scroll position
 */
function update() {
    localStorageSet('literature.scrollTop', sidebarBody.scrollTop);
}

var sidebarBody = document.querySelector('.literature__sidebar__body');
if (sidebarBody) {
    // keep scroll position in literature sidebar
    var scrollPos = localStorageGet('literature.scrollTop');
    if (scrollPos) {
        sidebarBody.scroll(0, scrollPos);
    }

    // ensure that the current page's link is in view
    var currentLink = document.querySelector('.literature__sidebar__current');
    if (currentLink) {
        var currentLinkRect = currentLink.getBoundingClientRect(),
            bodyRect        = sidebarBody.getBoundingClientRect();
        if (currentLinkRect.top < bodyRect.top) {
            // scroll into view so that it is displayed at the top
            currentLink.scrollIntoView(true);
        } else if (currentLinkRect.bottom > bodyRect.bottom) {
            // scroll into view so that it is displayed at the bottom
            currentLink.scrollIntoView(false);
        }
    }

    // store current scroll position
    update();

    // update scroll position dynamically
    sidebarBody.addEventListener('scroll', throttle(update, 100, true));
}

[distantnative]

I thought of something like this. Let me have a second look. So far I was a bit intimidated by the JS code that Fabian and Bastian wrote. Afraid to break anything.

[distantnative]

Got it! Thanks @lukasbestle - will push my solution now

[johannahoerrmann]

Thanks for working on this! Getting it straight will benefit a lot of users. However, I just checked out the latest version and still experience some unexpected strange behavior. Most notably, clicking on "Advanced" sends me back to the top of the sidebar, both in Firefox and Safari. This behaves different than the other navigation items anyway (no li, no marker), maybe that's why it doesn't work correctly yet? You seem to have experienced the minor jumping already reported by @nilshoerrmann on Slack yourself, although I only see it using Firefox.

[nilshoerrmann]

We are experiencing strange jumps in Firefox and now that I looked at the implemented script, I'm asking myself if this could be solved in an easier way without the need to throttle the scroll position detection:

1. Detecting the scroll position without delay

There is only one situation when the current scroll position should be restored: this is when I actually clicked a link in the navigation. So why don't you listen to the click event on the links in the navigation and store the scroll position in that event right before the browser actually follows the link?

currentSections.addEventListener('click', setScroll);

This will make sure that the localStorage only holds a value when necessary and that this value is actually correct in that moment of time.

2. Scrolling to the correct position

Currently, you are always using scrollIntoView to make sure that the current section is visible but that doesn't make sense to me: either you'd like to restore the previous scroll position or you'd like the browser to scroll the current section into view. So as soon as there is a stored position no automatic scrolling should happen because we want to restore that specific position. We'd like the current section to be scrolled into view only if we are loading from outside the navigation (external link or a link in the content area of the reference).

What about something this:

const currentSections = $(".cheatsheet-panel-scrollarea");
const currentSection  = $(".cheatsheet-sections a[aria-current]");
const currentEntry   = $(".cheatsheet-entries a[aria-current]");

function setScroll() {
  localStorage.setItem('getkirby$reference$scroll', currentSections.scrollTop);
}

function unsetScroll() {
  localStorage.removeItem('getkirby$reference$scroll');
}

if (currentSections) {
  const scroll = localStorage.getItem('getkirby$reference$scroll');

  if (scroll) {
    currentSections.scroll(0, scroll);
    unsetScroll();
  } else if(currentSection && currentSections.scrollIntoView) {
    const linkRect    = currentSection.getBoundingClientRect();
    const sidebarRect = currentSections.getBoundingClientRect();
    if (linkRect.top < sidebarRect.top) {
      currentSection.parentNode.parentNode.parentNode.scrollIntoView(true);
    } else if (linkRect.bottom > sidebarRect.bottom) {
      currentSection.parentNode.parentNode.parentNode.scrollIntoView(false);
    }
  }

  // store current scroll position
  currentSections.addEventListener('click', setScroll);
}

Does that make sense?

[lukasbestle]

I will take a look at it this evening. 🙂

[nilshoerrmann]

Thanks :)

[distantnative]

@johannahoerrmann:

Most notably, clicking on "Advanced" sends me back to the top of the sidebar, both in Firefox and Safari.

That's true at the moment but will be fixed with a restructuring PR coming up: https://github.com/getkirby/getkirby.com/pull/735 Once this one is merged, it won't be a problem anymore.

@nilshoerrmann

1. yes! agreed!
2. When we only store the scroll on click, this also makes much more sense.

[distantnative]

Implemented as suggested by @nilshoerrmann - thank you! Let's try it.

[lukasbestle]

@nilshoerrmann Currently, you are always using scrollIntoView to make sure that the current section is visible but that doesn't make sense to me: either you'd like to restore the previous scroll position or you'd like the browser to scroll the current section into view.

The reason my implementation has that feature is to make sure that the current page is always visible, no matter what the stored position is.

Try this with the current implementation by @distantnative:

1. Scroll the sidebar down and e.g. click on Server inside the Tools section.
2. Enter $file into the search inside the reference and click on the first result (docs/reference/objects/file).
3. The scroll position is still unchanged.

That edge case worked great on the site I took the code from, but it looked like there was a bug in the throttle helper of the Kirby site that made it buggy. I wanted to take a look at that, but 🤷‍♂.

Anyway, I think the current solution is good enough as it works for almost all cases.

[distantnative]

@lukasbestle I’m open to any changes :D bring them on 👍

[lukasbestle]

I would have known where to start with my implementation, but now it's a bit like how digging through the original code was for you if we don't want to introduce new bugs just to fix one. So let's just leave it as it is, I think it's pretty reliable already except that edge case, which is not a real problem in practice. :)

[nilshoerrmann]

Thanks, @distantnative – this works much better on Firefox. I can confirm the issue described by Lukas though. I think it's this line that should be removed:

https://github.com/getkirby/getkirby.com/blob/15b0b73d3f6bc9ceae7f6dda37c0d87a2a497947/src/js/templates/cheatsheet.js#L134-L135

… because otherwise this line will always be true:

https://github.com/getkirby/getkirby.com/blob/15b0b73d3f6bc9ceae7f6dda37c0d87a2a497947/src/js/templates/cheatsheet.js#L119

[distantnative]

I just stumbled upon this... did we not end up fixing it, @lukasbestle? Has anyone checked if @nilshoerrmann's suggestion works as intended? I'm confused XD

[lukasbestle]

Good question, looks like we missed it.

[distantnative]

Closing this for now.