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(cdk/overlay): fix wrong overflow calculation #17791

Merged
merged 3 commits into from Aug 6, 2020

Conversation

Goobles
Copy link
Contributor

@Goobles Goobles commented Nov 22, 2019

overflowBottom is currently calculated with viewport.bottom (which could be any number from the height of the viewport to as large as your page is)
calculating the overflow should just be difference between overlay.height + start.y and the height of the viewport, not the height of the entire page.

Right now the overlay is pushed out of the view in certain cases.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 22, 2019
@jelbourn
Copy link
Member

@Goobles can you create a stackblitz that shows the issue this is fixing? The part I don't follow here is that the overlayPoint and the viewportRect's right/bottom should be in the same coordinate space, so my understanding would be that bottom and right would be what we want since they include the scroll position of the page, while width and height don't.

@Goobles
Copy link
Contributor Author

Goobles commented Nov 27, 2019

@jelbourn Here is a minimal reproduction of the bug I am experiencing:
https://stackblitz.com/edit/angular-payabe

@Goobles
Copy link
Contributor Author

Goobles commented Nov 27, 2019

start.x and start.y are in the coordinate space of the overlay container, it does not contain the scrollposition of the page.

@jelbourn
Copy link
Member

Thanks! I see now; the change looks good to me, then. The unit tests just need to be updated.

@jelbourn jelbourn 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 Nov 28, 2019
@mmalerba
Copy link
Contributor

mmalerba commented Mar 5, 2020

@Goobles are you planning to update the tests here?

@Goobles
Copy link
Contributor Author

Goobles commented Mar 6, 2020

@mmalerba Oh, the test that are failing were from tests on experimental components, I don't really know how to change them correctly?

@mmalerba
Copy link
Contributor

mmalerba commented Mar 6, 2020

@crisbeto are you able to help with getting the tests fixed?

@mmalerba
Copy link
Contributor

It looks like the failure is just rounding issues. You can change the test to do approximate comparison by doing something like Math.abs(expected - actual) < 1

@mmalerba mmalerba added lgtm action: merge The PR is ready for merge by the caretaker labels Apr 29, 2020
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@Goobles Goobles force-pushed the flexible-connected-position-fix1 branch from f8452e5 to 34cb475 Compare May 4, 2020 10:58
@jelbourn jelbourn added this to PRs in P2 PRs Jul 9, 2020
@andrewseguin
Copy link
Contributor

This ones close to getting in - do you mind rebasing?

overflowBottom is currently calculated with viewport.bottom (which could be any number from the height of the viewport to as large as your page is)
calculating the overflow should just be difference between "overlay.height added to the current coordinate on the overlay container" and the height of the viewport, not the height of the entire page.
Tested component should always fit within the viewport.
Pane dimensions should be approximately compared.
@mmalerba mmalerba force-pushed the flexible-connected-position-fix1 branch from 34cb475 to eb830c3 Compare July 27, 2020 21:44
@mmalerba
Copy link
Contributor

@andrewseguin I rebased it

@andrewseguin andrewseguin moved this from PRs to In Progress in P2 PRs Jul 29, 2020
@andrewseguin andrewseguin force-pushed the flexible-connected-position-fix1 branch from a53a14a to fc0adf7 Compare July 29, 2020 18:31
@andrewseguin andrewseguin force-pushed the flexible-connected-position-fix1 branch from fc0adf7 to 2618c8a Compare July 29, 2020 18:32
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@mmalerba mmalerba merged commit d2b499d into angular:master Aug 6, 2020
P2 PRs automation moved this from In Progress to Done Aug 6, 2020
mmalerba pushed a commit that referenced this pull request Aug 6, 2020
* fix(cdk/overlay): fix wrong overflow calculation

overflowBottom is currently calculated with viewport.bottom (which could be any number from the height of the viewport to as large as your page is)
calculating the overflow should just be difference between "overlay.height added to the current coordinate on the overlay container" and the height of the viewport, not the height of the entire page.

* fix(popover-edit): fix failing test
Tested component should always fit within the viewport.
Pane dimensions should be approximately compared.

* avoid flaky errors due to narrow viewport width

Co-authored-by: Gobius Dolhain <Goobles@users.noreply.github.com>
Co-authored-by: Andrew Seguin <andrewjs@google.com>
(cherry picked from commit d2b499d)
@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 6, 2020
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 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
Projects
P2 PRs
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants