refactor!: update dialog to use native popover #9807
Conversation
| firstUpdated(props) { | ||
| super.firstUpdated(props); | ||
|
|
||
| this.role = 'dialog'; |
There was a problem hiding this comment.
Not necessary anymore, as it's set by DialogBaseMixin
| if (!this.hasAttribute('role')) { | ||
| this.role = 'dialog'; | ||
| } | ||
| } | ||
|
|
||
| /** @protected */ | ||
| updated(props) { | ||
| super.updated(props); | ||
|
|
||
| if (props.has('overlayRole')) { | ||
| this.role = this.overlayRole || 'dialog'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Changed this so you can use role instead of overlayRole.
packages/dialog/src/vaadin-dialog.js
Outdated
| /** @private */ | ||
| __applyHeaderTitleAriaLabel() { | ||
| // If there is a header title and no aria-label, | ||
| // set the aria-label to the header title. | ||
| if (this.headerTitle && !this.ariaLabel) { | ||
| this.ariaLabel = this.headerTitle; | ||
| this.__useHeaderTitleAriaLabel = true; | ||
| } | ||
|
|
||
| // If there is no header title and the aria-label was set | ||
| // using a header title, remove the aria-label. | ||
| if (this.__useHeaderTitleAriaLabel && !this.headerTitle) { | ||
| this.ariaLabel = null; | ||
| this.__useHeaderTitleAriaLabel = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Likewise, this is supposed to allow using aria-label instead of headerTitle.
Put this into dialog itself, as DialogBaseMixin is used in CRUD where we need different logic and it doesn't have headerTitle.
There was a problem hiding this comment.
I wonder if this can be simplified. If updated doesn't include headerTitle if it's never set, then tracking the state wouldn't be necessary.
There was a problem hiding this comment.
If updated doesn't include headerTitle if it's never set
Yes, I think that's the case.
Technically it's possible that ariaLabel is set, then headerTitle is set, and then ariaLabel gets removed - in theory, we probably should then set default ariaLabel based on headerTitle. But it's probably an edge case.
There was a problem hiding this comment.
I simplified it for now. A practical case might be to use a different aria-label than the header title, but that seems unlikely.
| await nextRender(); | ||
| }); | ||
|
|
||
| describe('aria-label', () => { |
There was a problem hiding this comment.
This was just testing delegating the label to the overlay. Just setting aria-label on the dialog shouldn't require tests now, the combination with headerTitle is tested in header-footer.test.js.
|
|
This ticket/PR has been released with Vaadin 25.0.0-beta2. |
3 participants
Description
Fixes #9727
Type of change