Skip to content

fix(common): make more restrictive the supportScrollRestoration method #28262

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
Closed
wants to merge 2 commits into from
Closed

fix(common): make more restrictive the supportScrollRestoration method #28262

wants to merge 2 commits into from

Conversation

wKoza
Copy link
Contributor

@wKoza wKoza commented Jan 20, 2019

closes #27957

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Currently, ViewportScroller does not work in Internet Explorer because of the use of scrollX and scrollY. Neither of which are supported by Internet Explorer.

Issue Number: #27957

What is the new behavior?

We check these properties with the supportScrollRestoration method before calling them in getScrollPosition()

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@wKoza wKoza requested a review from a team as a code owner January 20, 2019 13:47
@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from c665584 to 6e63c1d Compare January 20, 2019 13:59
@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
@petebacondarwin
Copy link
Contributor

I feel that a better solution would be to use pageXOffset and pageYOffset which are available on IE9-11. See #27957 (comment)

@wKoza
Copy link
Contributor Author

wKoza commented May 28, 2020

I can update this PR shortly

pkozlowski-opensource
pkozlowski-opensource previously requested changes Jun 17, 2020
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

I can update this PR shortly

@wKoza take your time. Also, please make sure that we've got a test exposing the problem we are trying to fix here (we do run test on IE).

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Jul 14, 2020
@petebacondarwin
Copy link
Contributor

@wKoza - are you still interested in fixing this issue?

@wKoza
Copy link
Contributor Author

wKoza commented Nov 9, 2020

@petebacondarwin , It was out of my mind but i can do that this week.

@google-cla
Copy link

google-cla bot commented Nov 11, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 11, 2020
@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from 39eba46 to 1371940 Compare November 11, 2020 13:16
@google-cla
Copy link

google-cla bot commented Nov 11, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from 1371940 to 50f8eeb Compare November 11, 2020 13:20
@google-cla
Copy link

google-cla bot commented Nov 11, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from 50f8eeb to e356b5d Compare November 11, 2020 13:23
@petebacondarwin petebacondarwin requested review from petebacondarwin and removed request for kyliau, alxhub, gkalpak and AndrewKushnir November 11, 2020 20:19
@petebacondarwin petebacondarwin dismissed pkozlowski-opensource’s stale review November 11, 2020 20:20

The PR has moved a lot since this and PK is not active at the moment.

@petebacondarwin petebacondarwin added action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 11, 2020
@petebacondarwin
Copy link
Contributor

Nice work @wKoza - I have added @atscott as reviewer because he can cover both fw-core and fw-router.

atscott
atscott approved these changes Nov 13, 2020
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

LGTM. Given that pageYOffset is just an alias for scrollY with better support (namely, in IE), this seems fine aside from the small concern that this could break a few tests.

packages/common/src/viewport_scroller.ts
@@ -166,7 +166,7 @@ export class BrowserViewportScroller implements ViewportScroller {

private supportsScrolling(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's really any value in this anymore. I wouldn't remove it at this point, but it appears that pageXOffset and scrollTo is supported in every browser that we support...

Copy link
Contributor Author

@wKoza wKoza Nov 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

scrollTo doesn't seem to work with IE 11 that is supported by Angular.

wKoza added 2 commits November 15, 2020 10:54
@wKoza
fix(common): Prefer to use pageXOffset / pageYOffset instance of scro…
a20fec1 
…llX / scrollY

This fix ensures a better cross-browser compatibility.
This fix has been used for angular.io.
@wKoza
fixup! fix(common): Prefer to use pageXOffset / pageYOffset instance …
9755ef2 
…of scrollX / scrollY
@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from b38df64 to 9755ef2 Compare November 15, 2020 09:55
@atscott atscott removed the action: presubmit The PR is in need of a google3 presubmit label Nov 19, 2020
@atscott
Copy link
Contributor

atscott commented Nov 19, 2020

global presubmit

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 19, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 19, 2020
@wKoza @AndrewKushnir
fix(common): Prefer to use pageXOffset / pageYOffset instance of scro…
5692607 
…llX / scrollY (#28262)

This fix ensures a better cross-browser compatibility.
This fix has been used for angular.io.

PR Close #28262
@AndrewKushnir AndrewKushnir mentioned this pull request Nov 19, 2020
Closed
@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 Dec 20, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@atscott atscott atscott approved these changes

@cunla cunla cunla approved these changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource left review comments

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package area: router cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

ViewportScroller getScrollPosition does not work in Internet Explorer
7 participants
@wKoza @petebacondarwin @atscott @pkozlowski-opensource @cunla @jasonaden @googlebot