Skip to content

Let ScrollSpy support uri encoded hash fragments - #14740

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

yuku wants to merge 3 commits into
twbs:masterfrom
yuku:scrollspy-decode-hash

Conversation

@yuku

@yuku yuku commented Oct 7, 2014

Copy link
Copy Markdown

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

cvrebert commented Oct 7, 2014

Copy link
Copy Markdown
Collaborator

Please add a unit test for this feature.

@hnrch02 hnrch02 added the js JavaScript or TypeScript sources and plugins label Oct 7, 2014
@hnrch02

hnrch02 commented Oct 7, 2014

Copy link
Copy Markdown
Collaborator

This would probably be necessary for other plugins too.

@yuku

yuku commented Oct 7, 2014

Copy link
Copy Markdown
Author

@cvrebert Added a unit test. Please review fbf44e8

Comment thread js/tests/unit/scrollspy.js Outdated

Copy link
Copy Markdown
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

yuku commented Oct 7, 2014

Copy link
Copy Markdown
Author

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

@cvrebert

Copy link
Copy Markdown
Collaborator

@hnrch02 Should we group this in with #14789?

@hnrch02

hnrch02 commented Oct 29, 2014

Copy link
Copy Markdown
Collaborator

@cvrebert Yeah, I think so.

@hnrch02

hnrch02 commented Oct 29, 2014

Copy link
Copy Markdown
Collaborator

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

js JavaScript or TypeScript sources and plugins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants