fix(ui5-calendar): min max date keyboard navigation issue #2549
Conversation
Keyboard navigation now works properly, when ui5-datepicker component popover is opened and there are disabled dates in the corresponding ui5-calendar component due to min/max set.
…into calendar-keyboard-nav-issue
|
Hello @SAP/ui5-webcomponents-topic-b @tsanislavgatev could some of the team review the change from domain knowledge perspective. |
There was a problem hiding this comment.
I realize that the code was already like this and you just added a couple of checks, and my comments are not directly related to this change.
However, the code I commented on needs to be changed (with this PR or another) as it has a couple of high impact issues:
- principle of least knowledge not followed
- misusing ItemNavigation (let me know if it cannot do something so we can update the APIs)
- Changing DOM directly. You are probably used to OpenUI5 development where it's common to find something with jQUery and change it directly, but in UI5 Web Components the paradigm is different - you must change the component's state and only that. Then the framework will sync this state to the DOM via the template.
This is how the whole ItemNavigation interaction is supposed to work:
- in DayPicker.js you bind the _weeks property in the template. There you already have tabindex bound:
tabindex="{{this._tabIndex}}"in DayPicker.hbs so this step is complete as it is. - Then you pass
focusableDaysto the item navigation (which is a subset ot _weeks) and whenever item navigation changes the items in focusableDays, it actually changes the items in _weeks. - Whenever this happens, item navigation invalidates DayPicker (in the update method) and it rerenders with the new values of _tabIndex inside _weeks and the "tabindex" attribute in DOM is thus updated correctly via the framework.
What I can see from the code is that you don't call the update() function (which does all this + the invalidation) and then you manually change the current item and manually update the DOM. Again, is there a technical reason to do so, or lack of documentation for ItemNavigation which would explain how to use it?
I won't be requesting changes so it's up to you to merge this change now and create a new PR for my comments or implement them here. But in any case this must be done as soon as possible.
| const currentDateIndex = this.dayPicker._getVisibleDays(currentDate).findIndex(date => date.valueOf() === currentDate.valueOf()); | ||
| this.dayPicker._itemNav.currentIndex = currentDateIndex; | ||
| const currentDate = new CalendarDate(calDate, this._primaryCalendarType); | ||
| const currentIndex = this.dayPicker.focusableDays.findIndex(item => { |
There was a problem hiding this comment.
Principle of least knowledge violated: https://en.wikipedia.org/wiki/Law_of_Demeter
It is wrong for calendar to drill down into the daypicker's data and filter it. This way daypicker internal logic is spread all over the files in the project. Instead calendar should call a function on daypicker with the necessary parameters and ask it to return something.
| const currentIndex = this.dayPicker.focusableDays.findIndex(item => { | ||
| return CalendarDate.fromLocalJSDate(new Date(item.timestamp * 1000), this._primaryCalendarType).isSame(currentDate); | ||
| }); | ||
| this.dayPicker._itemNav.currentIndex = currentIndex; |
There was a problem hiding this comment.
"currentIndex" was never meant to be changed, ItemNavigation.prototype.update() is the public function to use if you want to change the currently selected item in the item navigation. What you're doing here is find the index of the item (with that complex expression) in order to pass it to item navigation, while there is a public function (update) that can get the item itself as an argument and it will internally find its index, and change the current index and also update the component. Is there any technical reason you're not using update()?
| @@ -676,7 +676,10 @@ class DayPicker extends UI5Element { | |||
| } | |||
|
|
|||
| _setCurrentItemTabIndex(index) { | |||
| this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString()); | |||
| const currentItem = this._itemNav._getCurrentItem(); | |||
There was a problem hiding this comment.
Changing DOM directly is one of the biggest antipatterns and we avoid this at almost any cost. The only times we consider this allowed is when an asynchronous update will be too late and a direct DOM update is needed immediately.
Is this the case here and if yes, why?
If it is not, the expected behavior would be to call ItemNavigation.prototype.update() with the item you want to set the focus to, then ItemNavigation will update the _weeks property and will invalidate the DayPicker, and then the new tabindex will be applied to the DOM as it will be bound in the .hbs template.
| @@ -240,7 +240,10 @@ class MonthPicker extends UI5Element { | |||
| } | |||
|
|
|||
| _setCurrentItemTabIndex(index) { | |||
| this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString()); | |||
| @@ -265,7 +265,10 @@ class YearPicker extends UI5Element { | |||
| } | |||
|
|
|||
| _setCurrentItemTabIndex(index) { | |||
| this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString()); | |||
|
As there are valuable comments in this change regarding the component implementation I would prefer not to merge this change |
Keyboard navigation now works properly, when ui5-datepicker component popover is opened and there are disabled dates in the corresponding ui5-calendar component due to min/max set.
Keyboard navigation now works properly, when ui5-datepicker
component popover is opened and there are disabled dates
in the corresponding ui5-calendar component, due to min/max dates set.