prev() function fails when non-element nodes are present

[MartijnCuppens]

The prev() function doesn't take nodes other than elements into account. Also we could simplify things a lot using the previousElementSibling property. This property isn't fully supported in IE, it only works for elements, but since the element variable is an element, we can safely use it here.

I've also added an additional test.

I don't think we had this issue in v4, since we relied on jQuery back then.

Ref. https://developer.mozilla.org/en-US/docs/Web/API/NonDocumentTypeChildNode/nextElementSibling

---

• v4 (jQuery): https://getbootstrap.com/docs/4.4/components/scrollspy/#example-with-nested-nav
• v5 (broken): https://twbs-bootstrap.netlify.com/docs/4.3/components/scrollspy/#example-with-nested-nav
• v5 (fixed): https://deploy-preview-30117--twbs-bootstrap.netlify.com/docs/4.3/components/scrollspy/#example-with-nested-nav

[MartijnCuppens]

This probably fixes a bug in scrollspy so I moved this back to the v5 board. @XhmikosR, could you have a look at this? The test basically illustrates what the problem was.

[XhmikosR]

@MartijnCuppens is there any issue about this or failing snippets? Trying to understand why we need to make this change.

[MartijnCuppens]

Took a while to figure out where/how this was used, but i found what was going wrong, see
v4 (jQuery): https://getbootstrap.com/docs/4.4/components/scrollspy/#example-with-nested-nav
v5 (broken): https://twbs-bootstrap.netlify.com/docs/4.3/components/scrollspy/#example-with-nested-nav
v5 (fixed): https://deploy-preview-30117--twbs-bootstrap.netlify.com/docs/4.3/components/scrollspy/#example-with-nested-nav

[XhmikosR]

Actually this patch seems broken for me... Using Firefox 73.0.1

The master branch seems fine here.

[MartijnCuppens]

It seems to be broken after the last merge, gonna check what happend there

[MartijnCuppens]

It seems to be broken after the last merge

Lol, that was a lie, I just didn't scroll to the bottom to test this.

Anyway, I finally get why prev() returns an array. It's just easier to loop over the array which contains zero or one items. This way we avoid the check if the element is found.

@XhmikosR, can you recheck?

[XhmikosR]

It seems to work now, but maybe we should keep siblings like the original code?

[MartijnCuppens]

It seems to work now, but maybe we should keep siblings like the original code?

Why? previousSibling also targets comments and text elements which we do not need? (Also not sure what happens when we apply matches() to it)

[XhmikosR]

I meant just keep the variable and return siblings like before.

[XhmikosR]

But it's probably moot and the minifier might already drop this. I'll double check later.

[Johann-S]

Nice work @MartijnCuppens 👍