Skip to content

Let ScrollSpy support uri encoded hash fragments#14740

Closed
yuku wants to merge 3 commits intotwbs:masterfrom
yuku:scrollspy-decode-hash
Closed

Let ScrollSpy support uri encoded hash fragments#14740
yuku wants to merge 3 commits intotwbs:masterfrom
yuku:scrollspy-decode-hash

Conversation

@yuku
Copy link

@yuku yuku commented Oct 7, 2014

The updated line threw a syntax error when an encoded fragment is given.

href = encodeURIComponent('#テスト')
$(href)
// Error: Syntax error, unrecognized expression: %23%E3%83%86%E3%82%B9%E3%83%88

And browsers (at least Chrome 37.0.2062.124 and Firefox 32.0.3) automatically encodes a[href] values due to rfc3986:

<!-- server response -->
<a href="#テスト"></a>
<div id="テスト"></div>

<!-- browser recognizes as -->
<a href="#%23%E3%83%86%E3%82%B9%E3%83%88"></a>
<div id="テスト"></div>

This means that there is no way to use the plugin with non-ascii hash fragments. This PR fixes the problem.

Patched script works fine on my pages, however, I'm afraid of breaking something important because I'm not familiar with such encoding problems 😨
Thanks.

@cvrebert
Copy link
Collaborator

cvrebert commented Oct 7, 2014

Please add a unit test for this feature.

@hnrch02 hnrch02 added the js label Oct 7, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 7, 2014

This would probably be necessary for other plugins too.

@yuku
Copy link
Author

yuku commented Oct 7, 2014

@cvrebert Added a unit test. Please review fbf44e8

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use four spaces for indentation in front of these pluses.

@yuku
Copy link
Author

yuku commented Oct 7, 2014

@hnrch02 Done. If I should merge these three commits into a single commit, please let me know.

@cvrebert
Copy link
Collaborator

@hnrch02 Should we group this in with #14789?

@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 29, 2014

@cvrebert Yeah, I think so.

@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 29, 2014

Closing this per my answer in #14789 (comment). We'll try to support all kinds of exotic (but still valid) IDs in v4. Thanks for the work though!

@hnrch02 hnrch02 closed this Oct 29, 2014
@yuku yuku deleted the scrollspy-decode-hash branch October 29, 2014 03:27
@mdo mdo mentioned this pull request Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants