Handle multiple zero-offset Scrollspy elements. #15593

neoeno · 2015-01-17T14:24:45Z

When the first two elements tracked by Scrollspy have an offset.top of 0, Scrollspy will flip between activating their respective nav events on every scroll event. This causes a visual 'flicker', and heavily degrades scroll performance.

Examples:
Minimal example.
Example with nested sections.

I saw this happen in a system of nested sections positioned hard against the top of the document, e.g.

<section id="animals">
  <section id="dogs">
    Content
  </section>
</section>

The bug is that Scrollspy's check to see if it's at the end of the array of sections uses !arr[index + 1]. This misses the case where arr[index + 1] does exist and is zero.

This commit explicitly checks the array bounds. A unit test is included, but I'm not familiar with bootstrap's testing approach so feedback is appreciated.

(Thanks go to @theroux and @tnguyen14!)

twbs-savage · 2015-01-17T14:32:05Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 22f9e30e990a3d09e3cc585c5ddd5a38729ef1a6
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/47350532

(Please note that this is a fully automated comment.)

tnguyen14 · 2015-01-17T21:23:13Z

👍 I ran into this issue yesterday and spent about 5-6 hours trying to debug it. It was really challenging because everything was set up like the doc suggests. I narrowed down the culprit to be using body {margin: 0} set by normalize.css and a :before pseudo-element set by a clearfix technique together.

It took @neoeno an hour reading the source code to figure out the jankiness was happening. It'd be really great to have this fix be incorporated into bootstrap.

cvrebert · 2015-02-26T10:25:05Z

js/tests/unit/scrollspy.js

+    }
+
+    $.when(testElementsAreActiveAfterScroll())
+    .then(function () { return testElementsAreActiveAfterScroll() })


this and the next line should be indented

cvrebert · 2015-02-26T10:29:11Z

LGTM, modulo those nitpicks.
CC: @hnrch02 @XhmikosR for review

neoeno · 2015-02-26T14:39:28Z

Agreed on all counts. Commits added. Can squish before merge (— or not, not sure which is the preferred practice here!)

(offsets[i + 1] === undefined was the required expression though.)

XhmikosR · 2015-02-26T16:02:53Z

@neoeno: yeah, fetch, rebase and squash into one.

kkirsche · 2015-02-26T18:32:45Z

@XhmikosR what's the purpose of fetching before the rebase? Sorry to bring this PR off topic I'm just curious about that. Thanks 😃

XhmikosR · 2015-02-26T18:54:37Z

@kkirsche: that's the only way you can bring everything on par with upstream/master.

kkirsche · 2015-02-26T18:58:26Z

Ah. I always rebase with git rebase -i HEAD~[number of commits we want to squash] [patch-branch name]. That doesn't rebase it against master though. Every time I've tried that the branch ends up with a lot of odd things in it which aren't necessary. Do you have any links to resources I can use to understand how to bring everything to upstream/master?

cvrebert · 2015-02-26T20:49:15Z

For rebasing one branch against another, I've always done git checkout branch-to-rebase && git rebase main-branch-to-rebase-against (after having pulled down the latest master when applicable).

neoeno · 2015-02-26T22:49:16Z

Done. I also updated some of the tests to conform to a newer style that had been brought in by @cvrebert — think I did it right, but I'm new to QUnit so some review would be great!

cvrebert · 2015-02-27T01:49:10Z

js/tests/unit/scrollspy.js

+      var scrollHeight = $content.scrollTop() + 10
+      var done = assert.async()
+      $content.one('scroll', function () {
+        ok($('#li-1').hasClass('active'), 'element' + element)


These need to be assert.ok(

Thanks, done!

neoeno · 2015-02-28T10:09:44Z

@cvrebert Thanks for the comments — I think I understand assert a little better now.

twbs-savage · 2015-02-28T10:10:03Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a9cbfc08c86341a732a758260927ec424e4af5ab
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/52524203

(Please note that this is a fully automated comment.)

cvrebert · 2015-02-28T11:16:14Z

@hnrch02 LGTY?

hnrch02 · 2015-03-01T07:52:27Z

js/tests/unit/scrollspy.js

+
+    $.when(testElementsAreActiveAfterScroll())
+      .then(testElementsAreActiveAfterScroll)
+      .then(testElementsAreActiveAfterScroll)


I don't think this works as you intended - QUnit shows that only two of the planned six assertions are actually made.
If I make this test work like I think you intended (https://gist.github.com/hnrch02/434c11d59153663b999c), the fourth assertion actually fails.

Hi! I'm having a little confusion over this.

The first confusion is that the test in your gist only runs the assertions once. Since on the first pass ++times == 1 and the second pass ++times == 2, which triggers done() before doing an iteration.

When I change the condition to == 4 then the test iterates three times, as the one in this commit does. However, all the assertions pass as expected — I can't seem to reproduce the failure you mention.

I've made a new branch with both tests in, and some hacky logging to show the value of the assertion and the scrollTop. Both tests seem to generate the same output. Here's the branch.

Am I missing something?

I actually think your test, using recursion, looks a fair bit cleaner than this approach using promises. Perhaps at the expense of some readability (for me), but I'm definitely open to either option. I just copied and rewrote the "should add the active class correctly when there are nested elements at 0 scroll offset".

I stand corrected, silly mistake on my end - sorry. The test works fine, would appreciate it though if you replaced this one with the new one from your second branch without the debug code. Thanks!

twbs-savage · 2015-03-01T22:57:42Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 87b0b373d5a444812924188a241390e6396874bc
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/52672695

(Please note that this is a fully automated comment.)

cvrebert · 2015-03-01T23:07:28Z

js/tests/unit/scrollspy.js

+
+    var $content = $(contentHtml)
+      .appendTo('#qunit-fixture')
+      .bootstrapScrollspy({ offset: 0, target: '.navbar' })


For precision, let's use an ID selector here instead of a class selector.

When the first two elements in a scrollspy content block have a document offset of zero (i.e. they're hard against the top of the page), Scrollspy would switch between them on every scroll event. This could happen, for example, in a system of nested sections: ``` <section id="animals"> <section id="dogs"> Content </section> </section> ``` This ocurred because Scrollspy's check to see if it's at the end of the array of sections uses `!arr[index]`. This misses the case where `arr[index]` does exist and is zero. This commit explicitly checks the array bounds.

twbs-savage · 2015-03-01T23:58:38Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a1aa0f8
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/52677790

(Please note that this is a fully automated comment.)

…ollspy_elements Handle multiple zero-offset Scrollspy elements.

cvrebert · 2015-03-02T00:04:46Z

@neoeno Thank you!

neoeno · 2015-03-02T00:07:45Z

My pleasure! ✨

cvrebert added the js label Jan 17, 2015

cvrebert added this to the v3.3.3 milestone Jan 17, 2015

cvrebert reviewed Feb 26, 2015
View reviewed changes

cvrebert reviewed Feb 27, 2015
View reviewed changes

hnrch02 reviewed Mar 1, 2015
View reviewed changes

cvrebert reviewed Mar 1, 2015
View reviewed changes

cvrebert added a commit that referenced this pull request Mar 2, 2015

Merge pull request #15593 from neoeno/handle_multiple_zero_offset_scr…

015e63f

…ollspy_elements Handle multiple zero-offset Scrollspy elements.

cvrebert merged commit 015e63f into twbs:master Mar 2, 2015

cvrebert mentioned this pull request Mar 2, 2015

v3.3.4 ship list #15624

Closed

twbs locked and limited conversation to collaborators Mar 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple zero-offset Scrollspy elements. #15593

Handle multiple zero-offset Scrollspy elements. #15593

neoeno commented Jan 17, 2015

twbs-savage commented Jan 17, 2015

tnguyen14 commented Jan 17, 2015

cvrebert Feb 26, 2015

cvrebert commented Feb 26, 2015

neoeno commented Feb 26, 2015

XhmikosR commented Feb 26, 2015

kkirsche commented Feb 26, 2015

XhmikosR commented Feb 26, 2015

kkirsche commented Feb 26, 2015

cvrebert commented Feb 26, 2015

neoeno commented Feb 26, 2015

cvrebert Feb 27, 2015

neoeno Feb 27, 2015

neoeno commented Feb 28, 2015

twbs-savage commented Feb 28, 2015

cvrebert commented Feb 28, 2015

hnrch02 Mar 1, 2015

neoeno Mar 1, 2015

hnrch02 Mar 1, 2015

neoeno Mar 1, 2015

twbs-savage commented Mar 1, 2015

cvrebert Mar 1, 2015

neoeno Mar 1, 2015

twbs-savage commented Mar 1, 2015

cvrebert commented Mar 2, 2015

neoeno commented Mar 2, 2015

Handle multiple zero-offset Scrollspy elements. #15593

Handle multiple zero-offset Scrollspy elements. #15593

Conversation

neoeno commented Jan 17, 2015

twbs-savage commented Jan 17, 2015

tnguyen14 commented Jan 17, 2015

Choose a reason for hiding this comment

cvrebert commented Feb 26, 2015

neoeno commented Feb 26, 2015

XhmikosR commented Feb 26, 2015

kkirsche commented Feb 26, 2015

XhmikosR commented Feb 26, 2015

kkirsche commented Feb 26, 2015

cvrebert commented Feb 26, 2015

neoeno commented Feb 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neoeno commented Feb 28, 2015

twbs-savage commented Feb 28, 2015

cvrebert commented Feb 28, 2015

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

twbs-savage commented Mar 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twbs-savage commented Mar 1, 2015

cvrebert commented Mar 2, 2015

neoeno commented Mar 2, 2015