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

Allow scrollBehavior selectors to match ids with unescaped CSS special characters (feature request #3008) #3009

Closed
wants to merge 3 commits into from
Closed

Allow scrollBehavior selectors to match ids with unescaped CSS special characters (feature request #3008) #3009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97b7e72
better 'selector' type matching in scrollBehavior
theprojectsomething Oct 24, 2019
6223466
Fix scroll behaviour example for CSS special chars
theprojectsomething Oct 24, 2019
0bce743
Test scroll behaviour example for CSS special chars
theprojectsomething Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions examples/scroll-behavior/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const Bar = {
<p id="anchor" style="height:500px">Anchor</p>
<p id="anchor2" style="height:500px">Anchor2</p>
<p id="1number">with number</p>
<p id="1number/2number">with CSS special characters</p>
</div>
`
}
Expand All @@ -37,8 +38,8 @@ const scrollBehavior = function (to, from, savedPosition) {
position.offset = { y: 100 }
}

// bypass #1number check
if (/^#\d/.test(to.hash) || document.querySelector(to.hash)) {
// bypass #1number && #1number/2number check
if (/^#[^, ]+$/.test(to.hash) || document.querySelector(to.hash)) {
return position
}

Expand Down Expand Up @@ -89,6 +90,7 @@ new Vue({
<li><router-link to="/bar#anchor">/bar#anchor</router-link></li>
<li><router-link to="/bar#anchor2">/bar#anchor2</router-link></li>
<li><router-link to="/bar#1number">/bar#1number</router-link></li>
<li><router-link to="/bar#1number/2number">/bar#1number/2number</router-link></li>
</ul>
<transition name="fade" mode="out-in" @after-leave="afterLeave">
<router-view class="view"></router-view>
Expand Down
6 changes: 4 additions & 2 deletions src/util/scroll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,16 @@ function isNumber (v: any): boolean {
return typeof v === 'number'
}

const hashStartsWithNumberRE = /^#\d/
const selectorLooksLikeIdRE = /^#[^, ]+$/
Copy link
Member

Choose a reason for hiding this comment

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

why any character but , and ?

Copy link
Author

@theprojectsomething theprojectsomething Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@posva a , or suggests a complex selector (either nested or an OR). Technically an id selector could contain an , but the above provides the best balance of simplicity vs. coverage without breaking changes. Consider:

// Simple selector (passed to getElementById):
#one
#1
#one/two

// Complex selector (passed to querySelector): 
#one .two
#one, .two
#one\,two // selects id="one,two"

Did you have any suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @posva any update on this PR?


function scrollToPosition (shouldScroll, position) {
const isObject = typeof shouldScroll === 'object'
if (isObject && typeof shouldScroll.selector === 'string') {
// getElementById is used only where a selector looks like a single [id] ... this allows for use of CSS
// 'special characters' that would otherwise fail in querySelector without escaping, e.g. "#one/two"
// getElementById would still fail if the selector contains a more complicated query like #main[data-attr]
// but at the same time, it doesn't make much sense to select an element with an id and an extra selector
const el = hashStartsWithNumberRE.test(shouldScroll.selector) // $flow-disable-line
const el = selectorLooksLikeIdRE.test(shouldScroll.selector) // $flow-disable-line
? document.getElementById(shouldScroll.selector.slice(1)) // $flow-disable-line
: document.querySelector(shouldScroll.selector)

Expand Down
12 changes: 11 additions & 1 deletion test/e2e/specs/scroll-behavior.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module.exports = {
browser
.url('http://localhost:8080/scroll-behavior/')
.waitForElementVisible('#app', 1000)
.assert.count('li a', 6)
.assert.count('li a', 7)
.assert.containsText('.view', 'home')

.execute(function () {
Expand Down Expand Up @@ -127,6 +127,16 @@ module.exports = {
null,
'scroll to anchor that starts with number'
)
.execute(function () {
document.querySelector('li:nth-child(7) a').click()
})
.assert.evaluate(
function () {
return document.getElementById('1number/2number').getBoundingClientRect().top < 1
},
null,
'scroll to anchor that includes CSS special characters'
)
.end()
}
}