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): restore previous host element before attaching #17855

Open
wants to merge 1 commit into
base: master
from

Conversation

@dirkluijk
Copy link

dirkluijk commented Dec 2, 2019

The first time when PositionStrategy.attach() is called, the overlay is already added to the DOM. However, after detaching and attaching again, it is not yet added to the DOM.

This PR changes the order and first restores the host element back, and then invokes the attach() method.

@dirkluijk dirkluijk requested review from crisbeto and jelbourn as code owners Dec 2, 2019
@googlebot googlebot added the cla: yes label Dec 2, 2019
@dirkluijk dirkluijk force-pushed the dirkluijk:patch-1 branch from 4e0364a to 7cae90a Dec 2, 2019
@jelbourn

This comment has been minimized.

Copy link
Member

jelbourn commented Dec 3, 2019

Is there a bug this is fixing? Are you able to add a unit test that captures the desired behavior?

@dirkluijk

This comment has been minimized.

Copy link
Author

dirkluijk commented Dec 3, 2019

Yes, I am trying to calculate the size of the overlay in my own PositionStrategy. However, after detaching and attaching, the size is 0,0 because it is still outside the DOM.

I will add a unit test for that.

@dirkluijk dirkluijk force-pushed the dirkluijk:patch-1 branch from 7cae90a to 52e15bc Dec 3, 2019
@dirkluijk

This comment has been minimized.

Copy link
Author

dirkluijk commented Dec 3, 2019

I added the test. I double-checked it; without my fix it fails as expected.

@dirkluijk dirkluijk force-pushed the dirkluijk:patch-1 branch from 52e15bc to 25ef747 Dec 3, 2019
@dirkluijk

This comment has been minimized.

Copy link
Author

dirkluijk commented Dec 3, 2019

Pushed the requested change.

Copy link
Member

crisbeto left a comment

LGTM

Copy link
Member

jelbourn left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.