FocusZone: Add the ability for directional no wrapping #3962

jspurlin · 2018-02-13T17:13:08Z

Pull request checklist

Include a change request file using $ npm run change

Description of changes

For FocusZones that contain multiple navigation models (especially when one is a super set of the other) the resulting navigation can muddy. For example, if a ContextualMenu contains a section with 2d layout and another section with 1d layout, the FocusZone is bidirectional to allow for the 2d navigation but this makes left/right naviagate the 1d section of the menu (and additionally breaks the submenu arrow collapse behavior). The change adds a check for data-no-horizontal-wrap and data-no-vertical-wrap on the element the FocusZone is acting on (it check the element itself as well and all ancestors until it finds a match or hits the body). Additionally this adds a checkForNoWrap prop to FocusZone and ContextualMenu so that we won't do the searching for the data attributes unless the creator specifies that we should be looking for it.

This is a before behavior: the vertical wrapping when the focus gets to the top of the 2d section (e.g. focus moves to the left instead of wrapping); also note that the navigation in the 1d layout is moving when left/right are pressed (and pressing left in the 1d layout does not collapse the submenu)

This is the after behavior: the vertical wrapping in the 2d layout, the horizontal wrapping in the 1d layout, and the ability for the submenu to collapse with the arrow keys are all working as expected now

Focus areas to test

Verified that existing behavior has not changed at all, and the new behavior is scoped to only when the user explicitly wants that behavior.

…y) when they get to the bounds of the zone

…c-react into jspurlin/FocusZoneAddWrapProps

joschect · 2018-02-14T22:43:20Z

I want to try to avoid adding a bunch of new properties to contextualmenu's overall. It seems like having a catch all focuszoneprops would be a good way to help cut down on the clutter and provide some of this functionality.

I think the changes to the focus zone could be useful, but for now this same problem could be solved by custom rendering an item which contains a focus zone and then controlling which keypresses trigger an entry into the subfocuszone. I don't know if that's the right way to accomplish things either but it would allow users to do this without having a more complicated focuszone.

jspurlin · 2018-02-14T23:14:29Z

Yeah, I thought about adding a focusZoneProps which should include deprecating arrowDirection, not opposed to making this change

I attempted the nested focus zone approach but it was overly complicated and didn't solve all of the issues I was running in to.

I had chatted with @dzearing about approaches to solve this issue we landed on the approach I implemented. @joschect let's chat offline if you have an idea on how get the desired end result: where you can have an vertical outer focus zone with a bidirectional focus zone inside (for the 2d content) where focus flows between them as if they were one focus zone (especially when entering the bidirectional focus zone from top and bottom), the keyboarding doesn't move left when you press up at the top of a 2d set of controls (when you are also not the control in the left-most column. In the before gif (in the description) this happens starting when focus is on the airplane), and collapses submenus when focus is in the 1d portion and the appropriate left/right arrow key is presssed (which you should get for free with a vertical focus zone)

dzearing · 2018-02-16T02:46:11Z

common/changes/@uifabric/utilities/jspurlin-FocusZoneAddWrapProps_2018-02-13-16-53.json

+  "changes": [
+    {
+      "packageName": "@uifabric/utilities",
+      "comment": "Focus/DOM: add the ability to find if an element (or any ancestor) contians a  given attribute;  to the dom utility ; add  a shouldrWapFocus  function to  the focus utility ",


These go into the release notes, and you have some extra spaces and typos

dzearing · 2018-02-16T23:05:22Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

    getDistanceFromCenter: (activeRect: ClientRect, targetRect: ClientRect) => number,
-    ev?: Event): boolean {
+    ev?: Event,
+    shouldWrap: boolean = true): boolean {


it is weird to default this to true. it should default to false.

I think the name is misleading, I'll update it to be useDefaultWrap so that it's more clear that when this is true it will do the default wrapping behavior

dzearing · 2018-02-16T23:05:56Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

        distance = activeRect.right - targetRect.right;
+      } else {
+        if (!shouldWrap) {
+          distance = -2;


I don't quite understand the -2

It's just one less than -1 :) I needed to use a more negative number (for the distance from center) to keep the loop going in. I pulled out a const for both the upper value (that already existed) and the lower value that I added. The large value picks the element as a potential element, while the negative number is for not picking that element

dzearing · 2018-02-16T23:14:03Z

packages/utilities/src/dom.ts

+ * @param attribute - the attribute to search for
+ * @returns the value of the first instance found
+ */
+export function elementContainsAttribute(element: HTMLElement, attribute: string): string | undefined {


I feel like I've seen this code repeated elsewhere.

Could we just have one method to find a node which matches some criteria?

Maybe findElementRecursive(cb: (el) => boolean): HTMLElement or something:

// hmm better name? findElementRecursive(el => el.hasAttribute(attr));

Then you could reuse the "go up the parents" logic elsewhere.

getElementAttributeRecursive(el, attr) {
const el = findElementRecursive(el => el.hasAttribute(attr));

return el && el.getAttribute(attr);
}

Yeah, I'll look into this

updated the contents of the change file

add missing comma

dzearing · 2018-02-17T19:28:53Z

packages/utilities/src/dom.ts

+ * @returns the value of the first instance found
+ */
+export function elementContainsAttribute(element: HTMLElement, attribute: string): string | null {
+  let elementMatch = findElementRecursive(element, element => element.hasAttribute(attribute));


ERROR: 228:52 typedef expected arrow-parameter: 'element' to have a typedef
ERROR: 228:52 no-shadowed-variable Shadowed name: 'element'

maybe I can inline fix it for you

dzearing · 2018-02-17T19:29:16Z

packages/utilities/src/dom.ts

 }

+/**
+ * Finds the first parent element where the matchFunctionReturns true


dzearing · 2018-02-17T19:33:39Z

packages/utilities/src/dom.ts

+ * @returns the value of the first instance found
+ */
+export function elementContainsAttribute(element: HTMLElement, attribute: string): string | null {
+  let elementMatch = findElementRecursive(element, (testElement: HTMLElement) => testElement.hasAttribute(attribute));


i fixed the lint errors here. Also fixed the comment typo above.

One other note: on 218, I changed element.parent to getParent(element). This allows for virtual parent chains to be respected.

…c-react into jspurlin/FocusZoneAddWrapProps

jspurlin · 2018-02-22T02:11:40Z

@dzearing not sure why there's some screener changes, it's showing some slight changes for date pickers but I'm not changing any styling....

…c-react into jspurlin/FocusZoneAddWrapProps

…c-react into jspurlin/FocusZoneAddWrapProps # Conflicts: # scripts/package.json

dzearing · 2018-02-22T06:43:51Z

Large_Negative_DISTANCE_FROM_CENTER.... hmmmmmmmm

jspurlin · 2018-02-22T06:46:02Z

yep, fixed the casing and am trying to push the update

…spurlin/office-ui-fabric-react into jspurlin/FocusZoneAddWrapProps

jspurlin added 5 commits February 12, 2018 19:45

Add the ability for FocusZones to not wrap (vertically or horizontall…

6aca76d

…y) when they get to the bounds of the zone

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

968c74a

…c-react into jspurlin/FocusZoneAddWrapProps

update the example to make it more clear the changes I'm making

3450460

rush change

9beb6c0

remove unneeded resolution

3f4b516

jspurlin requested review from christiango, dzearing and whykushal93 February 13, 2018 17:13

jspurlin requested a review from joschect as a code owner February 13, 2018 17:13

jspurlin added 2 commits February 13, 2018 10:52

Remove an extra change from a different issue I'm working on

46a18bc

merge

f7c07c8

manishgarg1 assigned dzearing Feb 14, 2018

dzearing reviewed Feb 16, 2018

View reviewed changes

jspurlin added 3 commits February 16, 2018 16:58

Update jspurlin-FocusZoneAddWrapProps_2018-02-13-16-53.json

f8e6dd5

updated the contents of the change file

Update jspurlin-FocusZoneAddWrapProps_2018-02-13-16-53.json

4b9902a

add missing comma

Address feedback

85b4e1a

dzearing reviewed Feb 17, 2018

View reviewed changes

packages/utilities/src/dom.ts Outdated

}

/**

* Finds the first parent element where the matchFunctionReturns true

Copy link

Member

dzearing Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Update dom.ts

011f551

dzearing reviewed Feb 17, 2018

View reviewed changes

jspurlin added 5 commits February 18, 2018 14:57

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

7a3f90c

…c-react into jspurlin/FocusZoneAddWrapProps

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

6c7edea

…c-react into jspurlin/FocusZoneAddWrapProps

change let to const

f6cc8b9

Add FocusZoneProps for ContextualMenu

ec4b935

fix test and add missing semicolon

040ea3f

joschect approved these changes Feb 21, 2018

View reviewed changes

dzearing approved these changes Feb 21, 2018

View reviewed changes

jspurlin added 4 commits February 21, 2018 12:28

update the max bundle size

6cc9aec

merge from master

8e6d61f

update example to include key... must be a new rule

1f1e385

bumping the bundle size that went up due to the merge from master

b7be7a2

add a missing line

633906c

dzearing closed this Feb 22, 2018

dzearing reopened this Feb 22, 2018

jspurlin and others added 3 commits February 21, 2018 21:41

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

2fbb4c9

…c-react into jspurlin/FocusZoneAddWrapProps

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

84319b9

…c-react into jspurlin/FocusZoneAddWrapProps # Conflicts: # scripts/package.json

Update FocusZone.tsx

b3e8b64

update incorrectly cased variable

2b28e22

Merge branch 'jspurlin/FocusZoneAddWrapProps' of https://github.com/j…

ff23aad

…spurlin/office-ui-fabric-react into jspurlin/FocusZoneAddWrapProps

jspurlin merged commit b055014 into microsoft:master Feb 22, 2018

jspurlin mentioned this pull request Feb 22, 2018

Focus Zone: Allow Tab to Skip Selection #4061

Merged

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

FocusZone: Add the ability for directional no wrapping #3962

FocusZone: Add the ability for directional no wrapping #3962

Uh oh!

Conversation

jspurlin commented Feb 13, 2018

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

joschect commented Feb 14, 2018

Uh oh!

jspurlin commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin commented Feb 22, 2018

Uh oh!

dzearing commented Feb 22, 2018

Uh oh!

jspurlin commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants