From 5ed2c7361dea9f749bf8547b481cce4ba8f67749 Mon Sep 17 00:00:00 2001 From: Saad Najmi Date: Wed, 9 Nov 2022 14:28:11 -0800 Subject: [PATCH 1/4] Revert "Fix infinite loop in ContextualMenu (#2297)" This reverts commit 6f086d9beaa1aea80f475f62d8bdb80f3c855c83. --- .../components/FocusZone/macos/RCTFocusZone.m | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/components/FocusZone/macos/RCTFocusZone.m b/packages/components/FocusZone/macos/RCTFocusZone.m index 17b68bd113..97871eb0b7 100644 --- a/packages/components/FocusZone/macos/RCTFocusZone.m +++ b/packages/components/FocusZone/macos/RCTFocusZone.m @@ -418,12 +418,6 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action nextViewToFocus = [self nextValidKeyView]; while([nextViewToFocus isDescendantOf:focusZoneAncestor]) { - // there are no views left in the key view loop - if ([nextViewToFocus isEqual:focusZoneAncestor]) - { - nextViewToFocus = nil; - break; - } nextViewToFocus = [nextViewToFocus nextValidKeyView]; } } @@ -432,12 +426,6 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action nextViewToFocus = [self previousValidKeyView]; while([nextViewToFocus isDescendantOf:focusZoneAncestor]) { - // there are no views left in the key view loop - if ([nextViewToFocus isEqual:focusZoneAncestor]) - { - nextViewToFocus = nil; - break; - } nextViewToFocus = [nextViewToFocus previousValidKeyView]; } @@ -492,8 +480,7 @@ - (void)keyDown:(NSEvent *)event [[self window] makeFirstResponder:viewToFocus]; [viewToFocus scrollRectToVisible:[viewToFocus bounds]]; } - // Call super only if there are views to focus - else if (viewToFocus != nil) + else { [super keyDown:event]; } From d01aa88e97f7bdba34d5bfa1622b373a7691292b Mon Sep 17 00:00:00 2001 From: Saad Najmi Date: Wed, 9 Nov 2022 14:29:38 -0800 Subject: [PATCH 2/4] Revert "FocusZone (macOS): New prop to ignore OS keyboard shortcut preference (#2267)" This reverts commit d5c85eced1b43510215dc7aa7f79a3db80616ce7. --- .../ContextualMenu/src/ContextualMenu.tsx | 1 - .../components/ContextualMenu/src/Submenu.tsx | 1 - .../components/FocusZone/macos/RCTFocusZone.h | 1 - .../components/FocusZone/macos/RCTFocusZone.m | 142 ++++++++---------- .../FocusZone/macos/RCTFocusZoneManager.m | 2 - .../components/FocusZone/src/FocusZone.ts | 5 +- .../FocusZone/src/FocusZone.types.ts | 37 +---- 7 files changed, 69 insertions(+), 120 deletions(-) diff --git a/packages/components/ContextualMenu/src/ContextualMenu.tsx b/packages/components/ContextualMenu/src/ContextualMenu.tsx index cb9430e3e6..0c4bfad266 100644 --- a/packages/components/ContextualMenu/src/ContextualMenu.tsx +++ b/packages/components/ContextualMenu/src/ContextualMenu.tsx @@ -97,7 +97,6 @@ export const ContextualMenu = compose({ componentRef: focusZoneRef, defaultTabbableElement: focusZoneRef, focusZoneDirection: 'vertical', - forceFocusMacOS: true, }, }); diff --git a/packages/components/ContextualMenu/src/Submenu.tsx b/packages/components/ContextualMenu/src/Submenu.tsx index 54dd843de9..505eb96182 100644 --- a/packages/components/ContextualMenu/src/Submenu.tsx +++ b/packages/components/ContextualMenu/src/Submenu.tsx @@ -109,7 +109,6 @@ export const Submenu = compose({ focusZone: { componentRef: focusZoneRef, focusZoneDirection: 'vertical', - forceFocusMacOS: true, }, }); diff --git a/packages/components/FocusZone/macos/RCTFocusZone.h b/packages/components/FocusZone/macos/RCTFocusZone.h index 3bba0be2ae..fbdf0adc96 100644 --- a/packages/components/FocusZone/macos/RCTFocusZone.h +++ b/packages/components/FocusZone/macos/RCTFocusZone.h @@ -13,7 +13,6 @@ typedef NS_ENUM(NSInteger, FocusZoneDirection) { @interface RCTFocusZone : RCTView @property(nonatomic) BOOL disabled; -@property(nonatomic) BOOL forceFocus; @property(nonatomic) FocusZoneDirection focusZoneDirection; @property(nonatomic) NSString *navigateAtEnd; @property(nonatomic) NSView *defaultKeyView; diff --git a/packages/components/FocusZone/macos/RCTFocusZone.m b/packages/components/FocusZone/macos/RCTFocusZone.m index 97871eb0b7..3be648ef39 100644 --- a/packages/components/FocusZone/macos/RCTFocusZone.m +++ b/packages/components/FocusZone/macos/RCTFocusZone.m @@ -18,31 +18,53 @@ // enumerated in the same row (or column) as the current focused view static const CGFloat FocusZoneBuffer = 3; +@implementation RCTFocusZone + +static inline CGFloat GetDistanceBetweenPoints(NSPoint point1, NSPoint point2) +{ + NSPoint delta = NSMakePoint(point1.x - point2.x, point1.y - point2.y); + return sqrt(delta.x * delta.x + delta.y * delta.y); +} + +static inline CGFloat GetDistanceBetweenRects(NSRect rect1, NSRect rect2) +{ + // Get the top left corner of the rect, top right in RTL + bool isRTL = [[RCTI18nUtil sharedInstance] isRTL]; + + CGFloat rect1Offset = isRTL ? rect1.size.width : 0; + CGFloat rect2Offset = isRTL ? rect2.size.width : 0; + + NSPoint rect1Corner = NSMakePoint(rect1.origin.x + rect1Offset , rect1.origin.y); + NSPoint rect2Corner = NSMakePoint(rect2.origin.x + rect2Offset , rect2.origin.y); + + return GetDistanceBetweenPoints(rect1Corner, rect2Corner); +} -/// Returns whether FocusZone should move focus on the view in question. -/// - Parameters: -/// - view: The view to check if we should move focus to, should be a child / subview of the FocusZone in question. -/// - forceFocus: Check if we should focus on all views that accept first responder status, rather than just key views. -BOOL ShouldFocusOnView(NSView *view, BOOL focusOnAllFirstResponders) +static inline CGFloat GetMinDistanceBetweenRectVerticesAndPoint(NSRect rect, NSPoint point) { - return focusOnAllFirstResponders ? [view acceptsFirstResponder] : [view canBecomeKeyView]; + return fmin( + fmin(GetDistanceBetweenPoints(point, NSMakePoint(NSMinX(rect), NSMinY(rect))), + GetDistanceBetweenPoints(point, NSMakePoint(NSMaxX(rect), NSMaxY(rect)))), + fmin(GetDistanceBetweenPoints(point, NSMakePoint(NSMaxX(rect), NSMinY(rect))), + GetDistanceBetweenPoints(point, NSMakePoint(NSMinX(rect), NSMaxY(rect)))) + ); } /// Performs a depth first search looking for the first key view in a parent view's view hierarchy. /// This function does not take into account the geometric position of the view. -static NSView *GetFirstKeyViewWithin(NSView *parentView, BOOL forceFocus) +static NSView *GetFirstKeyViewWithin(NSView *parentView) { - if ([[parentView subviews] count] < 1) - { - return nil; - } + if ([[parentView subviews] count] < 1) + { + return nil; + } for (NSView *view in [parentView subviews]) { - if (ShouldFocusOnView(view, forceFocus)) { + if ([view canBecomeKeyView]) { return view; } - NSView *match = GetFirstKeyViewWithin(view, forceFocus); + NSView *match = GetFirstKeyViewWithin(view); if (match) { return match; } @@ -53,14 +75,14 @@ BOOL ShouldFocusOnView(NSView *view, BOOL focusOnAllFirstResponders) /// Performs a depth first search looking for the last key view in a parent view's view hierarchy. /// We find the last view by simply reversing the order of the subview array. /// This function does not take into account the geometric position of the view. -static NSView *GetLastKeyViewWithin(NSView *parentView, BOOL forceFocus) +static NSView *GetLastKeyViewWithin(NSView *parentView) { for (NSView *view in [[parentView subviews] reverseObjectEnumerator]) { - if (ShouldFocusOnView(view, forceFocus)) { + if ([view canBecomeKeyView]) { return view; } - NSView *match = GetLastKeyViewWithin(view, forceFocus); + NSView *match = GetLastKeyViewWithin(view); if (match) { return match; } @@ -68,38 +90,6 @@ BOOL ShouldFocusOnView(NSView *view, BOOL focusOnAllFirstResponders) return nil; } -static inline CGFloat GetDistanceBetweenPoints(NSPoint point1, NSPoint point2) -{ - NSPoint delta = NSMakePoint(point1.x - point2.x, point1.y - point2.y); - return sqrt(delta.x * delta.x + delta.y * delta.y); -} - -static inline CGFloat GetDistanceBetweenRects(NSRect rect1, NSRect rect2) -{ - // Get the top left corner of the rect, top right in RTL - bool isRTL = [[RCTI18nUtil sharedInstance] isRTL]; - - CGFloat rect1Offset = isRTL ? rect1.size.width : 0; - CGFloat rect2Offset = isRTL ? rect2.size.width : 0; - - NSPoint rect1Corner = NSMakePoint(rect1.origin.x + rect1Offset , rect1.origin.y); - NSPoint rect2Corner = NSMakePoint(rect2.origin.x + rect2Offset , rect2.origin.y); - - return GetDistanceBetweenPoints(rect1Corner, rect2Corner); -} - -static inline CGFloat GetMinDistanceBetweenRectVerticesAndPoint(NSRect rect, NSPoint point) -{ - return fmin( - fmin(GetDistanceBetweenPoints(point, NSMakePoint(NSMinX(rect), NSMinY(rect))), - GetDistanceBetweenPoints(point, NSMakePoint(NSMaxX(rect), NSMaxY(rect)))), - fmin(GetDistanceBetweenPoints(point, NSMakePoint(NSMaxX(rect), NSMinY(rect))), - GetDistanceBetweenPoints(point, NSMakePoint(NSMinX(rect), NSMaxY(rect)))) - ); -} - -/// Returns the first NSView in the given windows NSResponder chain. -/// - Parameter window: The window whose NSResponder chain we should check. static NSView *GetFirstResponder(NSWindow *window) { NSResponder *responder = [window firstResponder]; @@ -110,9 +100,6 @@ static inline CGFloat GetMinDistanceBetweenRectVerticesAndPoint(NSRect rect, NSP return [responder isKindOfClass:[NSView class]] ? (NSView *)responder : nil; } - -/// Returns a `FocusZoneAction` for a given NSEvent -/// - Parameter event: The event to interpret into a command. Should be a keyDown event. static FocusZoneAction GetActionForEvent(NSEvent *event) { FocusZoneAction action = FocusZoneActionNone; @@ -183,39 +170,32 @@ static inline BOOL IsHorizontalNavigationWithinZoneAction(FocusZoneAction action return focusZoneAncestor; } -@implementation RCTFocusZone - -- (NSView *)firstFocusableView +/// Bypass FocusZone if it's empty or has no focusable elements +static BOOL ShouldSkipFocusZone(NSView *view) { - return GetFirstKeyViewWithin(self, [self forceFocus]); -} + if([view isKindOfClass:[RCTFocusZone class]]) + { + NSView *keyView = GetFirstKeyViewWithin(view); + // FocusZone is empty or has no focusable elements + if (keyView == nil) + { + return YES; + } + } -- (NSView *)lastFocusableView -{ - return GetLastKeyViewWithin(self, [self forceFocus]); + return NO; } -/// Accept firstResponder on FocusZone itself in order to reassign it within the FocusZone with `becomeFirstResponder`. -/// Reject firstResponder if FocusZone is disabled or should be skipped. +/// Accept firstResponder on FocusZone itself in order to reassign it within the FocusZone. - (BOOL)acceptsFirstResponder { - BOOL rejectsResponderStatus = NO; - if (!_disabled) { - // Bypass FocusZone if it's empty or has no focusable elements - NSView *focusableView = [self firstFocusableView]; - // FocusZone is empty or has no focusable elements - if (focusableView == nil) - { - rejectsResponderStatus = YES; - } - } - return !rejectsResponderStatus; + // Reject first responder if FocusZone is disabled or should be skipped. + return !_disabled && !ShouldSkipFocusZone(self); } -/// Reassign firstResponder within the FocusZone to either the default key view or first focusable view. - (BOOL)becomeFirstResponder { - NSView *keyView = _defaultKeyView ?: [self firstFocusableView]; + NSView *keyView = _defaultKeyView ?: GetFirstKeyViewWithin(self); return !_disabled && [[self window] makeFirstResponder:keyView]; } @@ -230,9 +210,7 @@ - (NSView *)nextViewToFocusForCondition:(IsViewLeadingCandidateForNextFocus)isLe NSView *candidateView = [queue firstObject]; [queue removeObjectAtIndex:0]; - if ([candidateView isNotEqualTo:self] && - ShouldFocusOnView(candidateView, [self forceFocus]) && - isLeadingCandidate(candidateView)) + if ([candidateView isNotEqualTo:self] && [candidateView canBecomeKeyView] && isLeadingCandidate(candidateView)) { nextViewToFocus = candidateView; } @@ -303,7 +281,7 @@ - (NSView *)nextViewToFocusForAction:(FocusZoneAction)action if (!skip) { CGFloat distance = GetDistanceBetweenRects(firstResponderRect, candidateRect); - + // If there are other candidate views inside the same ScrollView as the firstResponder, // prefer those views over other views outside the scrollview, even if they are closer. if ([firstResponderEnclosingScrollView isEqualTo:[candidateView enclosingScrollView]]) @@ -378,11 +356,11 @@ - (NSView *)nextViewToFocusWithFallback:(FocusZoneAction)action { if (action == FocusZoneActionDownArrow) { - return [self firstFocusableView]; + return GetFirstKeyViewWithin(self); } else if (action == FocusZoneActionUpArrow) { - return [self lastFocusableView]; + return GetLastKeyViewWithin(self); } } @@ -409,7 +387,7 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action NSView *nextViewToFocus; [[self window] recalculateKeyViewLoop]; - + // Find the first view outside the FocusZone (or any parent FocusZones) to place focus RCTFocusZone *focusZoneAncestor = GetFocusZoneAncestor(self); @@ -428,7 +406,7 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action { nextViewToFocus = [nextViewToFocus previousValidKeyView]; } - + // If the previous view is in a FocusZone, focus on its defaultKeyView // (For FocusZoneActionTab, this is handled by becomeFirstResponder). RCTFocusZone *focusZoneAncestor = GetFocusZoneAncestor(nextViewToFocus); diff --git a/packages/components/FocusZone/macos/RCTFocusZoneManager.m b/packages/components/FocusZone/macos/RCTFocusZoneManager.m index 132d377afc..534a53e5bb 100644 --- a/packages/components/FocusZone/macos/RCTFocusZoneManager.m +++ b/packages/components/FocusZone/macos/RCTFocusZoneManager.m @@ -7,8 +7,6 @@ @implementation RCTFocusZoneManager RCT_EXPORT_MODULE() -RCT_EXPORT_VIEW_PROPERTY(forceFocus, BOOL) - RCT_EXPORT_VIEW_PROPERTY(disabled, BOOL) RCT_CUSTOM_VIEW_PROPERTY(focusZoneDirection, NSString, RCTFocusZone) diff --git a/packages/components/FocusZone/src/FocusZone.ts b/packages/components/FocusZone/src/FocusZone.ts index de0e4e5651..4adcdad09e 100644 --- a/packages/components/FocusZone/src/FocusZone.ts +++ b/packages/components/FocusZone/src/FocusZone.ts @@ -1,5 +1,5 @@ import * as React from 'react'; -import { findNodeHandle, Platform } from 'react-native'; +import { findNodeHandle } from 'react-native'; import { FocusZoneProps, FocusZoneSlotProps, FocusZoneType } from './FocusZone.types'; import { IUseStyling, composable } from '@uifabricshared/foundation-composable'; import { mergeSettings } from '@uifabricshared/foundation-settings'; @@ -12,7 +12,7 @@ const filterOutComponentRef = (propName) => propName !== 'componentRef'; export const FocusZone = composable({ usePrepareProps: (userProps: FocusZoneProps, useStyling: IUseStyling) => { - const { componentRef, defaultTabbableElement, isCircularNavigation, forceFocusMacOS, ...rest } = userProps; + const { componentRef, defaultTabbableElement, isCircularNavigation, ...rest } = userProps; const ftzRef = useViewCommandFocus(componentRef); @@ -29,7 +29,6 @@ export const FocusZone = composable({ slotProps: mergeSettings(useStyling(userProps), { root: { ...rest, - ...(Platform.OS === 'macos' && { forceFocus: forceFocusMacOS }), defaultTabbableElement: targetNativeTag, ref: ftzRef, navigateAtEnd: isCircularNavigation ? 'NavigateWrap' : 'NavigateStopAtEnds', diff --git a/packages/components/FocusZone/src/FocusZone.types.ts b/packages/components/FocusZone/src/FocusZone.types.ts index 737ad2a467..a85d44832d 100644 --- a/packages/components/FocusZone/src/FocusZone.types.ts +++ b/packages/components/FocusZone/src/FocusZone.types.ts @@ -18,55 +18,32 @@ export interface FocusZoneProps { defaultTabbableElement?: React.RefObject; /** - ** Defines which arrow keys to react to + ** Defines which arrows to react to */ focusZoneDirection?: FocusZoneDirection; /** - * Disables the FocusZone, with slightly different behavior on macOS and win32. - * On win32, the FocusZone will not be tabbable and keyboard navigation will be disabled. - * on macOS, the FocusZone will "pass through" events, and the children will respond as if there was no FocusZone. + ** If set, the FocusZone will not be tabbable and keyboard navigation will be disabled */ disabled?: boolean; - /** - * Circular Navigation prop - * @platform win32 - */ + /* Circular Navigation prop */ isCircularNavigation?: boolean; /** - * Allows for 2D navigation. This navigation strategy takes into account the position of elements - * on screen, and navigates in the direction the user selects to the nearest element. - * @platform win32 + *Allows for 2D navigation. This navigation strategy takes into account the position of elements + on screen, and navigates in the direction the user selects to the nearest element. */ use2DNavigation?: boolean; - /** - * Moves focus between all focusable views, rather than just key views. - * - * On macOS, not every focusable view is a key view (i.e: we can press Tab to move focus to it). - * Rather, there is a system preference to toggle which views are in the key view loop. - * This prop allows you to focus on all focusable views, rather than just key views. - * For more info, see https://microsoft.github.io/apple-ux-guide/KeyboardFocus.html - * @platform macOS - */ - forceFocusMacOS?: boolean; - /** * Callback called when “focus” event triggered in FocusZone */ onFocus?: (e?: any) => void; } -// Props on JS FocusZone that don't exist in the native module -interface NonNativeProps { - isCircularNavigation: FocusZoneProps['isCircularNavigation']; - forceFocusMacOS: FocusZoneProps['forceFocusMacOS']; -} -export interface NativeProps extends Exclude { - navigateAtEnd?: NavigateAtEnd; // win32 only - forceFocus?: boolean; // macOS only +export interface NativeProps extends Omit { + navigateAtEnd?: NavigateAtEnd; } export type FocusZoneDirection = From f3f067bced1cc4197e792b3f9809736fe59733c9 Mon Sep 17 00:00:00 2001 From: chiuam <67026167+chiuam@users.noreply.github.com> Date: Mon, 7 Nov 2022 15:42:51 -0500 Subject: [PATCH 3/4] Fix infinite loop in ContextualMenu (#2297) --- ...zone-2e5b4851-4168-40f4-a1db-48730ab06dd6.json | 7 +++++++ .../components/FocusZone/macos/RCTFocusZone.m | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 change/@fluentui-react-native-focus-zone-2e5b4851-4168-40f4-a1db-48730ab06dd6.json diff --git a/change/@fluentui-react-native-focus-zone-2e5b4851-4168-40f4-a1db-48730ab06dd6.json b/change/@fluentui-react-native-focus-zone-2e5b4851-4168-40f4-a1db-48730ab06dd6.json new file mode 100644 index 0000000000..7f9064e8e4 --- /dev/null +++ b/change/@fluentui-react-native-focus-zone-2e5b4851-4168-40f4-a1db-48730ab06dd6.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Fix infinite loop in ContextualMenu", + "packageName": "@fluentui-react-native/focus-zone", + "email": "email not defined", + "dependentChangeType": "patch" +} diff --git a/packages/components/FocusZone/macos/RCTFocusZone.m b/packages/components/FocusZone/macos/RCTFocusZone.m index 3be648ef39..3432f4db82 100644 --- a/packages/components/FocusZone/macos/RCTFocusZone.m +++ b/packages/components/FocusZone/macos/RCTFocusZone.m @@ -396,6 +396,12 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action nextViewToFocus = [self nextValidKeyView]; while([nextViewToFocus isDescendantOf:focusZoneAncestor]) { + // there are no views left in the key view loop + if ([nextViewToFocus isEqual:focusZoneAncestor]) + { + nextViewToFocus = nil; + break; + } nextViewToFocus = [nextViewToFocus nextValidKeyView]; } } @@ -404,6 +410,12 @@ - (NSView *)nextViewToFocusOutsideZone:(FocusZoneAction)action nextViewToFocus = [self previousValidKeyView]; while([nextViewToFocus isDescendantOf:focusZoneAncestor]) { + // there are no views left in the key view loop + if ([nextViewToFocus isEqual:focusZoneAncestor]) + { + nextViewToFocus = nil; + break; + } nextViewToFocus = [nextViewToFocus previousValidKeyView]; } @@ -458,7 +470,8 @@ - (void)keyDown:(NSEvent *)event [[self window] makeFirstResponder:viewToFocus]; [viewToFocus scrollRectToVisible:[viewToFocus bounds]]; } - else + // Call super only if there are views to focus + else if (viewToFocus != nil) { [super keyDown:event]; } From af53e2958dc182fa2cf7ffe827e9cb2d5edf98d0 Mon Sep 17 00:00:00 2001 From: Saad Najmi Date: Wed, 9 Nov 2022 15:13:49 -0800 Subject: [PATCH 4/4] Change files --- ...ntextual-menu-10dad183-5a47-4e0c-8a3a-fcbcfaa597eb.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-native-contextual-menu-10dad183-5a47-4e0c-8a3a-fcbcfaa597eb.json diff --git a/change/@fluentui-react-native-contextual-menu-10dad183-5a47-4e0c-8a3a-fcbcfaa597eb.json b/change/@fluentui-react-native-contextual-menu-10dad183-5a47-4e0c-8a3a-fcbcfaa597eb.json new file mode 100644 index 0000000000..39e886bcce --- /dev/null +++ b/change/@fluentui-react-native-contextual-menu-10dad183-5a47-4e0c-8a3a-fcbcfaa597eb.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Revert \"FocusZone (macOS): New prop to ignore OS keyboard shortcut preference (#2267)\"", + "packageName": "@fluentui-react-native/contextual-menu", + "email": "sanajmi@microsoft.com", + "dependentChangeType": "patch" +}