refactor!: update popover overlay to use native popover #9785
Conversation
web-padawan
force-pushed
the
refactor/popover-native-popover
branch
from
1580e0d to
c5e6551
Compare
|
|
||
| root.innerHTML = ` | ||
| <button>I have a popover</button> | ||
| <vaadin-popover> |
There was a problem hiding this comment.
Added a nested popover to verify that dragging dialog while popover is opened doesn't hide popover overlay below the dialog one (which was originally reported at #8521).
|
|
||
| const button = document.createElement('vaadin-button'); | ||
| button.textContent = 'Apply'; | ||
| const nested = document.createElement('vaadin-popover'); |
There was a problem hiding this comment.
Added a nested popover to make it easier to test different combination of triggers.
| if (target) { | ||
| const parent = getClosestOverlay(target); | ||
| if (parent) { | ||
| setNestedOverlay(parent, opened ? this : null); |
There was a problem hiding this comment.
This is no longer needed and I'll create a separate PR for removing setNestedOverlay helper altogether.
Calling bringToFront() for dialog when it has a popover open won't do anything since #9773.
web-padawan
force-pushed
the
refactor/popover-native-popover
branch
from
c5e6551 to
f4a0321
Compare
| return; | ||
| } | ||
|
|
||
| // Copy custom properties from the owner |
There was a problem hiding this comment.
This is no longer needed as offset custom CSS properties will inherit to the overlay via cascade.
| }); | ||
| }); | ||
|
|
||
| describe('bring to front', () => { |
There was a problem hiding this comment.
Corresponding logic related to setNestedOverlay() was removed as no longer needed.
| }); | ||
| }); | ||
|
|
||
| describe('modeless dialog', () => { |
There was a problem hiding this comment.
These tests don't make sense anymore: even if bringToFront() sets higher z-index on the overlay, the popover will still be shown on top of it because it's using popover, and the dialog currently does not.
I'll create a separate issue to add a new logic for bringing nested overlays to front and then re-add ITs.
| return; | ||
| } | ||
|
|
||
| if ((this.__hasTrigger('focus') && this.__mouseDownInside) || this._overlayElement.contains(event.relatedTarget)) { |
There was a problem hiding this comment.
Changed this._overlayElement.contains with this.contains here and elsewhere which seems correct: in case of moving focus by the browser Tab logic, the relatedTarget is vaadin-popover itself.
|
|
||
| /** | ||
| * String used to label the overlay to screen reader users. | ||
| * String used to label the popover to screen reader users. |
There was a problem hiding this comment.
We could probably deprecate accessibleName and accessibleNameRef or even remove them in favor of regular aria-label and aria-labelledby, respectively (and remove overrides from the Flow counterpart).
|
|
||
| // If no user ID is provided, set generated ID | ||
| if (!this.id) { | ||
| this.id = this.__generatedId; |
There was a problem hiding this comment.
We could also add a mutation observer that would check if the user ID is removed and then set a generated one. Not sure if that's a common case, maybe we should create a follow-up issue and do it separately.
web-padawan
force-pushed
the
refactor/popover-native-popover
branch
2 times, most recently
from
e00cbf0 to
d1bd34f
Compare
web-padawan
force-pushed
the
refactor/popover-native-popover
branch
from
3ffd864 to
5d87b70
Compare
|
|
||
| const popoverOverlay = css` | ||
| :host { | ||
| --_default-offset: 0; |
There was a problem hiding this comment.
Aligned the custom CSS property name with the one used in base styles.
Co-authored-by: Sascha Ißbrücker <sissbruecker@vaadin.com>
|
|
This ticket/PR has been released with Vaadin 25.0.0-alpha3. |
3 participants
Description
Fixes #9713
This changes the
vaadin-popoverto render content to itself. We can therefore consider deprecatingrendererproperty and Lit directive and possibly update Flow counterpart to just use regular slotted content.Note: snapshot tests for this component are missing, I'll add them in a follow-up PR.
Type of change