Skip to content
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

Jump to bottom

fix(ui5-calendar): autofocus on render #10739

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix(ui5-calendar): autofocus on render #10739

wants to merge 7 commits into from
+8 −40

Conversation

nnaydenow
Copy link
Contributor

Fixes: #10728

@vladitasev
Copy link
Contributor

The calendar should not auto-focus for sure (for reasons explained in the issue)

However, we must make sure all popup-based components (datepicker/datetimepicker come to mind) that took advantage of this auto-focusing behavior work correctly now). Please check.

@nnaydenow
Copy link
Contributor Author

nnaydenow commented Feb 3, 2025 •
edited
Loading

The calendar should not auto-focus for sure (for reasons explained in the issue)

However, we must make sure all popup-based components (datepicker/datetimepicker come to mind) that took advantage of this auto-focusing behavior work correctly now). Please check.

Compared to the behaviour in main branch and they are working in the same way.

vladitasev
vladitasev approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

looks good to me, maybe wait for someone from Team B to have a look too

hinzzx
hinzzx requested changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@hinzzx hinzzx left a comment

Choose a reason for hiding this comment

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

Focus behaviour when the ui5-calendar is with ui5-calendar-legend is not correct.

Navigating through the calendar header should focus either first or the last day of the calendar (1st if next month, last if previous month). Please refer:

Now

2025-02-03_14-52-14

Correct

2025-02-03_14-53-53

Also if a day is focused with tab, and the same action is performed (change month through header), when the month is changed the focus is perseved, which is incorrect, it should be again 1st day of the month, or the last one.

@vladitasev
Copy link
Contributor

vladitasev commented Feb 4, 2025 •
edited
Loading

There are 2 solutions IMO:

  1. Return the "auto focus" functionality conditionally (add a new protected property enableAutoFocus which will be explicitly set by ui5-date-picker and ui5-datetime-picker). For pure ui5-calendar this property will never be set, and this will fix the bug.
  2. We implement new focus functionality from the datepicker to propertly focus the first/last date, but this sounds more complicated to me.

@nnaydenow @hinzzx What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hinzzx hinzzx hinzzx requested changes

@vladitasev vladitasev vladitasev approved these changes

@tsanislavgatev tsanislavgatev Awaiting requested review from tsanislavgatev

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ui5-calendar]: takes page focus when rendered
3 participants
@nnaydenow @vladitasev @hinzzx