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.

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

Fix selector finding the target matching HTML5 specification #66

Closed
wants to merge 1 commit into from
Closed

Conversation

martinhartmann
Copy link

The current version fails for valid HTML 5 anchors which could be like this:

<p id="this_(should_be_a_valid)_anchor:according_&quot;to_Spec">HTML5 Anchor</p>

The reason for this is the way it tries to find the anchors in function getNavs(). This pull request fixes this problem with those anchors.

The spec says:

The value must be unique amongst all the IDs in the element's home subtree and must contain at least one character. The value must not contain any space characters.

Note: 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.

The spec (https://www.w3.org/TR/html5/dom.html#the-id-attribute) says:

The value must be unique amongst all the IDs in the element's home subtree and must contain at least one character. The value must not contain any space characters.
Note: 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.
@cferdinandi
Copy link
Owner

Can you explain a bit more about what's happening here, and what the purpose of this PR is? Thanks!

@martinhartmann
Copy link
Author

The problem

Today I tried to implement gumshoe in one of our projects and stumbled upon the following error in Chrome:

Uncaught DOMException: Failed to execute 'querySelector' on 'Document': '#Sicherheitsfolie(Dokumentenfolie),manipulationssicher' is not a valid selector.

It's a shop and the problem occurs in the product listing. Products are split into groups and for every group and product i wanted to have an entry in the scrollspy.

The structure for example is as follows:

<!-- Group -->
<a href="#Standardfolie">Standardfolie</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

<!-- Group, problematic -->
<a href="#Sicherheitsfolie-(Dokumentenfolie),-manipulationssicher">Sicherheitsfolie (Dokumentenfolie), manipulationssicher</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

<!-- Group -->
<a href="#Standardfolie-auf-Rolle">Standardfolie auf Rolle</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

<!-- Group -->
<a href="#Sicherheitsfolie-mit-Schutzlaminat,-auf-Rolle">Sicherheitsfolie mit Schutzlaminat, auf Rolle</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

<!-- Group -->
<a href="#Standardfolie-auf-Bogen">Standardfolie auf Bogen</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

<!-- Group, problematic -->
<a href="#Sicherheitsfolie-(Dokumentenfolie),-manipulationssicher-auf-Bogen">Sicherheitsfolie (Dokumentenfolie), manipulationssicher auf Bogen</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>
	<a href="#...">Product...</a>

You can think of many many more strings, for instance:

  • "Defekt! Keine weitere Benutzung!", Standardfolie
  • 3M-Folie Reflexfolie Typ1, selbstklebend, mit rückseitigem Trägerpapier/Abdeckpapier
  • Aluminium 0,4 mm nachleuchtend nach DIN 67510 160 mcd/m²
  • Aluminium, VE = 1 Satz (rechts- + linksweisend)
  • Bündel-Ø 16 mm
  • Endlosband, Vinyl blau, Länge 15 Meter (B595)
  • Papier-Etiketten weiß, Ausführung permanent klebend, Temperatur bis zu +50 °C
  • ...

So i wanted to find out which characters i could use in HTML5 for the anchors. As it turned out, i could use anything except spaces. So i wrote a function on server side to properly format these strings into valid attributes but i got stopped whilst implementing the scrollspy on client side with gumshoe.

The problematic part of it is the function document.querySelector which obviously can't handle such (according to the spec) valid values. It would result in a

document.querySelector( "#Sicherheitsfolie-(Dokumentenfolie),-manipulationssicher" )

whereas Chrome 60.0.3104.0, Edge 15.15063 and Firefox 54.0a2 have problems with (the error Chrome gives is listed at the beginning). Of course this may be the problem of the browsers which in my opinion are not following the spec. But I don't want to manipulate these group names and remove some characters, introduce a blacklist or a translation for special characters (Unicode anyone?) or anything else weird. I also wanted to keep the real name in the URL.

By the way &lpar;, &#x00028; and &#40; also don't work.

The fix

Changing

document.querySelector( "#..." )

into

document.querySelector( "[id='#...']" )

And it just works.

@cferdinandi
Copy link
Owner

Looking at this, I think you've actually touched on a bigger issue, similar to one I found in Smooth Scroll.

I need to do two things to properly fix this:

  1. Apply decodeURIComponent() on the hash.
  2. Escape valid but confusing characters that cause querySelector issues: https://github.com/cferdinandi/smooth-scroll/blob/master/dist/js/smooth-scroll.js#L140-L218

Your fix while much nicer and simpler, doesn't address the full scope of the issue.

BUT, thank you SO much for helping me find this bug! Moving this to an issue.

@slavaGanzin
Copy link

@cferdinandi Why this wasn't merged, even as temporary soution?

p.s. I'd reinvented same and wanted to make PR :)

@cferdinandi
Copy link
Owner

The new v4.x release of Gumshoe should fix this issue.

@cferdinandi cferdinandi closed this Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants