Skip to content

feat(material-experimental): MDC-based version of dialog #19038

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

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

devversion
Copy link
Member

Initial version of the MDC-based dialog.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 10, 2020
@devversion devversion force-pushed the wip/mdc-dialog branch 3 times, most recently from 8fc11a3 to 569d482 Compare April 10, 2020 12:57
@devversion devversion added the target: patch This PR is targeted for the next patch release label Apr 10, 2020
@devversion devversion marked this pull request as ready for review April 10, 2020 13:25
@devversion devversion requested review from crisbeto, jelbourn, mmalerba and a team as code owners April 10, 2020 13:25

/** Starts the dialog open animation if enabled. */
private _startOpenAnimation() {
this._animationStateChanged.emit({state: 'opening', totalTime: this._openAnimationDuration});
Copy link
Member

Choose a reason for hiding this comment

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

It seems like all the places where this is used are stubbing out the interface for Angular's AnimationEvent. Since this is an internal event, couldn't we change it to something that is more convenient to pass around?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how this can be made more convenient. From my perspective, this interface is easy to use and understand. It's not too coupled to Angular's AnimationEvent since it contains a specific state for the dialog (e.g. closed, closing and more).

Happy to change to something else if you have any ideas.

@devversion devversion force-pushed the wip/mdc-dialog branch 4 times, most recently from ebddaaf to 2761cab Compare April 14, 2020 18:01
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Do we need to add an instance to the kitchen sink app too?

Hopefully @jelbourn can take a look at this next week. He's a lot more familiar with the dialog than I am

@devversion
Copy link
Member Author

devversion commented Apr 21, 2020

@mmalerba Addressed feedback. Thanks! Please have another look when you get a chance.

@mmalerba
Copy link
Contributor

LGTM, but @jelbourn still needs to take a look

@mmalerba mmalerba added the lgtm label Apr 21, 2020
@devversion
Copy link
Member Author

@jelbourn Addressed feedback. Please have another look when you get a chance.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

We can change the way it does padding in a follow-up

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Apr 22, 2020
@andrewseguin
Copy link
Contributor

andrewseguin commented Jul 23, 2020

Due to the focus changes (and thus some new triggered change detection changes), it's possible for MatDialogClose to run its ngOnInit before the dialog finishes initializes everything. Therefore, this.dialogRef could be left undefined.

If _onButtonClick is called during this, an error will be thrown because this.dialogRef isn't present. To fix, let's change MatDialogRef._onButtonClick (https://github.com/angular/components/blob/master/src/material/dialog/dialog-content-directives.ts#L36) to the following:

  _onButtonClick(event: MouseEvent) {
    if (!this.dialogRef) {
      this.dialogRef = getClosestDialog(this._elementRef, this._dialog.openDialogs)!;
    }

    // Determinate the focus origin using the click event, because using the FocusMonitor will
    // result in incorrect origins. Most of the time, close buttons will be auto focused in the
    // dialog, and therefore clicking the button won't result in a focus change. This means that
    // the FocusMonitor won't detect any origin change, and will always output `program`.
    _closeDialogVia(this.dialogRef,
        event.screenX === 0 && event.screenY === 0 ? 'keyboard' : 'mouse', this.dialogResult);
  }

@andrewseguin
Copy link
Contributor

We're close to getting this one in - mind fixing that browserstack test failure?

@devversion devversion force-pushed the wip/mdc-dialog branch 6 times, most recently from e460f46 to a5ced9f Compare July 29, 2020 11:50
Angular's `TestBed` by default only removes a test component from the
document body and destructs individual component fixtures. It never
destroys the test module properly, but always re-creates it.

This means that providers are not necessarily cleaned up (could
leak in memory) and component styles are not cleaned up / deduped.
This results in thousands of style elements in the browsers. Ultimately
causing significantly increased memory consumption and CPU blocking.

This potentially leads to repeated crashes within browsers (as seen in
BrowserStack and Saucelabs in the past). Initial testing of this change
unveiled a reduction from 30min tests to 5min in BrowserStack. This is
a signficant improvement and we should consider moving this change
upstream with: angular/angular#31834.

Benefits are: reduced test time; increased test/browser stability;
isolated tests without leaking styles and services.
@devversion devversion requested a review from andrewseguin as a code owner July 29, 2020 19:55
@devversion devversion added the merge: preserve commits When the PR is merged, a rebase and merge should be performed label Jul 29, 2020
@devversion
Copy link
Member Author

This is finally green! While fixing tests in Safari and Firefox I found that we leak providers and styles between tests. Fixing this reduced average CI time for Browserstack from 30min to 5min! Previously browsers on BS and SL were close to crash (and sometimes did) due to too many style elements in the DOM (and too many providers being retained).

@mmalerba mmalerba added target: minor This PR is targeted for the next minor release and removed lgtm target: patch This PR is targeted for the next patch release labels Jul 31, 2020
@mmalerba mmalerba merged commit a295ed0 into angular:master Aug 4, 2020
@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 4, 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 merge: preserve commits When the PR is merged, a rebase and merge should be performed P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants