fix: do not reopen popover when clicking target #9979
Conversation
packages/popover/test/basic.test.js
Outdated
| @@ -264,6 +264,7 @@ describe('popover', () => { | |||
| target.click(); | |||
| await nextRender(); | |||
| expect(overlay.opened).to.be.false; | |||
| expect(popover.opened).to.be.false; | |||
There was a problem hiding this comment.
This is surprisingly hard to test. With the test setup the following happens:
- Overlay closes, opened state is reflected back to popover
- Popover changes back to opened immediately
- The
_openedChangedhandler is called with both opened and oldOpened being true?? - I assume due to that the template doesn't render again because the opened state in popover didn't change, so the overlay stays closed
Might have something to do with the not animated styles? I'm a bit at a loss on how to make this test setup work in a better way.
There was a problem hiding this comment.
@web-padawan Do you have any ideas?
There was a problem hiding this comment.
Maybe we should use sendMouseToElement({ type: 'click', element: target }) instead, for me it helps.
There was a problem hiding this comment.
Yeah, we apparently can not use sendMouseToElement because it's async and might resolve after vaadin-overlay-open. So the following await for that event times out.
There was a problem hiding this comment.
Let's revert back to using click() then. I think your original test looks good.
We can probably add sync: true to opened property, then it fails as expected.
There was a problem hiding this comment.
I've now changed it to only use the browser command for closing and left a remark why it's only used there.
|
There is now a timeout in Firefox which is also reproducible locally. |
1dd043a to
8303700
Compare
|
Description
Addresses a regression from #9972
When clicking on the target when the popover is opened, it currently closes due to the outside click, and then reopens immediately as clicking the target also toggles the opened state. This excludes the target from the outside click handling, which should be how it was before with the custom global click handler in popover: https://github.com/vaadin/web-components/pull/9972/files#diff-1171ae48b99c01700dcc87f053aefeeeef342b9c9a12ad3c731a1dccf9188f5dL663-L673
Type of change