feat(Accessibility): Add accessibility behavior to Popup

[sophieH29]

This PR aims to make popup accessible, by adding PopupBehavior to the Popup component.

For accessibility purpose, we should be able to manage open/close state of the Portal from Popup component. So in th PR I've added popupState to the Popup which controlls open prop of the Portal.

PopupBehavior details:

• aria-* attributes - popup's trigger will receive attributes like 'aria-haspopup'
• keyActions
  - for the trigger component's part - performClick (on Enter & Space), which currently will toggle open/close state)
  - for popup component's part - closeAndFocusTrigger - when user presses Esc on a popup, it will be closed and the trigger node will be focused

Extended Portal's API

• triggerAccessibility - receives accessibility related attributes, like

{
  aria-haspopup: true,
  aria-disabled: false,
  onKeyDown: f()
}

• onOutsideClick - callback whick can be invoked when document was clicked outside of trigger and popup nodes
• onTriggerClick - since it's needed to control open state from popup, we have to handle trigger click in a popup too

Suggestion, probably for future:

• extend Popup API, by adding props onOpen, onClose, so users can controll what should be done before opening and closing a popup, for e.g. managing focus

[codecov]

Codecov Report

Merging #218 into master will decrease coverage by 0.63%.
The diff coverage is 73.52%.

@@            Coverage Diff            @@
##           master    #218      +/-   ##
=========================================
- Coverage   92.44%   91.8%   -0.64%     
=========================================
  Files          63      63              
  Lines        1151    1171      +20     
  Branches      151     173      +22     
=========================================
+ Hits         1064    1075      +11     
- Misses         84      92       +8     
- Partials        3       4       +1

Impacted Files	Coverage Δ
src/components/Portal/index.ts	100% <100%> (ø)	⬆️
src/components/Portal/Portal.tsx	100% <100%> (ø)	⬆️
src/components/Menu/MenuItem.tsx	89.28% <100%> (ø)	⬆️
src/components/Popup/positioningHelper.ts	100% <100%> (ø)	⬆️
src/components/Popup/Popup.tsx	84.21% <66.66%> (-15.79%)	⬇️

---

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 b71d78d...051509a. Read the comment docs.

[kuzhelov]

great improvement - lets address (discuss) the problem related to triggerAccessibility object's type, and it seems that we would be good to go :)

[kuzhelov]

another related problem (to the fact that there is no enforcement over the triggerAccessibility type on the client-code side) is that currently it is very easy to accidentally do things wrong: it is sufficient to add any property to triggerAccessibility object that cannot be properly expanded to element's attributes to break the entire app. For the sake of argument, you could try the following for Popup.tsx (it won't be caught by the compiler now):

return (
      <ElementType
       ... 
        triggerAccessibility={{
          ...accessibility.attributes.trigger,
          ...accessibility.keyHandlers.trigger,
          someFoo: { bar: true }
        }}
       ...

I would really suggest to introduce some type to restrict the set of possible values, and use it on the client side as well - the least we can do for now:

// in Popup
  return (
      <ElementType
       ... 
        triggerAccessibility={{
          ...accessibility.attributes.trigger,
          ...accessibility.keyHandlers.trigger,
        } as TriggerAccessibility}

// in types declaration
type TriggerAccessibility =  IAccessibilityAttributes & OnKeyDownHandler // or better { attributes: ..., handlers: ...}

[sophieH29]

I'll make this type

type TriggerAccessibility = {
  attributes?: IAccessibilityAttributes
  keyHandlers?: OnKeyDownHandler
}

[sophieH29]

@kuzhelov Thank you for your review! Made changes regarding triggerAccessebility type

[kuzhelov]

lets, please, make explicit as cast here (and use TriggerAccessibility type) - this will ensure that it would be hard to accidentally break the app with new props being added to either attributes or handlers

[kuzhelov]

lets, please, ensure that it would be hard to break the app with new property being added to either attributes or keyHandlers part of the triggerAccessibility object - have left a comment. Other than that we are good to merge 👍

Also, please, do not forget to mention the changes made in the CHANGELOG file.