Nav Accessibility: Expose focus public method so that caller can explicitly set focus to Nav component. #10310
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dzearing
reviewed
Aug 29, 2019
| @@ -33,16 +34,29 @@ export class NavBase extends React.Component<INavProps, INavState> implements IN | |||
| groups: null | |||
| }; | |||
|
|
|||
| private _focusZone: IFocusZone | null; | |||
dzearing
reviewed
Aug 29, 2019
| @@ -55,7 +69,8 @@ export class NavBase extends React.Component<INavProps, INavState> implements IN | |||
| const classNames = getClassNames(styles!, { theme: theme!, className, isOnTop, groups }); | |||
|
|
|||
| return ( | |||
| <FocusZone direction={FocusZoneDirection.vertical}> | |||
| // tslint:disable-next-line: jsx-no-lambda | |||
| <FocusZone direction={FocusZoneDirection.vertical} componentRef={focusZone => (this._focusZone = focusZone)}> | |||
There was a problem hiding this comment.
no need to pass in a function; just use createRef.
dzearing
reviewed
Aug 29, 2019
| @@ -67,6 +82,13 @@ export class NavBase extends React.Component<INavProps, INavState> implements IN | |||
| return this.state.selectedKey; | |||
| } | |||
|
|
|||
| private _setNavWithFocus() { | |||
There was a problem hiding this comment.
I don't think this is the right thing to do and makes me really confused.
If you want to expose a focus method on Nav, and direct it to the focuszone.focus, that would make sense.
As this is I would not have expected this behavior as a user.
There was a problem hiding this comment.
it is not the idea solution for sure and Ping and I discussed and found if isOnTop already there we might be able to use it so there will be no change in SP pages since it always set isOnTop for this condition.
We tried to do it in wrapper comp but simply Nav is not focus-able. We will explore to see if expose focuse method on Nav object perhaps be better fit?
|
Also in the future please title your PRs with meaningful names, it helps identify it at a glance. :) |
dzearing
changed the title
dzearing
requested changes
Aug 29, 2019
There was a problem hiding this comment.
I really don't believe this is the right fix. No component should force focus onto an element unless the prop is named something like "autoFocus" or "setInitialFocus".
A better approach would be to expose a focus imperative method which you can call at the right time.
inateeg
changed the title
|
@david Zearing<mailto:dzearing@microsoft.com> you are right the clean & right fix is for caller to explicitly calling navComponent.focus(forceFirstElementFocus)… I have refactor the PR so that Nav component will expose focus public api. Please take the look. Only thing is deadline for accessibility fix is this Friday. This PR will need to pick up to V6 since sp-client still on V6 for a while and then it require sp-pages code change to get Nav componentRef and call focus explicitly. Hmm even we do 60 meter dash speed, perhaps all the PR in will pass Friday deadline that is tomorrow ☹
From: David Zearing <notifications@github.com>
Sent: Wednesday, August 28, 2019 9:12 PM
To: OfficeDev/office-ui-fabric-react <office-ui-fabric-react@noreply.github.com>
Cc: Ina Teegan <inateeg@microsoft.com>; Author <author@noreply.github.com>
Subject: Re: [OfficeDev/office-ui-fabric-react] Inateeg/0828nav new (#10310)
@dzearing commented on this pull request.
________________________________
In packages/office-ui-fabric-react/src/components/Nav/Nav.base.tsx<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FOfficeDev%2Foffice-ui-fabric-react%2Fpull%2F10310%23discussion_r318878842&data=02%7C01%7Cinateeg%40microsoft.com%7Cdd357123b9e044f8cc2c08d72c3717a4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637026487496466382&sdata=IJMh6eBlXW28BJtGQne1BEFifPrbrTekSPJDiAZK1J0%3D&reserved=0>:
@@ -67,6 +82,13 @@ export class NavBase extends React.Component<INavProps, INavState> implements IN
return this.state.selectedKey;
}
+ private _setNavWithFocus() {
I don't think this is the right thing to do and makes me really confused.
If you want to expose a focus method on Nav, and direct it to the focuszone.focus, that would make sense.
As this is I would not have expected this behavior as a user.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FOfficeDev%2Foffice-ui-fabric-react%2Fpull%2F10310%3Femail_source%3Dnotifications%26email_token%3DAE6WWLTCUHIJPK5JE54AUW3QG5ECNA5CNFSM4IRZQCSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDBO6GY%23pullrequestreview-281210651&data=02%7C01%7Cinateeg%40microsoft.com%7Cdd357123b9e044f8cc2c08d72c3717a4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637026487496476377&sdata=TZy9P6%2BZrROTi2U5EJWG2LestprdtAz619qiDeLh6kA%3D&reserved=0>, or mute the thread<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAE6WWLSZNSNDEOU3O57S4ELQG5ECNANCNFSM4IRZQCSA&data=02%7C01%7Cinateeg%40microsoft.com%7Cdd357123b9e044f8cc2c08d72c3717a4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637026487496476377&sdata=oQylcCAN4cmFUNgRh4GQZY3IAjSIfClPJV14n8XitOs%3D&reserved=0>.
|
inateeg
changed the title
Asset size changes
Baseline commit: 17e5e3ab7c6b049cc1e1bd4a7126f9f3988e8f4c (build) |
Component Perf AnalysisNo significant results to display. All results
|
pingj
approved these changes
Aug 29, 2019
dzearing
reviewed
Aug 30, 2019
packages/office-ui-fabric-react/src/components/Nav/examples/Nav.Basic.Example.tsx
Outdated
Show resolved
Hide resolved
dzearing
approved these changes
Aug 30, 2019
|
🎉 Handy links: |
ecraig12345
pushed a commit
that referenced
this pull request
Feb 4, 2020
cliffkoh
pushed a commit
that referenced
this pull request
Feb 7, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeWhen Nav component is rendered, there is property 'isOnTop' can by passed in when the Nav is rendered as popup. IN SPO case, this is when in T-narrow mode, Nav is only rendered when user click on Hamburg. Under this condition, we would like Nav get focus since it is intentionally popup opened by user action. This PR ensure when those conditions meet, Nav focusZone get focus so user can use arrow key to navigate through nodes.
(give an overview)
Focus areas to test
set isOnTop to true and verify the arrow key can navigate through the nav nodes while without this property set, that is not possible.
Microsoft Reviewers: Open in CodeFlow