Scrollspy selecting the last element when at the bottom of the page #18804
Conversation
|
Please add a relevant JS unit test or visual test for your change; see https://github.com/twbs/bootstrap/tree/v4-dev/js/tests for instructions. |
| @@ -212,6 +217,7 @@ const ScrollSpy = (($) => { | |||
| if (this._activeTarget !== target) { | |||
| this._activate(target) | |||
| } | |||
| return | |||
There was a problem hiding this comment.
Because there is a case when scrollTop is lower than offset of the last target and there is no more space to scroll down in this._scrollElement. In this case this._activeTarget would get overwritten to the last but one target in line 235 if there is no return statement in this line.
There was a problem hiding this comment.
Ah, sorry, I was on mobile and it looked like this was already the end of the function (obviously it's not).
|
@cvrebert is the visual test enough for now? |
| @@ -28,6 +28,7 @@ | |||
| <a class="dropdown-item" href="#three" tabindex="-1">three</a> | |||
| </div> | |||
| </li> | |||
| <li class="nav-item"><a class="nav-link" href="#four">@four</a></li> | |||
There was a problem hiding this comment.
Change this to something like "Selected when page scrolled to bottom", so as to explicitly give the test expectation.
|
What's left to do before this can be merged in? |
|
All comments should be implemented. @timfish Sorry, I missed the notifications. I believe nothing is left to do before the merge anymore. |
|
So, this has the side-effect of making the unselectability of middle items on the last screen noticeable. Which also feels weird from a user perspective. Here's an example: Notice how "2nd to last" is unreachable. I'm not sure how we ought to handle that. |
|
Still slightly strange behaviour but not sure how else it should work and doesn't actually appear to be a regression from v3: |
|
That's a though UX decision—I'm not sure about this either. But this PR restores the original behavior from v2 and v3 so I'd say 👍 . |
Fixes #17739