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.

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

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

Closed
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 as a code owner Jan 20, 2019
@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from c665584 to 6e63c1d Compare Jan 20, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 23, 2019
cunla
cunla approved these changes Apr 5, 2020
@pullapprove pullapprove bot requested a review from pkozlowski-opensource Apr 5, 2020
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented May 28, 2020

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 wKoza commented May 28, 2020

I can update this PR shortly

Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

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 target: patch This PR is targeted for the next patch release labels Jul 14, 2020
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 9, 2020

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

@wKoza
Copy link
Contributor Author

@wKoza 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 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 Nov 11, 2020
@google-cla
Copy link

@google-cla 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 Nov 11, 2020
@google-cla
Copy link

@google-cla 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 Nov 11, 2020
@petebacondarwin petebacondarwin requested review from petebacondarwin and removed request for kyliau, alxhub, gkalpak and AndrewKushnir Nov 11, 2020
@petebacondarwin petebacondarwin dismissed pkozlowski-opensource’s stale review Nov 11, 2020

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

@petebacondarwin petebacondarwin added action: presubmit A standard presubmit is running / required action: review and removed action: cleanup labels Nov 11, 2020
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 11, 2020

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

Copy link
Contributor

@atscott atscott left a comment

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 Outdated Show resolved Hide resolved
@@ -166,7 +166,7 @@ export class BrowserViewportScroller implements ViewportScroller {

private supportsScrolling(): boolean {
Copy link
Contributor

@atscott atscott Nov 13, 2020

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

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 Nov 15, 2020
…llX / scrollY

This fix ensures a better cross-browser compatibility.
This fix has been used for angular.io.
@wKoza wKoza force-pushed the fix_supportScrollRestoration branch from b38df64 to 9755ef2 Compare Nov 15, 2020
@atscott atscott removed the action: presubmit A standard presubmit is running / required label Nov 19, 2020
@atscott
Copy link
Contributor

@atscott atscott commented Nov 19, 2020

global presubmit

@atscott atscott added action: merge PR author is ready for this to merge and removed action: review labels Nov 19, 2020
AndrewKushnir pushed a commit that referenced this issue Nov 19, 2020
…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
Copy link

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

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
@pullapprove pullapprove bot added comp: docs-infra The a.io application & docs infra (use comp: docs for content) comp: router labels 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.
Labels
action: merge PR author is ready for this to merge cla: yes comp: common comp: docs-infra The a.io application & docs infra (use comp: docs for content) comp: router target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants