[lion-calendar] Selecting a date in the lion-input-datepicker with the space key immediately reopens the modal.

[Sciurus7]

Reproduction recipe

• Open a page containing a Lion input datepicker.
• Open the calendar by clicking the "Open date picker" button.
• Click and hold space
• Release the space key.

Expected behavior

After releasing the space key, the modal closes, selecting the focused day button and does not open again.

Actual Behavior

On the keydown event the modal closes selecting focused day button. After releasing the space key the modal opens again.

Note

On the page linked in the reproduction recipe you might have to hold down the space key for a second to reproduce the result. On our own extension of the lion-datepicker a quick click of the space key alwasy opens the modal instantly after closing it. I'm not sure what causes the discrepancy, though I do not think it is relevant for the main cause of the bug.

Our extension uses a lion-button instead of a normal button to invoke the calendar, so maybe that is the cause of the issue.

Additional context

Version of @lion/ui at time of testing is 0.8.6. Based on the git blame the bug is at least a year old.

Research notes

I believe the bug is caused by the __keyboardNavigationEvent method in lion-calendar.js which selects a day for space on enter and is bound to keydown. Given the calender day element are semantically buttons they should only be triggered on keyup for the space key.

A similar argument can be made for the enter key, though given the invoker button of the lion-input-datepicker is a native html button triggering the day button on the keydown for enter is not neccessarily wrong. (It means that my team which uses a lion-button as invoker instead will have to overwrite that behaviour somehow, but that could definitely be seen as an "us"-problem.)

[tlouisse]

Hi, good research. I couldn't reproduce it yet after a quick test in Chrome, but I can imagine this can happen looking at the code :)

Would you be willing to investigate a PR to change the behaviour? And actually what we should check for is this:

• do we receive both the keydown and keyup event?

In this fiddle you can verify thuis behavior: https://jsfiddle.net/7nkd3to1/3/

Maybe it's possible to abstract this in a utility somehow, that:

• keeps track of last keydown element
• keeps track of last keyup element
• registers a valid "key press" if both landed on the same element
• receives a root node (calendar in this case), so we can leverage event delegation for best perf

[tlouisse]

@Sciurus7 Do you think something like this would work?

/**
 * @example 
 * ```js
 *  // track buttons in calendar
 *  trackKeyDownAndUp(calendarRootNode, (({target, keyupEvent})) => {
 *     if (isCalendarButton(target)) {
 *       // here we make an event handler that previously relied on just keyup a bit more resilient
 *.      // and platform behavior abiding 
 *       handlePressOfCalendarBtn(keyupEvent)
 *.    }
 *  });
 * ```
 *
 * @param {Element} rootElem
 * @param {({target: Element; keydownEvent: KeyboardEvent; keyupEvent: KeyboardEvent;}) => void} onKeyDownAndUp
 */
export function trackKeyDownAndUp(rootElem, onKeyDownAndUp) {
  /** @type {KeyboardEvent} */
  let currentKeydownEvent;
  
  rootElem.addEventListener('keydown', (event) => {
    currentKeydownEvent = event;
  });
  
  // N.B. listen on window, as we can be outside root as well
  window.addEventListener('keyup', (event) => {
    if (currentKeydownEvent.target === event.target) {
      onKeyDownAndUp({target: event.target, keyupEvent: event, keydownEvent: currentKeydownEvent});
    }
    currentKeydownEvent = undefined;
  });
}

[Sciurus7]

Hi @tlouisse, first of all my apologies for the long silence on the issue. I have been swamped at work and have not been able to find time to look into your proposed solution until now. I did some research of my own and I do think your solution will do the trick.

One thing it fails to mimick from a normal button click event is that in a button click event you can press keydown with space, than click with some other key (say the 'e' key) and than release space to trigger the keyup event. This is still considered a valid click by html, but not by your solution. That being said, I believe this is corner casey enough to ignore.

[tlouisse]

Ah, good catch @Sciurus7! In that case it's better indeed if we mimic the native button behavior as close as possible.
I tested this on both chrome and firefox and we indeed should allow for keys inbetween. So the code above should be different indeed.

I was looking for a good test / spec for this, but couldn't find it. (this unfortunately tests keydown+up via webdriver sendKeys: https://github.com/web-platform-tests/wpt/blob/7089d2984688f8eca55d422fd5a016a754108eef/uievents/interface/keyboard-click-event.html#L44C6-L44C15).
Maybe you can find the official spec for this.
Otherwise I would indeed go for "count it as clicked when both keydown and keyup are released on same elem (given that keyup was not released outside elem)".

/**
 * @type {WeakMap<Element, (({target: Element; keyupEvent: KeyboardEvent; keydownEvent: KeyboardEvent; }) => void)[]>}
 */
const keyDownAndUpHandlers = new WeakMap();

/** @type {Record<string,KeyboardEvent>} */
const currentKeydownEventMapping = {};
  
window.addEventListener('keydown', (downEvent) => {
  currentKeydownEventMapping[downEvent.name] = downEvent;
}, true);
  
// N.B. listen on window, as we can be outside root as well
window.addEventListener('keyup', (upEvent) => {
  const [, downEvent] = Object.entries(currentKeydownEventMapping).find(([downEventName]) => downEventName === upEvent.name);
  if (!downEvent) return;
    
  if (downEvent.target !== upEvent.target) {
     currentKeydownEvent[downEvent.name] = undefined;     
     return;
  }
    
  for (const handler of keyDownAndUpHandlers.get(upEvent.target)) {
    handler({target: upEvent.target, keyupEvent: upEvent, keydownEvent: downEvent});
  }
}, true);

/**
 *
 * @example 
 * ```js
 *  // track buttons in calendar
 *  onKeyDownAndUp(calendarButtonElem, (({target, keyupEvent})) => {
 *       // here we make an event handler that previously relied on just keyup a bit more resilient
 *.      // and platform behavior abiding 
 *       handlePressOfCalendarBtn(keyupEvent)
 * });
 * 
 * @param {Element} targetElem
 * @param {({target: Element; keyupEvent: KeyboardEvent; keydownEvent: KeyboardEvent; }) => void} callback
 */
export function onKeyDownAndUp(targetElem, callback) {
   keyDownAndUpHandlers.set(targetElem, [...keyDownAndUpHandlers.get(targetElem) || [], callback]);
}

N.B. notice we listen on window (cheaper as we're using evt. delegation to its max) and use capture phase (making sure event bubbling is not canceled)