feat(accessibility): Adding attachment behavior to handle space/enter key

[kolaps33]

attachmentBehavior

There is requirement that Attachment component is clickable and when user navigate on, then by space/enter key invoke the onclick action.

What I changed:
Adding attachmentBehavior to specify space/enter key.
Adding onClick handling to Attachment component.

[levithomason]

I am unsure whether or not this PR is applicable, If I am understanding the goal here. It seems the goal is to forward keyboard invoked clicks (space/enter) on the root down the action slot. My concern are:

1. this seems to be the need of only one application and not representative of all implementations
2. there is an assumption that the action slot contains a single action that is also a context menu
3. it does not seem intuitive that root events should be forwarded to a child in this way
4. the user's action.onClick is called only when using object shorthand but not for elements nor when using the render callback

It may be that the consumer in this case can render as a button and get these behaviors for free. They would then handle the click to toggle a Popup. In fact, they may even be able to use the Attachment as the trigger to a Popup. I think we should investigate alternative approaches to this before merging.

I've left some comments in-line as well.

[jurokapsiar]

@levithomason we are totally open for ideas on this.

Yes, this might be just one use case, but that is exactly what the concept of behaviors allows us to do - we can provide multiple behaviors for the users to choose from. Question is what should be the default one.

Shift+F10 is a standard for conext menu on Windows, mac does not have a standard shortcut for this so people usually go for the same shortcut independently of the platform. As an alternative, you can make the Action trigger element focusable, but especially in cases when the attachments are organized in a grid and you allow arrow navigation to move focus between them, it becomes hard to focus something that is inside of the focusable element. A specific keyboard shortcut is easier to use and is also more standard (but not an ARIA-standard as this is not defined there).

This solves one specific case when the action is a context menu. We do not have a good solution for other actions yet.

[levithomason]

Let's update the Attachment component to have an actions prop and a contextMenu prop. The Context Menu component is not yet done, #345. For now, let's just use the behavior against the action prop. Once we have a Context Menu component, we'll update the Attachment to take a list of actions and a context menu, then update the behavior.

[levithomason]

Deferring review / approvals to @jurokapsiar

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@ba725b1). Click here to learn what that means.
The diff coverage is 47.61%.

@@            Coverage Diff            @@
##             master     #375   +/-   ##
=========================================
  Coverage          ?   87.58%           
=========================================
  Files             ?       42           
  Lines             ?     1474           
  Branches          ?      215           
=========================================
  Hits              ?     1291           
  Misses            ?      178           
  Partials          ?        5

Impacted Files	Coverage Δ
src/components/Attachment/Attachment.tsx	78.57% <47.61%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba725b1...236c396. Read the comment docs.

[jurokapsiar]

@layershifter can you pls look at this? All the wierdnes with stopPropagation needs to be done just because there is a Attachment <div> element and a <Button> inside of it. Both are clickable, but when you click on the button, it should not trigger the click on the parent <div>. Similarly, when user presses enter or space on the button, it should only click on that button, not on <div>. However, TAB key needs to be propagated because FocusZone has handlers that react on it. This requires a lot of knowledge on the consumer side. I'm wondering if there is a generic solution for these cases. I was also thinking about performing the action depending on the event target, but I think that is equally strange. What do you think?

fyi @sophieH29

[layershifter]

I previously did something like this, it's a little more obvious:

const stopPropagationOnKeys = (keys: number[]) => (e: Event) => {
   if (keys.includes(keyboardKey.getCode(e))) {
     e.stopPropagation()
   }
}

//

onKeyDown={stopPropagationOnKeys([keyboardKey.Enter, keyboardKey.Spacebar])}