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

wKoza · 2019-01-20T13:47:42Z

closes #27957

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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

petebacondarwin · 2020-05-28T09:40:44Z

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

wKoza · 2020-05-28T09:53:04Z

I can update this PR shortly

pkozlowski-opensource

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 · 2020-11-09T15:08:13Z

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

wKoza · 2020-11-09T15:18:46Z

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

google-cla · 2020-11-11T13:13:44Z

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 · 2020-11-11T13:17:03Z

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.

google-cla · 2020-11-11T13:20:32Z

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.

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

petebacondarwin · 2020-11-11T20:21:43Z

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

atscott

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/router/test/bootstrap.spec.ts

atscott · 2020-11-13T21:32:46Z

packages/common/src/viewport_scroller.ts

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...

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

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

…of scrollX / scrollY

atscott · 2020-11-19T17:53:09Z

global presubmit

…llX / scrollY (#28262) This fix ensures a better cross-browser compatibility. This fix has been used for angular.io. PR Close #28262

angular-automatic-lock-bot · 2020-12-20T16:38:31Z

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.}

wKoza requested a review from a team as a code owner January 20, 2019 13:47

googlebot added the cla: yes label Jan 20, 2019

wKoza force-pushed the fix_supportScrollRestoration branch from c665584 to 6e63c1d Compare January 20, 2019 13:59

jasonaden added the area: common Issues related to APIs in the @angular/common package label Jan 23, 2019

ngbot bot added this to the needsTriage milestone Jan 23, 2019

cunla approved these changes Apr 5, 2020

View reviewed changes

pullapprove bot requested a review from pkozlowski-opensource April 5, 2020 13:20

pkozlowski-opensource previously requested changes Jun 17, 2020

View reviewed changes

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

pullapprove bot requested review from AndrewKushnir, IgorMinar, alxhub, atscott, gkalpak, jelbourn, kyliau and mhevery November 9, 2020 20:21

google-cla bot added cla: no and removed cla: yes labels Nov 11, 2020

wKoza force-pushed the fix_supportScrollRestoration branch from 39eba46 to 1371940 Compare November 11, 2020 13:16

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

wKoza force-pushed the fix_supportScrollRestoration branch from 50f8eeb to e356b5d Compare November 11, 2020 13:23

petebacondarwin requested review from petebacondarwin and removed request for AndrewKushnir, alxhub, gkalpak and kyliau November 11, 2020 20:19

petebacondarwin approved these changes Nov 11, 2020

View reviewed changes

atscott approved these changes Nov 13, 2020

View reviewed changes

wKoza added 2 commits November 15, 2020 10:54

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.

fixup! fix(common): Prefer to use pageXOffset / pageYOffset instance …

9755ef2

…of scrollX / scrollY

wKoza force-pushed the fix_supportScrollRestoration branch from b38df64 to 9755ef2 Compare November 15, 2020 09:55

atscott removed the action: presubmit The PR is in need of a google3 presubmit label Nov 19, 2020

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 closed this in b1d300d Nov 19, 2020

AndrewKushnir pushed a commit that referenced this pull request Nov 19, 2020

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 mentioned this pull request Nov 19, 2020

ViewportScroller getScrollPosition does not work in Internet Explorer #27957

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 20, 2020

pullapprove bot added comp: docs-infra area: router labels Dec 20, 2020

ngbot bot modified the milestones: needsTriage, Backlog Dec 20, 2020

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

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

Uh oh!

Conversation

wKoza commented Jan 20, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

petebacondarwin commented May 28, 2020

Uh oh!

wKoza commented May 28, 2020

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Nov 9, 2020

Uh oh!

wKoza commented Nov 9, 2020

Uh oh!

google-cla bot commented Nov 11, 2020

Uh oh!

google-cla bot commented Nov 11, 2020

Uh oh!

google-cla bot commented Nov 11, 2020

Uh oh!

petebacondarwin commented Nov 11, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atscott Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

wKoza Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atscott commented Nov 19, 2020

Uh oh!

angular-automatic-lock-bot bot commented Dec 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wKoza Nov 15, 2020 •

edited

Loading