feat(FocusZone): Implement FocusZone into renderComponent #116

tomasiser · 2018-08-20T16:03:18Z

Implement FocusZone into renderComponent

Hi, this PR does the following:

adds @uifabric/utilities into package.json,
adds a local copy of Fabric FocusZone into lib/accessibility,
adds a new focusZone key into IAccessibilityDefinition,
modifies renderComponent.ts to wrap a component in <FocusZone ...>...</FocusZone> if it's set in its accessibility properties,
modifies MenuBehavior.ts to enable keyboard navigation in menu and show how FocusZone works,

and modifies unit tests the following:

modifies handlesAccessibility.tsx: adds unified TestBehavior and fixes FocusZone wrapping,
modifies isConformant.tsx to fix FocusZone wrapping,
adds unit test for FocusZone based on Fabric FocusZone.test.tsx.

The user of the FocusZone has to set up the focus zone in a corresponding xxxBehavior.ts file like so:

import { Accessibility, FocusZoneMode } from '../../interfaces'

const MenuBehavior: Accessibility = {
  attributes: {
    root: {
      role: 'menu',
    },
  },
  focusZone: {
    mode: FocusZoneMode.Wrap,
    props: {
      /* properties of <FocusZone {...props} /> */
    },
  },
}

export default MenuBehavior

The FocusZoneMode.Wrap means that the component will be wrapped by the FocusZone, creating an additional <div> around it and handling the focus. We might need to add a different mode in the future like FocusZoneMode.Embed but we are not sure yet. A consumer of a component may overload the accessibility by using FocusZoneMode.Custom.

codecov · 2018-08-21T15:54:53Z

Codecov Report

Merging #116 into master will increase coverage by 21.18%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #116       +/-   ##
===========================================
+ Coverage   67.86%   89.04%   +21.18%     
===========================================
  Files         101       47       -54     
  Lines        1366      776      -590     
  Branches      270      101      -169     
===========================================
- Hits          927      691      -236     
+ Misses        437       83      -354     
  Partials        2        2

Impacted Files	Coverage Δ
src/components/Input/Input.tsx	`78.68% <0%> (-6.5%)`	⬇️
src/components/Avatar/Avatar.tsx	`91.17% <0%> (-0.26%)`	⬇️
src/components/Chat/Chat.tsx	`94.73% <0%> (ø)`	⬆️
src/components/Menu/MenuItem.tsx	`92.3% <0%> (ø)`	⬆️
src/index.ts	`100% <0%> (ø)`	⬆️
src/components/Accordion/AccordionTitle.tsx	`65% <0%> (ø)`	⬆️
src/components/Button/Button.tsx	`93.93% <0%> (ø)`	⬆️
src/components/Icon/Icon.tsx	`100% <0%> (ø)`	⬆️
src/components/Header/Header.tsx	`95% <0%> (ø)`	⬆️
src/components/Header/HeaderDescription.tsx	`100% <0%> (ø)`	⬆️
... and 64 more

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 20b6681...25092d4. Read the comment docs.

sophieH29 · 2018-08-22T08:14:31Z

src/lib/accessibility/Behaviors/Menu/MenuBehavior.ts

+    mode: FocusZoneMode.Wrap,
+    props: {
+      onActiveElementChanged: (element, ev) => {
+        console.error('MENU BEHAVIOR', 'on active element changed', 'element', element, 'ev', ev)


Do we need to have this? We'll get many errors into console then :)

Good catch, we should decide if this PR should modify MenuBehavior.ts or not. If yes, then we probably do not need the props at all? @jurokapsiar

I think for now you can set in props isCircularNavigation: true, as it's a requirement by ARIA documentation
Also, It might be in separate PR, but would be good to have VerticalMenuBehavior as we have for VerticalMenuItemBehavior, so in props can be set directions:

props: { direction: FocusZoneDirection.horizontal, // vertical for Vertical behavior isCircularNavigation: true, },

agree with @sophieH29, we should include props, but not onActiveElementChanged

and horizontal/vertical menu should be in a separate PR. for now let's just include isCircularNavigation

sophieH29 · 2018-08-22T08:22:27Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

@@ -8,7 +8,7 @@ const MenuItemBehavior: Accessibility = (props: any) => ({
    anchor: {
      role: 'menuitem',
      'aria-expanded': props['submenuOpened'],
-      tabIndex: '0',
+      'data-is-focusable': true,


I am having a concern of using hardcode attribute which is directly related to FocusZone. So if it changes in Focus, we'll have to update all the behaviors.

What do you think if we move data-is-focusable to interfaces.ts?

export const IS_FOCUSABLE_ATTRIBUTE = 'data-is-focusable'

and use it in behaviors

... [IS_FOCUSABLE_ATTRIBUTE]: 'true' ...

@tomasiser @jurokapsiar

…at/acc-focuszone-in-rendercomponent

mnajdova · 2018-08-23T10:33:20Z

test/specs/commonTests/isConformant.tsx

+        .childAt(0) // <div> inside FocusZone wrap
+        .childAt(0) // the actual component
+    }
+    return component


Do we have idea how will the Custom FocusZone be handled? Is it going to be completely different implementation for each custom zone, where in some cases there will be wrappers and in some other won't. I am a bit scared about how we would be able to provide the rendered component itself in those cases.

Should we test here the mode wrap in this case as well?

I think the idea of FocusZoneMode is still a little bit uncertain. The custom FocusZoneMode is that the consumer of Stardust would provide his own functions, meaning that it will actually never affect these unit tests. I think @jurokapsiar could provide more details? Should we modify this test to check FocusZoneMode?

yes - custom means that the focus zone functionality is handled outside of Stardust. We have test (handlesAccessibility for menu-test) that checks if the component is correctly wrapped in the focus zone. The isConformant just skips FocusZone if it finds it. Currently we do not have tests that test only the behaviors - we just have tests that check, if the behaviors correctly modify particular components. It would be useful to 3 tests that just check if renderComponent uses FocusZone correctly - one for wrap, one for custom and one without FocusZone. Is this what you meant @mnajdova?

I think that the custom mode is basically equivalent to disabling FocusZone. Is that right, @jurokapsiar? In that case, maybe we can completely eliminate the FocusZoneMode for now. 🤔

@jurokapsiar you are right that's what I meant, explicitly handing having mode wrap, custom or no focus zone, but I was uncertain what is the expected result of having custom focus zone. If it all sums up to having this condition, then we are good to stay with this condition as is.

Ok, let's leave it as it is for now. :)

mnajdova · 2018-08-23T10:38:30Z

test/specs/commonTests/isConformant.tsx

+        .prop('className')
+      return classes
+    }
+


Thanks for refactoring this!

mnajdova · 2018-08-23T10:43:15Z

test/specs/commonTests/handlesAccessibility.tsx

  const target = partSelector
    ? renderedComponent.render().find(partSelector)
    : renderedComponent.render()

-  return target.first().prop(propName)
+  let node = target.first()
+  if (isWrappedInFocusZone) {


I would add here check for the node, as using partSelector might return nothing if it doesn't exists in the component. If the intention is for the test to fall in that case, I would rather handle that and throw explicit exception explaining what was the cause for the test to fail.

Thanks! So I investigated and actually, render() should return Cheerio objects and Cheerio has a chaining API which always returns another Cheerio object, meaning it can never be undefined and you can call functions even on empty Cheerio wrappers.

So what happens if no element is matched is that the whole getProp function would just return undefined from the last .prop(...) call, which is exactly how it worked before my PR.

I guess it's ok to keep it as it is?

Yes, should be ok then.

miroslavstastny · 2018-08-23T12:46:38Z

src/lib/renderComponent.tsx

@@ -93,7 +115,7 @@ const renderComponent = <P extends {}>(
        const classes: IComponentPartClasses = getClasses(renderer, mergedStyles, styleParam)
        classes.root = cx(className, classes.root, props.className)

-        const accessibility = getAccessibility(props, state)
+        const accessibility = getAccessibility(props, state) as IAccessibilityDefinition


I would modify getAccessibility() to return IAccessibilityDefinition and remove the cast here.

Agree, done! :)

…in-rendercomponent

…b.com/stardust-ui/react into feat/acc-focuszone-in-rendercomponent

…in-rendercomponent

codecov · 2018-08-30T09:00:07Z

Codecov Report

Merging #116 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #116   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files          61       61           
  Lines        1028     1028           
  Branches      136      136           
=======================================
  Hits          941      941           
  Misses         83       83           
  Partials        4        4

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 53a453c...10025e6. Read the comment docs.

src/lib/accessibility/FocusZone/FocusUtilities.ts

src/lib/accessibility/FocusZone/FocusZone.tsx

src/lib/accessibility/FocusZone/CHANGELOG.md

src/lib/accessibility/FocusZone/FocusZone.tsx

src/lib/accessibility/FocusZone/FocusZone.types.ts

test/specs/commonTests/handlesAccessibility.tsx

src/lib/accessibility/FocusZone/CHANGELOG.md

src/lib/renderComponent.tsx

src/lib/accessibility/FocusZone/FocusZone.tsx

src/lib/accessibility/FocusZone/CHANGELOG.md

src/lib/accessibility/FocusZone/FocusZone.tsx

…in-rendercomponent # Conflicts: # src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts # src/lib/accessibility/interfaces.ts # src/lib/renderComponent.tsx # test/specs/components/Menu/Menu-test.tsx

…in-rendercomponent

miroslavstastny

RTL support and more Behaviors will be added in separate PRs.

…in-rendercomponent

tomasiser added 3 commits August 20, 2018 16:20

Add @uifabric/utilities

faa40f4

customPropTypes fix for @types

41f022d

FocusZone in renderComponent

b486711

tomasiser requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 20, 2018 16:03

tomasiser added 2 commits August 21, 2018 16:33

Fixed handlesAccessibility unit test for FocusZone wrap

d445a4c

Fixed isConformant.tsx unit tests for FocusZone

4889064

tomasiser requested a review from miroslavstastny as a code owner August 21, 2018 15:50

sophieH29 reviewed Aug 22, 2018

View reviewed changes

tomasiser requested a review from jurokapsiar August 22, 2018 08:42

tomasiser added 2 commits August 22, 2018 10:57

Merge branch 'master' of https://github.com/stardust-ui/react into fe…

ff124af

…at/acc-focuszone-in-rendercomponent

Removed debug logging, added circular navigation to menu

9356364

tomasiser changed the title ~~[RFC] Implement FocusZone into renderComponent~~ [RFC] feat(FocusZone): Implement FocusZone into renderComponent Aug 22, 2018

tomasiser added 2 commits August 22, 2018 13:17

Fix menu props and add constant

3d63a14

FocusZone unit tests

e9f1cf1

tomasiser changed the title ~~[RFC] feat(FocusZone): Implement FocusZone into renderComponent~~ feat(FocusZone): Implement FocusZone into renderComponent Aug 22, 2018

tomasiser added the 🚀 ready for review label Aug 22, 2018

mnajdova reviewed Aug 23, 2018

View reviewed changes

tomasiser self-assigned this Aug 23, 2018

miroslavstastny reviewed Aug 23, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Aug 23, 2018

tomasiser added 2 commits August 24, 2018 11:33

Improved typing of a function

518e3f8

Merge remote-tracking branch 'origin/master' into feat/acc-focuszone-…

59a05af

…in-rendercomponent

tomasiser added 9 commits August 29, 2018 09:54

More cleanup

e073afe

Added PropTypes, fixed unit tests

a07c290

Renamed props and refactored unit tests

f3e0f19

Typo in test

0e6825d

Temporary fix for customPropTypes.as not working

772df90

Updated FocusZone CHANGELOG

441c135

Merge branch 'feat/acc-focuszone-in-rendercomponent' of https://githu…

0e01d04

…b.com/stardust-ui/react into feat/acc-focuszone-in-rendercomponent

Strict type

10f7aff

Merge remote-tracking branch 'origin/master' into feat/acc-focuszone-…

25092d4

…in-rendercomponent

sophieH29 reviewed Aug 30, 2018

View reviewed changes

src/lib/accessibility/FocusZone/FocusUtilities.ts Outdated Show resolved Hide resolved

sophieH29 reviewed Aug 30, 2018

View reviewed changes

src/lib/accessibility/FocusZone/FocusZone.tsx Outdated Show resolved Hide resolved

sophieH29 reviewed Aug 30, 2018

View reviewed changes

src/lib/accessibility/FocusZone/FocusZone.tsx Outdated Show resolved Hide resolved

Added isRtl, moved constants

1676c56

mnajdova reviewed Sep 5, 2018

View reviewed changes

src/lib/accessibility/FocusZone/CHANGELOG.md Outdated Show resolved Hide resolved

miroslavstastny reviewed Sep 10, 2018

View reviewed changes

tomasiser added 6 commits September 11, 2018 10:10

Merge remote-tracking branch 'origin/master' into feat/acc-focuszone-…

cc3fdb0

…in-rendercomponent # Conflicts: # src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts # src/lib/accessibility/interfaces.ts # src/lib/renderComponent.tsx # test/specs/components/Menu/Menu-test.tsx

Fixed FocusZone return values

d1c1831

Fixed prop types

4f85192

Typos and styling changes

d4228da

Fixed ElementType and focusLast

69083ea

Merge remote-tracking branch 'origin/master' into feat/acc-focuszone-…

d873f09

…in-rendercomponent

miroslavstastny approved these changes Sep 11, 2018

View reviewed changes

miroslavstastny added ready for merge and removed 🚀 ready for review labels Sep 11, 2018

tomasiser added 3 commits September 12, 2018 09:25

Merge remote-tracking branch 'origin/master' into feat/acc-focuszone-…

c3e40fe

…in-rendercomponent

Changelog

e90054d

Update changelog

10025e6

tomasiser merged commit 2cfa283 into master Sep 12, 2018

sophieH29 deleted the feat/acc-focuszone-in-rendercomponent branch November 14, 2018 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(FocusZone): Implement FocusZone into renderComponent #116

feat(FocusZone): Implement FocusZone into renderComponent #116

tomasiser commented Aug 20, 2018 •

edited

codecov bot commented Aug 21, 2018 •

edited

sophieH29 Aug 22, 2018

tomasiser Aug 22, 2018

sophieH29 Aug 22, 2018 •

edited

jurokapsiar Aug 22, 2018

jurokapsiar Aug 22, 2018

sophieH29 Aug 22, 2018

jurokapsiar Aug 22, 2018

mnajdova Aug 23, 2018

mnajdova Aug 23, 2018

tomasiser Aug 23, 2018

jurokapsiar Aug 23, 2018

tomasiser Aug 23, 2018

mnajdova Aug 23, 2018

tomasiser Aug 24, 2018

mnajdova Aug 23, 2018

mnajdova Aug 23, 2018

tomasiser Aug 24, 2018

mnajdova Sep 5, 2018

miroslavstastny Aug 23, 2018

tomasiser Aug 24, 2018

codecov bot commented Aug 30, 2018 •

edited

miroslavstastny left a comment

feat(FocusZone): Implement FocusZone into renderComponent #116

feat(FocusZone): Implement FocusZone into renderComponent #116

Conversation

tomasiser commented Aug 20, 2018 • edited

Implement FocusZone into renderComponent

codecov bot commented Aug 21, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 Aug 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2018 • edited

Codecov Report

miroslavstastny left a comment

Choose a reason for hiding this comment

tomasiser commented Aug 20, 2018 •

edited

codecov bot commented Aug 21, 2018 •

edited

sophieH29 Aug 22, 2018 •

edited

codecov bot commented Aug 30, 2018 •

edited