- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6.8k
fix(dialog): capture previously focused element immediately #3875
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
| if (this._document) { | ||
| this._elementFocusedBeforeDialogWasOpened = this._document.activeElement as HTMLElement; | ||
| } | ||
| } | 
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.
Any way to capture this in a unit test?
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.
It was tricky to write a test that fails when we expect it to since we trigger the change detection and animations manually.
        
          
                src/lib/dialog/dialog-container.ts
              
                Outdated
          
        
      |  | ||
| /** | ||
| * Saves a reference to the element that was focused before the dialog was opened. | ||
| * @private | 
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.
Don't need @private JsDoc
        
          
                src/lib/dialog/dialog-container.ts
              
                Outdated
          
        
      | private _elementRef: ElementRef, | ||
| private _focusTrapFactory: FocusTrapFactory) { | ||
| private _focusTrapFactory: FocusTrapFactory, | ||
| @Optional() @Inject(DOCUMENT) private _document: any) { | 
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.
We can add a type for this while working around Angular Universal's limitations by doing Document|Document
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.
Doing Document|Document fails in AoT.
Error: Error encountered resolving symbol values statically. Expression form not supported (position 84:54 in the original .ts file), resolving symbol MdDialogContainer
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.
Chatted with Chuck and this is a known issue with the metadata extractor (angular/angular#15424). Until that's fixed, we can inject it as any   without the private sugar and then manually assign it to the field with type Document.
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.
Done.
0098279    to
    0470d35      
    Compare
  
    | Addressed the feedback @jelbourn. | 
3690447    to
    ebf8407      
    Compare
  
    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.
LGTM
| @crisbeto Can you rebase? | 
Captures the previously-focused element immediately, instead of waiting until the animation is done. This fixes a regression from angular#3774, because if we wait for the animation to finish, the focus might have shifted, or the element could have been disabled.
231f23a    to
    43a16fa      
    Compare
  
    | 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. | 
Captures the previously-focused element immediately, instead of waiting until the animation is done. This fixes a regression from #3774, because if we wait for the animation to finish, the focus might have shifted, or the element could have been disabled.