From 56a4c9053ee50dc04d000317d71c25be4018cce8 Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Wed, 14 Oct 2020 22:16:01 -0700 Subject: [PATCH 1/7] Add pagesVersion to List to debounce updates --- .../src/components/List/List.tsx | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/packages/office-ui-fabric-react/src/components/List/List.tsx b/packages/office-ui-fabric-react/src/components/List/List.tsx index a7405365a5d6a..c32bfcb3c3eda 100644 --- a/packages/office-ui-fabric-react/src/components/List/List.tsx +++ b/packages/office-ui-fabric-react/src/components/List/List.tsx @@ -44,6 +44,8 @@ export interface IListState { measureVersion?: number; isScrolling?: boolean; getDerivedStateFromProps(nextProps: IListProps, previousState: IListState): IListState; + + pagesVersion?: {}; } interface IPageCacheItem { @@ -342,38 +344,38 @@ export class List extends React.Component, IListState> } } - public componentDidUpdate(): void { - // Multiple updates may have been queued, so the callback will reflect all of them. - // Re-fetch the current props and states to avoid using a stale props or state captured in the closure. + public componentDidUpdate(previousProps: IListProps, previousState: IListState): void { const finalProps = this.props; const finalState = this.state; - // If we weren't provided with the page height, measure the pages - if (!finalProps.getPageHeight) { - // If measured version is invalid since we've updated the DOM - const heightsChanged = this._updatePageMeasurements(finalState.pages!); - - // On first render, we should re-measure so that we don't get a visual glitch. - if (heightsChanged) { - this._materializedRect = null; - if (!this._hasCompletedFirstRender) { - this._hasCompletedFirstRender = true; - this.setState(this._updatePages(finalProps, finalState)); + if (this.state.pagesVersion !== previousState.pagesVersion) { + // If we weren't provided with the page height, measure the pages + if (!finalProps.getPageHeight) { + // If measured version is invalid since we've updated the DOM + const heightsChanged = this._updatePageMeasurements(finalState.pages!); + + // On first render, we should re-measure so that we don't get a visual glitch. + if (heightsChanged) { + this._materializedRect = null; + if (!this._hasCompletedFirstRender) { + this._hasCompletedFirstRender = true; + this.setState(this._updatePages(finalProps, finalState)); + } else { + this._onAsyncScroll(); + } } else { - this._onAsyncScroll(); + // Enqueue an idle bump. + this._onAsyncIdle(); } } else { - // Enqueue an idle bump. + // Enqueue an idle bump this._onAsyncIdle(); } - } else { - // Enqueue an idle bump - this._onAsyncIdle(); - } - // Notify the caller that rendering the new pages has completed - if (finalProps.onPagesUpdated) { - finalProps.onPagesUpdated(finalState.pages as IPage[]); + // Notify the caller that rendering the new pages has completed + if (finalProps.onPagesUpdated) { + finalProps.onPagesUpdated(finalState.pages as IPage[]); + } } } @@ -719,6 +721,7 @@ export class List extends React.Component, IListState> return { ...previousState, ...newListState, + pagesVersion: {}, }; } From 2238f3e0e2e9d2c5175a6d3e159e54e7f0a8336a Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Tue, 18 Aug 2020 16:34:21 -0700 Subject: [PATCH 2/7] Make FocusTrapZone properly focus the first element of a FocusZone --- packages/utilities/src/focus.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/utilities/src/focus.ts b/packages/utilities/src/focus.ts index 790bf0e4daeae..a083e85510718 100644 --- a/packages/utilities/src/focus.ts +++ b/packages/utilities/src/focus.ts @@ -467,10 +467,23 @@ export function focusAsync(element: HTMLElement | { focus: () => void } | undefi if (win) { // element.focus() is a no-op if the element is no longer in the DOM, meaning this is always safe win.requestAnimationFrame(() => { - targetToFocusOnNextRepaint && targetToFocusOnNextRepaint.focus(); + const focusableElement = targetToFocusOnNextRepaint as HTMLElement | null; // We are done focusing for this frame, so reset the queued focus element targetToFocusOnNextRepaint = undefined; + + if (focusableElement) { + if (focusableElement.getAttribute && focusableElement.getAttribute(IS_FOCUSABLE_ATTRIBUTE) === 'true') { + // Normally, a FocusZone would be responsible for setting the tabindex values on all its descendants. + // However, even this animation frame callback can pre-empt the rendering of a FocusZone's child elements, + // so it may be necessary to set the tabindex directly here. + if (!focusableElement.getAttribute('tabindex')) { + focusableElement.setAttribute('tabindex', '0'); + } + } + + focusableElement.focus(); + } }); } } From f2dc693dfee0a3fdc2e15348813342441b1d79c2 Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Tue, 18 Aug 2020 18:59:19 -0700 Subject: [PATCH 3/7] Add 'firstFocusableTarget' prop to FocusTrapZone --- .../src/components/FocusTrapZone/FocusTrapZone.tsx | 9 +++++++-- .../src/components/FocusTrapZone/FocusTrapZone.types.ts | 8 ++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx index c4dd067264c3a..ed9967c0a6703 100644 --- a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx +++ b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx @@ -138,7 +138,8 @@ export class FocusTrapZone extends React.Component impl } public focus() { - const { focusPreviouslyFocusedInnerElement, firstFocusableSelector } = this.props; + // eslint-disable-next-line deprecation/deprecation + const { focusPreviouslyFocusedInnerElement, firstFocusableSelector, firstFocusableTarget } = this.props; if ( focusPreviouslyFocusedInnerElement && @@ -158,7 +159,11 @@ export class FocusTrapZone extends React.Component impl let _firstFocusableChild: HTMLElement | null = null; if (this._root.current) { - if (focusSelector) { + if (typeof firstFocusableTarget === 'string') { + _firstFocusableChild = this._root.current.querySelector(firstFocusableTarget); + } else if (firstFocusableTarget) { + _firstFocusableChild = firstFocusableTarget(this._root.current); + } else if (focusSelector) { _firstFocusableChild = this._root.current.querySelector('.' + focusSelector); } diff --git a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.types.ts b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.types.ts index d2685bb90c1fd..5378324ddd152 100644 --- a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.types.ts +++ b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.types.ts @@ -61,9 +61,17 @@ export interface IFocusTrapZoneProps extends React.HTMLAttributes string); + /** + * Either a full query selector for the first focusable element, or a function to select the focusable element + * within the area directly. + */ + firstFocusableTarget?: string | ((element: HTMLElement) => HTMLElement | null); + /** * Do not put focus onto the first element inside the focus trap zone. * @defaultvalue false From b541955362e3c262589982a9d40d2e5e80c01fa0 Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Wed, 28 Oct 2020 15:41:44 -0700 Subject: [PATCH 4/7] Add unit tests for new FocusTrapZone behaviors --- .../FocusTrapZone/FocusTrapZone.test.tsx | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx index 69d03a1c54e72..b9d52ae8d13ba 100644 --- a/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx +++ b/packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.test.tsx @@ -616,6 +616,54 @@ describe('FocusTrapZone', () => { expect(document.activeElement).toBe(buttonA); }); + + it('Focuses on firstFocusableTarget selector on mount', async () => { + expect.assertions(1); + + const { buttonC } = setupTest({ firstFocusableTarget: '.c' }); + + expect(document.activeElement).toBe(buttonC); + }); + + it('Focuses on firstFocusableTarget callback on mount', async () => { + expect.assertions(1); + + const { buttonC } = setupTest({ firstFocusableTarget: (element: HTMLElement) => element.querySelector('.c') }); + + expect(document.activeElement).toBe(buttonC); + }); + + it('Does not focus on firstFocusableTarget selector on mount while disabled', async () => { + expect.assertions(1); + + const activeElement = document.activeElement; + + setupTest({ firstFocusableTarget: '.c', disabled: true }); + + expect(document.activeElement).toBe(activeElement); + }); + + it('Does not focus on firstFocusableTarget callback on mount while disabled', async () => { + expect.assertions(1); + + const activeElement = document.activeElement; + + setupTest({ firstFocusableTarget: (element: HTMLElement) => element.querySelector('.c'), disabled: true }); + + expect(document.activeElement).toBe(activeElement); + }); + + it('Falls back to first focusable element with invalid firstFocusableTarget selector', async () => { + const { buttonA } = setupTest({ firstFocusableTarget: '.invalidSelector' }); + + expect(document.activeElement).toBe(buttonA); + }); + + it('Falls back to first focusable element with invalid firstFocusableTarget callback', async () => { + const { buttonA } = setupTest({ firstFocusableTarget: () => null }); + + expect(document.activeElement).toBe(buttonA); + }); }); describe('Focusing the FTZ', () => { From f7cc5dd5ccc2d6fc84374bc30dec521ad4227e42 Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Wed, 28 Oct 2020 15:42:51 -0700 Subject: [PATCH 5/7] Change files --- ...-utilities-2020-09-18-14-27-02-details-list-focus.json | 8 ++++++++ ...bric-react-2020-09-18-14-27-02-details-list-focus.json | 8 ++++++++ 2 files changed, 16 insertions(+) create mode 100644 change/@uifabric-utilities-2020-09-18-14-27-02-details-list-focus.json create mode 100644 change/office-ui-fabric-react-2020-09-18-14-27-02-details-list-focus.json diff --git a/change/@uifabric-utilities-2020-09-18-14-27-02-details-list-focus.json b/change/@uifabric-utilities-2020-09-18-14-27-02-details-list-focus.json new file mode 100644 index 0000000000000..98b2cb15722ec --- /dev/null +++ b/change/@uifabric-utilities-2020-09-18-14-27-02-details-list-focus.json @@ -0,0 +1,8 @@ +{ + "type": "patch", + "comment": "Make FocusTrapZone properly focus the first element of a FocusZone", + "packageName": "@uifabric/utilities", + "email": "tmichon@microsoft.com", + "dependentChangeType": "patch", + "date": "2020-09-18T21:26:51.071Z" +} diff --git a/change/office-ui-fabric-react-2020-09-18-14-27-02-details-list-focus.json b/change/office-ui-fabric-react-2020-09-18-14-27-02-details-list-focus.json new file mode 100644 index 0000000000000..92e8ac1a6b058 --- /dev/null +++ b/change/office-ui-fabric-react-2020-09-18-14-27-02-details-list-focus.json @@ -0,0 +1,8 @@ +{ + "type": "minor", + "comment": "Add 'firstFocusableTarget' prop to FocusTrapZone", + "packageName": "office-ui-fabric-react", + "email": "tmichon@microsoft.com", + "dependentChangeType": "patch", + "date": "2020-09-18T21:26:30.843Z" +} From 6b980204e49396506a6e315955c0b80d4492f8aa Mon Sep 17 00:00:00 2001 From: Thomas Michon Date: Wed, 28 Oct 2020 15:43:08 -0700 Subject: [PATCH 6/7] Update API signatures --- .../office-ui-fabric-react/etc/office-ui-fabric-react.api.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md index f6de1a31288d7..21ba92d4e8a02 100644 --- a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md +++ b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md @@ -4880,7 +4880,9 @@ export interface IFocusTrapZoneProps extends React.HTMLAttributes string); + firstFocusableTarget?: string | ((element: HTMLElement) => HTMLElement | null); focusPreviouslyFocusedInnerElement?: boolean; forceFocusInsideTrap?: boolean; ignoreExternalFocusing?: boolean; From 78d0f872242688acb657f974970943ff8c8bd596 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Wed, 28 Oct 2020 18:03:50 -0700 Subject: [PATCH 7/7] update API --- .../office-ui-fabric-react/etc/office-ui-fabric-react.api.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md index 21ba92d4e8a02..d8f8e977b4676 100644 --- a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md +++ b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md @@ -5698,6 +5698,8 @@ export interface IListState { measureVersion?: number; // (undocumented) pages?: IPage[]; + // (undocumented) + pagesVersion?: {}; } // @public (undocumented) @@ -8572,7 +8574,7 @@ export class List extends React.Component, IListState> // (undocumented) componentDidMount(): void; // (undocumented) - componentDidUpdate(): void; + componentDidUpdate(previousProps: IListProps, previousState: IListState): void; // (undocumented) componentWillUnmount(): void; // (undocumented)