Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Scrollspy does not handle all valid HTML5 element ids #34381

Closed
chrisyeh96 opened this issue Jun 30, 2021 · 2 comments · Fixed by #35566
Closed

Scrollspy does not handle all valid HTML5 element ids #34381

chrisyeh96 opened this issue Jun 30, 2021 · 2 comments · Fixed by #35566
Labels
has-pr js v5

Comments

@chrisyeh96
Copy link

chrisyeh96 commented Jun 30, 2021 •
edited
Loading

Bug Report

Scrollspy stops working when any of the DOM elements it is "spying" on has an id that starts with a number, such as

<div id="3rd">Text</div>

This should be considered a bug because HTML5 has no restrictions on what form an ID can take (official spec).

There are no other restrictions on what form an ID can take; in particular, IDs can consist of just digits, start with a digit, start with an underscore, consist of just punctuation, etc.

Historically, HTML4 did not allow for ids to start with numbers, but it would seem ridiculous to assume only HTML4 content today.

Bug Demo

See this JSBin for the demo. https://jsbin.com/rehematazo/3/edit?html,output. By default, I've commented out the <a> elements that link to <div>s whose IDs start with a digit, so Scrollspy works. But once you uncomment out these <a> tags, then Scrollspy stops working entirely.

I have also copied the HTML code for the demo here:

Code 
<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>JS Bin</title>
  <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-EVSTQN3/azprG1Anm3QDgpJLIm9Nao0Yz1ztcQTwFspd3yD65VohhpuuCOmLASjC" crossorigin="anonymous">
  <style>
    .tall { height: 300px; }
    
    .toc-nav { /* <nav> */
      position: sticky;
      top: 0;
      height: min-content;
      max-height: 100vh;
      overflow-y: auto;
    }
    
    .toc-nav a.active {
      color: #007bff;
    }
  </style>
</head>
<body data-bs-spy="scroll" data-bs-target="#nav">

  <div class="container"><div class="row">

  <main class="col-8">
  <h2 id="A">ID A</h2>
  <div class="tall">Chunk of text</div>
  <h2 id="B">ID B</h2>
  <div class="tall">Chunk of text</div>
  <h2 id="C">ID C</h2>
  <div class="tall">Chunk of text</div>

  <h2 id="1">ID 1</h2>
  <div class="tall">Chunk of text</div>
  <h2 id="2">ID 2</h2>
  <div class="tall">Chunk of text</div>
  <h2 id="3">ID 3</h2>
  <div class="tall">Chunk of text</div>
  </main>
  
  <nav class="col-4 toc-nav" id="nav">
    <nav class="navbar navbar-light">
      <span class="navbar-brand">Contents</span>
      <a class="list-group-item list-group-item-action" href="#A">Item A</a>
      <a class="list-group-item list-group-item-action" href="#B">Item B</a>
      <a class="list-group-item list-group-item-action" href="#C">Item C</a>

      <!-- Uncomment the following 3 lines to see the bug:
           Once you uncomment, you will notice that Scrollspy stops working.
        -->
      <!-- <a class="list-group-item list-group-item-action" href="#1">Item 1</a>
      <a class="list-group-item list-group-item-action" href="#2">Item 2</a>
      <a class="list-group-item list-group-item-action" href="#3">Item 3</a> -->
    </nav>
  </nav>

  </div></div>

  <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.min.js" integrity="sha384-cVKIPhGWiC2Al4u+LWgxfKTRIcfu0JTxR+EQDz/bgldoEyl4H0zUF0QKbrJ0EcQF" crossorigin="anonymous"></script>
</body>
</html>

Why this happens, and possible fixes

This bug seems to happen because Bootstrap uses document.querySelector() to identify elements:

bootstrap/js/dist/scrollspy.js

Line 78 in 696996d

return document.querySelector(selector) ? selector : null;

However, document.querySelector() only works for valid CSS selector strings, and CSS selector strings for IDs cannot start with a number (even though HTML IDs can). See this StackOverflow Q&A for more info: https://stackoverflow.com/q/20306204. This causes Bootstrap JS to encounter the following error (copied from Microsoft Edge console):

Uncaught DOMException: Failed to execute 'querySelector' on 'Document': '#3rd' is not a valid selector.
    at <anonymous>:1:10

The StackOverflow Q&A proposes several possible fixes. For example, document.getElementById("3rd") works fine, but only works with ID-based selectors. I am also not sure which fixes fall within Bootstrap's browser compatibility goals.

System Info

OS: Windows 10 20H1
Browser: Microsoft Edge (Chromium) 91.0.864.59 and Firefox 89.0.2 (64-bit)
Bootstrap version 5.0.2
@ffoodd
Copy link
Member

ffoodd commented Jun 30, 2021

Same topic than #31099 and a few others— this is a concern for all components. Any PR is welcome, but FWIW the HTML spec evolved but browsers and other specs (eg CSS or DOM) took some time to close the gap.

@chrisyeh96
Copy link
Author

chrisyeh96 commented Jun 30, 2021 •
edited
Loading

@ffoodd Thanks for pointing out the related issue.

Given that Scrollspy is only meant to point to elements with specific IDs, can we use document.getElementById() instead of document.querySelector()? Put another way, why does the code use document.querySelector() at all?

I'm tagging @XhmikosR, who seems to have been the last person to touch this code.

This was referenced Jul 3, 2021
Closed
Closed
@GeoSot GeoSot mentioned this issue Dec 17, 2021
Merged
@mdo mdo added the has-pr label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
has-pr js v5
Projects
None yet
Milestone
No milestone
3 participants
@mdo @ffoodd @chrisyeh96