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(overlay): flexible overlay with push not handling scroll offset and position locking #12624

Conversation

crisbeto
Copy link
Member

This is a resubmit of #11628.

  • Fixes the position of flexible overlays with pushing enabled being thrown off once the user starts scrolling.
  • Fixes flexible overlays with pushing not handling locked positioning. With these changes locked overlays will only be pushed when they're opened, whereas non-locked overlays will stay in the viewport, even when the user scrolls down.
  • Fixes a potential issue due to a couple of variables being initialized together where one is set to zero and the other one is undefined.

Fixes #11365.

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Aug 10, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 10, 2018
@jelbourn
Copy link
Member

jelbourn commented Aug 10, 2018

Caretaker note: this change has messed up menus in Firebase twice now. Do not merge without validating that the issue is resolved with a Firebase team member.

@jelbourn jelbourn added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 10, 2018
@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker G This is is related to a Google internal issue P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Aug 10, 2018
@ngbot
Copy link

ngbot bot commented Aug 22, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 11365/flexible-overlay-push-scroll-3-revenge-of-the-sith branch from afb4639 to 66e6e37 Compare August 22, 2018 14:49
@mmalerba
Copy link
Contributor

@crisbeto figured out that's going on with the g3 failures. You added withLockedPosition to the menu. It seems like firebase has a button that triggers a menu and a screenshot test where they open that menu and then resize the screen. They're expecting the menu to reposition into a better place after the resize, but that is no longer happening. Probably what we need to do is lock the position during scrolling, but still potentially reposition when the window resizes

…nd position locking

* Fixes the position of flexible overlays with pushing enabled being thrown off once the user starts scrolling.
* Fixes flexible overlays with pushing not handling locked positioning. With these changes locked overlays will only be pushed when they're opened, whereas non-locked overlays will stay in the viewport, even when the user scrolls down.
* Fixes a potential issue due to a couple of variables being initialized together where one is set to zero and the other one is undefined.

Fixes angular#11365.
@crisbeto crisbeto force-pushed the 11365/flexible-overlay-push-scroll-3-revenge-of-the-sith branch from 66e6e37 to f5408a7 Compare August 24, 2018 05:04
@crisbeto
Copy link
Member Author

crisbeto commented Aug 24, 2018

@mmalerba I'm not sure why somebody would have tests for this case, but it makes sense ¯\_(ツ)_/¯. I've reworked it so it treats repositioning after a page resize as a new render and it picks a new optimal position.

@davidmedina-contact
Copy link

Good work @crisbeto! Wondering when can this be merged? Anxiously waiting for a fix to this one

@lfdantoni
Copy link

Hi @crisbeto ! Good work, I am wondering if this PR was merged because I just saw that there is a new material version (6.4.7) and I can't find these changes applied. Thank you :)

@crisbeto
Copy link
Member Author

crisbeto commented Sep 3, 2018

It's in the 7.0 beta.

@LeonardoGastonRossi
Copy link

Hii @crisbeto how are you!? Is it possible for any chance that this fixed can be push in version 6?? Because It's a blocker for us, it's breaking the functionality in our site.

@amouly
Copy link

amouly commented Sep 5, 2018

@crisbeto this issue breaks and existing functionality on our Production website. There is any chance we can push this code to a future 6.X.X release? If not, what's the blocker? There is any known workaround?

Thanks in advance.

@adamdport
Copy link

On Cordova, the menus sometimes don't appear to open at all because they're positioned off screen.

For those that end up here from google, it looks like this has been included in 7.0.0-beta-0.

LeonardoGastonRossi pushed a commit to LeonardoGastonRossi/material2 that referenced this pull request Sep 22, 2018
…ng to be merge into 6.4.X.

* Fixes the position of flexible overlays with pushing enabled being thrown off once the user starts scrolling.
* Fixes flexible overlays with pushing not handling locked positioning. With these changes locked overlays will only be pushed when they're opened, whereas non-locked overlays will stay in the viewport, even when the user scrolls down.
* Fixes a potential issue due to a couple of variables being initialized together where one is set to zero and the other one is undefined.

Fixes angular#11365.
jelbourn pushed a commit that referenced this pull request Oct 4, 2018
…e merge into 6.4.X.

* Fixes the position of flexible overlays with pushing enabled being thrown off once the user starts scrolling.
* Fixes flexible overlays with pushing not handling locked positioning. With these changes locked overlays will only be pushed when they're opened, whereas non-locked overlays will stay in the viewport, even when the user scrolls down.
* Fixes a potential issue due to a couple of variables being initialized together where one is set to zero and the other one is undefined.

Fixes #11365.
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu positioning going wrong on scroll
10 participants