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

fix(common): escape anchor for some anchors containing characters whi… #28193

Closed
wants to merge 1 commit into from
Closed

fix(common): escape anchor for some anchors containing characters whi… #28193

wants to merge 1 commit into from

Conversation

kyeongmincho
Copy link

…ch is used for CSS

If there are some characters('.', ':', '>', etc.) which are for a part
of CSS selector in the anchor string, document.querySelector() throw
an error like below:

ERROR DOMException: Failed to execute 'querySelector'
on 'Document': '#fn:1' is not a valid selector.

So it should be escaped.
Ref: https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape#In_context_uses

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • [*] No

Other information

@kyeongmincho
fix(common): escape anchor for some anchors containing characters whi…
ebb3bc7 
…ch is used for CSS

If there are some characters('.', ':', '>', etc.)  which are for a part
of CSS selector in the anchor string, document.querySelector() throw
an error like below:

> ERROR DOMException: Failed to execute 'querySelector'
> on 'Document': '#fn:1' is not a valid selector.

So it should be escaped.
@kyeongmincho kyeongmincho requested a review from a team as a code owner January 16, 2019 21:22
alfaproject
alfaproject reviewed Jan 16, 2019
View reviewed changes
Copy link
Contributor

@alfaproject alfaproject left a comment

Choose a reason for hiding this comment

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

You also forgot to add unit tests ):

packages/common/src/viewport_scroller.ts
@@ -105,6 +105,7 @@ export class BrowserViewportScroller implements ViewportScroller {
* @param anchor The ID of the anchor element.
*/
scrollToAnchor(anchor: string): void {
anchor = CSS.escape(anchor);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work in all Angular supported browsers. Check the link you referenced for browser support

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this doesn't seem to be a role of Scroller, if Router allows special characters in fragment, then itself should handle the escaping (or just filter out invalid fragments).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I don't think this should be added to the Scroller. We assume a properly formatted anchor value for the platform you're on here.

@jasonaden jasonaden added the area: common Issues related to APIs in the @angular/common package label Jan 23, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 23, 2019
@jasonaden
Copy link
Contributor

I like the idea on this, however, CSS.escape() is a working draft proposal. Adding the polyfill is too much code to add to Angular when many apps wouldn't even need it since it depends on the format of these strings in a particular app.

I think the right answer here is for any application with this problem to provide their own ViewportScroller. It inject Angular's ViewportScroller, deferring to all the methods there, but format the anchor before calling scrollToAnchor.

Thoughts?

jasonaden added a commit to jasonaden/angular that referenced this pull request Mar 28, 2019
@jasonaden
fix(common): escape query selector used when anchor scrolling
855df7b 
When an anchor scroll happens, we run document.querySelector. This value can be taken directly from the user. Therefore it's possible to throw an error on scrolling, which can cause the application to fail.

This PR escapes the selector before using it.

Related to angular#28193
[Internal discussion](https://groups.google.com/a/google.com/forum/#!topic/angular-users/d82GHfmRKLc)
@jasonaden jasonaden mentioned this pull request Mar 28, 2019
Closed
14 tasks
@jasonaden
Copy link
Contributor

Thanks for this PR. I'm going to go ahead and close this one since I created a new one that has a backup for when CSS.escape isn't provided, as well as a test. Thanks for bringing up this issue!

#29577

@jasonaden jasonaden closed this Mar 28, 2019
jasonaden added a commit that referenced this pull request Mar 29, 2019
@jasonaden
fix(common): escape query selector used when anchor scrolling (#29577)
e8768ac 
When an anchor scroll happens, we run document.querySelector. This value can be taken directly from the user. Therefore it's possible to throw an error on scrolling, which can cause the application to fail.

This PR escapes the selector before using it.

Related to #28193
[Internal discussion](https://groups.google.com/a/google.com/forum/#!topic/angular-users/d82GHfmRKLc)

PR Close #29577
jasonaden added a commit that referenced this pull request Mar 29, 2019
@jasonaden
fix(common): escape query selector used when anchor scrolling (#29577)
7671c73 
When an anchor scroll happens, we run document.querySelector. This value can be taken directly from the user. Therefore it's possible to throw an error on scrolling, which can cause the application to fail.

This PR escapes the selector before using it.

Related to #28193
[Internal discussion](https://groups.google.com/a/google.com/forum/#!topic/angular-users/d82GHfmRKLc)

PR Close #29577
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@jasonaden @wKoza
fix(common): escape query selector used when anchor scrolling (angula…
004da4b 
…r#29577)

When an anchor scroll happens, we run document.querySelector. This value can be taken directly from the user. Therefore it's possible to throw an error on scrolling, which can cause the application to fail.

This PR escapes the selector before using it.

Related to angular#28193
[Internal discussion](https://groups.google.com/a/google.com/forum/#!topic/angular-users/d82GHfmRKLc)

PR Close angular#29577
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfaproject alfaproject alfaproject left review comments

@jasonaden jasonaden jasonaden left review comments

@trotyl trotyl trotyl left review comments

Assignees
No one assigned
Labels
area: common Issues related to APIs in the @angular/common package cla: yes
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kyeongmincho @jasonaden @alfaproject @trotyl @googlebot