Highlight active menu items as page scrolls divs (Sidebar onscroll menu)

Question

This JSFiddle by Gaurav Kalyan works well in Chrome, but in Safari and Firefox it activates the wrong menu item. Instead of highlighting the menu item clicked, it highlights the menu item before. So, for example, if you click on "Punkt 4", "Punkt 3" is highlighted instead. I haven’t been able to fix this. Can someone help? I've been trying to solve this for two weeks.

HTML

    <section id="main">
    <div class="target" id="1">TARGET 1</div>
    <div class="target" id="2">TARGET 2</div>
    <div class="target" id="3">TARGET 3</div>
    <div class="target" id="4">TARGET 4</div>
</section>
<aside id="nav">
    <nav>
        <a href="#1" class="active">Punkt 1</a>
        <a href="#2">Punkt 2</a>
        <a href="#3">Punkt 3</a>
        <a href="#4">Punkt 4</a>
    </nav>
</aside>

CSS

    * {
    margin: 0;
    padding: 0;
}

#main {
    width: 75%;
    float: right;
}

#main div.target {
    background: #ccc;
    height: 400px;
}

#main div.target:nth-child(even) {
    background: #eee;
}

#nav {
    width: 25%;
    position: relative;
}

#nav nav {
    position: fixed;
    width: 25%;
}

#nav a {
    border-bottom: 1px solid #666;
    color: #333;
    display: block;
    padding: 10px;
    text-align: center;
    text-decoration: none;
}

#nav a:hover, #nav a.active {
    background: #666;
    color: #fff;
}

JavaScript

    $('#nav nav a').on('click', function(event) {
    $(this).parent().find('a').removeClass('active');
    $(this).addClass('active');
});

$(window).on('scroll', function() {
    $('.target').each(function() {
        if($(window).scrollTop() >= $(this).offset().top) {
            var id = $(this).attr('id');
            $('#nav nav a').removeClass('active');
            $('#nav nav a[href=#'+ id +']').addClass('active');
        }
    });
});

Answer 1

This works fine as is if the viewport height (the inner height of the browser window) is <= 400px. That is because when you click on the a link in the nav element, with an href of #4, the default browser behavior kicks in and the element with id="4" is scrolled to the top (as much as is possible).

When the viewport is the same height or smaller than the element being scrolled to, then when your scroll handler gets triggered, the if($(window).scrollTop() >= $(this).offset().top) condition evaluates as true, because the scrollTop will be exactly equal to the offset().top of the #4 div.

However, when the viewport is bigger than the content div (in your case, > 400px), when the browser tries to scroll the last div into view, it can completely do so whilst still displaying part of the bottom half of the previous div. Which means that the 3rd div will pass your scroll handler if check, not your fourth. (The offset top of the last div will not be <= the scrollTop of the window).

So what's the solution?

I would make it so that each target div is at least the same height as the viewport. You can achieve this on modern browsers using min-height: 100vh; (100% of the viewport height). That means when the last one is scrolled into view, it will completely fill the viewport, and the correct div will pass your scroll logic check correctly.

See here for a working fork.

Bonus tip

There is a number of things you can do to improve performance of this code. Cache the creation of jQuery variables, avoid the repeated work happening 4 times on every scroll event (which can happen very often), etc. It works okay for now, but it may become a bottleneck later.