Lookup: Close dropdown when clicking outside area #28041
Conversation
| if (this.option('_scrollToSelectedItemEnabled')) this._popup._$arrow.remove(); | ||
|
|
||
| this._setPopupContentId(this._popup.$content()); | ||
|
|
||
| this._contentReadyHandler(); | ||
| }, | ||
|
|
||
| _popupFocusOutHandler($overlayContent) { | ||
| $overlayContent.get(0).addEventListener('focusout', (e) => { |
There was a problem hiding this comment.
Please, use eventsEngine syntax, not addEventListener.
There was a problem hiding this comment.
Other than that, you may notice that the focus from the content has moved to the address bar, the popup has closed, but the dx-lookup element still has a focused state:
| dropDownOptions: { | ||
| hideOnOutsideClick: false, // Ensures the dropdown does not close when clicking outside | ||
| }, | ||
| opened: true |
There was a problem hiding this comment.
| opened: true | |
| opened: true, |
| const defaultOptions = { | ||
| items: ['1', '11', '111'], | ||
| value: '1', | ||
| dropDownOptions: { |
There was a problem hiding this comment.
Check the scenario in which you install:
dropDownOptions: {
hideOnOutsideClick: false,
},
you will see that Lookup doesn't close before your changes if you click outside of Lookup (see demo: https://codepen.io/marker-dao/pen/WNqWJrq??editors=1010), but with your changes it works differently - Lookup closes if you click outside of it.
…24_2-lookup-close-dropdown-issue
| this.option('focusStateEnabled') && this.focus(); | ||
|
|
||
| if (this._isFocusOutTriggered) { | ||
| this.$element().removeClass('dx-state-focus'); |
There was a problem hiding this comment.
[incorrect logic]
We should never manually remove/add this class, it toggles automatically on focusin/focusout.
And I don't understand the goal, now u say "when popup is closing remove focus from input element if popup was focused out"
| @@ -780,6 +811,8 @@ const Lookup = DropDownList.inherit({ | |||
| $searchWrapper.insertBefore(this._$list); | |||
|
|
|||
| this._setSearchPlaceholder(); | |||
| } else { | |||
| this._removeSearch(); | |||
There was a problem hiding this comment.
[excess changes?]
For what? How search is related to closing popup on clicking outside of lookup?
|
|
||
| const nextFocusableElement = $allFocusable.get(currentIndex + 1) as HTMLElement | undefined; | ||
| nextFocusableElement?.focus(); | ||
| } |
There was a problem hiding this comment.
[code splitting]
This method "_toggleSearchClass" is designed for single class toggling, there should not be any other logic. Now u say "when we toggle search class we need to move focus to next focusable element or search field". Why?
| const currentIndex = $allFocusable.index(activeElement); | ||
|
|
||
| const nextFocusableElement = $allFocusable.get(currentIndex + 1) as HTMLElement | undefined; | ||
| nextFocusableElement?.focus(); |
There was a problem hiding this comment.
[excess code?]
I don't see a reason for this code with manual focus change. Our task is to close popup when focus goes out of component. Why do we need make all this additional stuff with focus?
JFYI for the future:
- we cannot use
documentdirectly in our code, it should be done throughdomAdapterutil - now your
$allFocusablevariable contains all focusable elements on a page. That's bad for performance. Focusing other element of page aftertabkey press is a browser behavior, we should not try do it our side
| $(this.lookup._$field).trigger('dxclick'); | ||
|
|
||
| assert.notOk(this.lookup.option('opened'), 'popup is hidden'); | ||
| }); |
There was a problem hiding this comment.
[test coverage]
How it can be true to have single tests for 50+ changed code rows?
And I actually see that all target scenarios do now work at the moment: closing on outside click, moving focus out of component by pressing tab and by pressing shift+tab
| QUnit.test('dropdown should close when focusout event happens outside component', function(assert) { | ||
| assert.ok(this.lookup.option('opened'), 'popup is shown'); | ||
|
|
||
| $(this.lookup._$field).trigger('dxclick'); |
There was a problem hiding this comment.
[not working test]
Closing lookup on its field clicking is controlled by other code. You can see that this scenario worked even before your changes
No description provided.