fix(editor): restore focus after closing event popover

[abhinavohri]

Summary

When the simple editor is dismissed without creating an event, keyboard focus jumps to the top of the page. This PR returns focus to the element that was focused before the editor opened.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[SebastianKrupinski]

Hi @abhinavohri

The ticket you linked has nothing to do with the changes you made. The ticket is about the calendar creation menu.

[abhinavohri]

Hi @SebastianKrupinski , my apologies I mixed up the issues.

I followed the steps in the #4127, and the focus went back to the + button. So I think that issue is already fixed.

If this change is still useful, should I open a new issue and update this PR to reference it, or would you prefer that I close the PR?

[SebastianKrupinski]

@abhinavohri

I can't really tell if this PR is useful as I don't have a reference of an issue. What issue does this PR solve?

[abhinavohri]

I can't really tell if this PR is useful as I don't have a reference of an issue. What issue does this PR solve?

When the simple editor is dismissed without creating an event, keyboard focus jumps to the top of the page. This PR returns focus to the element that was focused before the editor opened.

[SebastianKrupinski]

@odzhychko

What do you think?

[odzhychko]

In general, it is good accessibility to return the focus after a modal is closed.

This is why NcModal uses the focus-trap library.

For the simple editor event popover I currently see three separate cases:

1. It is created by clicking the "+ Event" button.

   In this case the focus should return to the button.
   This should be automatically happen if we use NcModal.
   Here it it worth investigation, why it does not happen.
   I suspect we use NcModal not as intended.

2. User clicks on existing event, which opens preview and then user can click "Edit"

   When use closes the editor, focus should return to the event where the user clicked.
   Also here the focus trap mechanism of NcModal should work out-of-the box, but doesn't.
   If it has to do with some way fullcalendar works, NcModal.setReturnFocus seems add custom focus return logic.

3. User clicks (or drags) somewhere on the calendar which opens a dialog for a new event.

   A sensible default would be to focus the day in the fullcalendar view.
   NcModal.setReturnFocus seems to be the indented way to control this.

In summery: It would be nice to have focus returning implemented correctly eventually, but the current proposed solution seems not the correct one. Please check first, why the NcModal does not behave as expected and use NcModal.setReturnFocus in case we really have to intervene manually.

[abhinavohri]

Hi @odzhychko , please correct me if I am missing something. The EditSimple popover does not use NcModal. Only some dialogs opened from it do.

I tested the three cases, and my change restores focus only for the “+ Event” button. It does not restore focus for an existing event or a calendar selection.

[odzhychko]

The EditSimple popover does not use NcModal. Only some dialogs opened from it do.

Oh, right. I just assume it usesNcModal...

Eventually we should refactor to NcModal.

I tested the three cases, and my change restores focus only for the “+ Event” button.

That's better then non of the cases ^^
The other cases are tricky because we open them from fullcalendar components somehow.

Just for completeness, I now also though of:

• When previewing event info (click of event in editor) is a popover and NcPopover could be used.
  NcPopover also has focus trapping (and customization with setReturnFocus) build-in.
• Some more cases I could think of:
  • Case 4: simple event editor is opened for existing event through URL navigation
    • Possible focus closing: the created event
  • Case 5: simple event editor is opened for new event through URL navigation
    • Possible focus after saving and closing: the created event
    • Possible focus after dismissal: new event button
  • Case 6: simple event editor is opened and event is deleted
    • Possible focus after dismissal: Focus the day like in case 3.
  • Case 7: All other case but when the full editor is directly opened or or the users transitions from the simple editor to the full editor

@SebastianKrupinski @abhinavohri I'm fine with merging this without full refactoring, even if it works only for case 1.

@abhinavohri In case you do not want to not handle the other cases or the refactoring in this PR, could you adapt the commit, PR title and description to reflect that? e.g. something like "restore focus to event creation button"

@abhinavohri Could you add an e2e-test. Else this feature might be quickly lost when someone tries to refactor to NcModal (or rework the component in major way).
(Might be as simple as adding one addition assertion at the end of test('create an event'.
CalendarPage could also expose .buttonNewEvent to encapsulate and avoid duplication of locator logic.)

@abhinavohri Out of curiosity: While looking at the code of the simple editor have you seen some reason that would block refactoring to to NcModal? Maybe some feature/behaviour that would not provided by it?

[abhinavohri]

Hi @odzhychko , I have updated the PR title and commit and have also added the E2E test.

Regarding refactoring EditSimple to NcModal, I can think of the following things to be blockers:

• EditSimple dynamically positions itself next to clicked events, whereas NcModal centers the dialog in the viewport.
• EditSimple skips the backdrop when viewing events (!isViewing). NcModal renders a screen mask unconditionally.
• There may also be focus-trap issues with internal nested popovers and dialogs.