-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(overlay): take scroll position into account on iOS (#6341) #11947
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out and it definitely works, but we need a unit test for it, otherwise we run the risk of removing it next time we do any larger refactoring. What issues did you have with setting it up locally?
private _getOverlayScrollY(): number { | ||
const clientRect = this._overlayRef.hostElement.getBoundingClientRect(); | ||
|
||
return Math.abs(clientRect.top); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we always want the absolute value here. If you flip the logic above so it's originPoint.y - overlayScrollY
it should work the same and account for cases where it might be pushed from the top for some reason.
* this will return a non-zero value. | ||
*/ | ||
private _getOverlayScrollY(): number { | ||
const clientRect = this._overlayRef.hostElement.getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether this would still apply for a flexible overlay since the bounding box isn't 100% by 100% anymore.
@@ -468,13 +468,28 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy { | |||
overlayStartY = pos.overlayY == 'top' ? 0 : -overlayRect.height; | |||
} | |||
|
|||
// On iOS, the .cdk-overlay-container will be positioned absolute instead of fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's what's happening (the container becoming absolute
). From what I can tell the element is still fixed, it's just that iOS shifts the entire coordinate system up to account for the keyboard.
@@ -468,13 +468,28 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy { | |||
overlayStartY = pos.overlayY == 'top' ? 0 : -overlayRect.height; | |||
} | |||
|
|||
// On iOS, the .cdk-overlay-container will be positioned absolute instead of fixed. | |||
// Adjust the y coordinate in that case by the scroll amount. | |||
const overlayScrollY = this._getOverlayScrollY(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this one inline since it's basically a one-liner?
@jpzwarte are you able to address the feedback on this one? |
I want to, but i have no time atm :( |
In that case do you want me to open a separate PR and try to get the fix in? |
Please do 👍 |
This is taking over from angular#11947. Fixes the connected overlay positioning being thrown off, if it is opened while an input is focused (e.g. when building an autocomplete component). The issue comes from the fact that the browser will offset the entire page in order to put the input in the middle and to make space for the virtual keyboard. Fixes angular#6341.
This is taking over from angular#11947. Fixes the connected overlay positioning being thrown off, if it is opened while an input is focused (e.g. when building an autocomplete component). The issue comes from the fact that the browser will offset the entire page in order to put the input in the middle and to make space for the virtual keyboard. Fixes angular#6341.
This is taking over from #11947. Fixes the connected overlay positioning being thrown off, if it is opened while an input is focused (e.g. when building an autocomplete component). The issue comes from the fact that the browser will offset the entire page in order to put the input in the middle and to make space for the virtual keyboard. Fixes #6341.
…12119) This is taking over from #11947. Fixes the connected overlay positioning being thrown off, if it is opened while an input is focused (e.g. when building an autocomplete component). The issue comes from the fact that the browser will offset the entire page in order to put the input in the middle and to make space for the virtual keyboard. Fixes #6341.
…12119) This is taking over from #11947. Fixes the connected overlay positioning being thrown off, if it is opened while an input is focused (e.g. when building an autocomplete component). The issue comes from the fact that the browser will offset the entire page in order to put the input in the middle and to make space for the virtual keyboard. Fixes #6341.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When the keyboard is visible on iOS, the
.cdk-overlay-container
will be positioned absolutely instead of fixed. The can be seen by checking thetop
value of the bounding client rect of the overlay container. When scrolled + keyboard, this will have a negative value on iOS. This PR adds the absolute top value to the y coordinate of the overlay point.Note: unfortunately, "npm install" in material2 fails locally so i cannot test it on my own code :(