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(expansion-panel): appearing as open if placed inside animating element #14127

Closed

Conversation

crisbeto
Copy link
Member

This is an alternate approach to #13888 which had to be reverted due to some presubmit issues. These changes address the problem by providing some default styles that will be applied if the expansion panel is inside an animating element.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Nov 13, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 13, 2018
// header's default height. If the animations aren't blocked, these styles will be overridden
// by the inline styles from the animations module.
.ng-animating .mat-expansion-panel & {
min-height: 48px;
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that the height of the panels is going to be 48px and 64px. Does this account for if someone overrides those values using the @Inputs in the panel header?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't account for the case where people are setting it through the input, but I wasn't able to think of a better way to handle it.

Copy link
Member

Choose a reason for hiding this comment

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

I think we might need to reassess if we can try to do the fix from #13888 again.

I think the "correct" solution is for the containing components to allow the animations rather than the components which don't necessarily know what components they will be in.

Because While this is something that we are seeing in expansion-panel, the issue is actually in things like dialog/tabs and if someone has a custom component doing animations they will have the same issue we are seeing.

Copy link

Choose a reason for hiding this comment

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

In regards to #13888 and the revert of it in #14017,

This change broke an app inside of Google where tabs appear inside of a
dialog. The tabs get stuck in a weird frozen animation state.

Is there a test to make sure apps like the internal one mentioned above do not get caught in a weird animation state?

I want to contribute to solving #13870 in any way I am able, just not sure how to test this type of fix/regression.

Copy link
Member

Choose a reason for hiding this comment

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

@virtuoushub Unfortunately there is not a test to check it because we haven't been able to isolate it outside of the app for a reproduction. We haven't been able to create a test to check the situation in which it occurs, but with the change from #13888 the failure reproduced in the app with perfect consistency.

I am working with @matsko on sorting it out, but unfortunately we just haven't been able to sort it out yet.

Choose a reason for hiding this comment

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

Thanks for the update. As @Teamop mentioned, whatever fix is decided on, does it relate to #11765 and it's respective PR #11778?

@simeyla
Copy link

simeyla commented Feb 20, 2019

Make sure to check that any fix considers the opposite case, where an already OPEN expansion panel is animated off the screen.

I have an accordian with represents a menu, so each panel contains a mat-nav-list of buttons (submenu). When I select a sub-option the whole menu should slide to the left offscreen, but instead the panel collapses itself first.

To try to figure out what's going on I set a very long animation duration (30s) and it seems that if I interact with the menu at all while it is animating on screen then I can't even open an accordion item once it's completed animating. If I don't click on any of the items during the animation then it works as normal when the animation is complete.

It's all extremely confusing to even figure out what's happening, but I think the fix is going to be beyond a few css tweaks.

Copy link

@simeyla simeyla left a comment

Choose a reason for hiding this comment

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

There's two shortcomings I've identified with this solution.

Issue 1) If you interact with the expansion panel during animation (open/close panels) then it can get in a completely stuck and broken state where you can't open or close any panels at all. My guess is that it's internally doing something with the height - which we've tampered with and therefore it gets confused.

Any usage of opacity or outline below is obviously just for demonstration purposes. You will probably have to see the parent animation duration to 30seconds to see what's going on.

Suggested css solution - prevent clicking during animation :

.mat-expansion-panel-header {

    .ng-animating .mat-expansion-panel &
    {
        pointer-events: none !important;
        opacity: .5;   // remove this
    }
}

.mat-expansion-panel-content {

    .ng-animating .mat-expansion-panel &
    {
        pointer-events: none !important;
        opacity: .5;   // remove this
    }
}

Issue 2)

If an expander item is open - such as an accordion containing groups of sub-menu items - then when it is animated off screen (:leave) you likely want it to remain open, but it closes.

To achieve that you need to also add this rule - or something equivalent:

.mat-expansion-panel-content {

    .ng-animating .mat-expansion-panel.mat-expanded & {
        height: auto !important;
        visibility: visible !important;
        outline: 1px solid orange;  // remove this
      }
  }

These additional changes seem to work ok for me on real iOS device and Chrome testing tools.

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@crisbeto crisbeto force-pushed the 13870/expansion-panel-default-styles branch from c456aa9 to 5119909 Compare June 2, 2019 08:38
@crisbeto crisbeto added the P2 The issue is important to a large percentage of users, with a workaround label Jun 2, 2019
@crisbeto crisbeto force-pushed the 13870/expansion-panel-default-styles branch from 5119909 to 8df25c9 Compare July 13, 2019 14:56
…ement

This is an alternate approach to angular#13888 which had to be reverted due to some presubmit issues. These changes address the problem by providing some default styles that will be applied if the expansion panel is inside an animating element.
@crisbeto crisbeto force-pushed the 13870/expansion-panel-default-styles branch from 8df25c9 to 8284736 Compare October 3, 2019 19:20
@josephperrott josephperrott removed their assignment Oct 15, 2019
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@crisbeto
Copy link
Member Author

crisbeto commented Mar 6, 2022

This shouldn't be necessary after the changes in #24529.

@crisbeto crisbeto closed this Mar 6, 2022
@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 Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants