Skip to content

fix(ui5-calendar): min max date keyboard navigation issue #2549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 16, 2020
Merged

fix(ui5-calendar): min max date keyboard navigation issue #2549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0988ff3
fix(ui5-datepicker): keyboard navigation works properly
unazko Dec 7, 2020
7a05764
Merge branch 'master' of https://github.com/unazko/ui5-webcomponents …
unazko Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/main/src/Calendar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,9 +631,11 @@ class Calendar extends UI5Element {

async _setDayPickerCurrentIndex(calDate, applyFocus) {
await RenderScheduler.whenFinished();
const currentDate = new CalendarDate(calDate);
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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return CalendarDate.fromLocalJSDate(new Date(item.timestamp * 1000), this._primaryCalendarType).isSame(currentDate);
});
this.dayPicker._itemNav.currentIndex = currentIndex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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()?

if (applyFocus) {
this.dayPicker._itemNav.focusCurrent();
} else {
Expand Down
11 changes: 7 additions & 4 deletions packages/main/src/DayPicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ class DayPicker extends UI5Element {
}

currentTimestamp = calDate.valueOf() / 1000;
const newItemIndex = this._itemNav._getItems().findIndex(item => parseInt(item.timestamp) === currentTimestamp);
const newItemIndex = this.focusableDays.findIndex(item => parseInt(item.timestamp) === currentTimestamp);

this._itemNav.currentIndex = newItemIndex;
this._itemNav.focusCurrent();
Expand Down Expand Up @@ -660,7 +660,7 @@ class DayPicker extends UI5Element {
const focusableDays = [];

for (let i = 0; i < this._weeks.length; i++) {
const week = this._weeks[i].slice(1).filter(x => !x.disabled);
const week = this._weeks[i].slice(1).filter(dayItem => !dayItem.disabled);
focusableDays.push(week);
}

Expand All @@ -676,7 +676,10 @@ class DayPicker extends UI5Element {
}

_setCurrentItemTabIndex(index) {
this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString());
const currentItem = this._itemNav._getCurrentItem();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (currentItem) {
currentItem.setAttribute("tabindex", index.toString());
}
}

_modifySelectionAndNotifySubscribers(timestamp) {
Expand Down Expand Up @@ -841,7 +844,7 @@ class DayPicker extends UI5Element {
this.fireEvent("navigate", { timestamp });
await RenderScheduler.whenFinished();

const newItemIndex = this._itemNav._getItems().findIndex(item => parseInt(item.timestamp) === timestamp);
const newItemIndex = this.focusableDays.findIndex(item => parseInt(item.timestamp) === timestamp);
this._itemNav.currentIndex = newItemIndex;
this._itemNav.focusCurrent();
}
Expand Down
5 changes: 4 additions & 1 deletion packages/main/src/MonthPicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,10 @@ class MonthPicker extends UI5Element {
}

_setCurrentItemTabIndex(index) {
this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

const currentItem = this._itemNav._getCurrentItem();
if (currentItem) {
currentItem.setAttribute("tabindex", index.toString());
}
}

_onmousedown(event) {
Expand Down
5 changes: 4 additions & 1 deletion packages/main/src/YearPicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,10 @@ class YearPicker extends UI5Element {
}

_setCurrentItemTabIndex(index) {
this._itemNav._getCurrentItem().setAttribute("tabindex", index.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

const currentItem = this._itemNav._getCurrentItem();
if (currentItem) {
currentItem.setAttribute("tabindex", index.toString());
}
}

_onmousedown(event) {
Expand Down
17 changes: 17 additions & 0 deletions packages/main/test/specs/DatePicker.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,4 +909,21 @@ describe("Date Picker Tests", () => {
assert.strictEqual(date.getMonth(), 0, "Correct month value");
assert.strictEqual(date.getFullYear(), 2000, "Correct year value");
});

it("Keyboard navigation works when there are disabled dates in the calendar grid", () => {
datepicker.id = "#dp33";
datepicker.innerInput.click();
browser.keys("Jan 1, 2000");

datepicker.valueHelpIcon.click();

browser.keys("ArrowDown");

assert.ok(datepicker.getDisplayedDay(13).isFocusedDeep(), "Successfully navigated");

browser.keys("Escape");
datepicker.innerInput.click();
browser.keys(["Control", "A"]);
browser.keys("Backspace");
});
});