Skip to content

fix: GroupedList sets correct focus attributes on List #30484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smhigley wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: GroupedList sets correct focus attributes on List #30484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ce9144
fix: GroupedList sets correct focus attributes on List
smhigley Feb 6, 2024
88c88b1
add focusrects
smhigley Feb 6, 2024
14d8d72
change file
smhigley Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-973ad22b-82a2-4ee9-aa9c-42cf7b5b8ea4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: GroupedList sets correct focus attributes on List",
"packageName": "@fluentui/react",
"email": "[email protected]",
"dependentChangeType": "patch"
}
11 changes: 10 additions & 1 deletion packages/react/src/components/GroupedList/GroupedList.base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import * as React from 'react';
import { initializeComponentRef, classNamesFunction, KeyCodes, getRTLSafeKeyCode, css } from '../../Utilities';
import {
initializeComponentRef,
classNamesFunction,
FocusRects,
KeyCodes,
getRTLSafeKeyCode,
css,
} from '../../Utilities';
import { GroupedListSection } from './GroupedListSection';
import { List, ScrollToMode } from '../../List';
import { SelectionMode } from '../../Selection';
Expand Down Expand Up @@ -154,6 +161,7 @@ export class GroupedListBase extends React.Component<IGroupedListProps, IGrouped
shouldEnterInnerZone={shouldEnterInnerZone}
className={css(this._classNames.root, focusZoneProps.className)}
>
<FocusRects />
Copy link
Contributor

@spmonahan spmonahan Feb 6, 2024

Choose a reason for hiding this comment

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

Do we need to make the same change for GroupedListV2?

{!groups ? (
this._renderGroup(undefined, 0)
) : (
Expand All @@ -168,6 +176,7 @@ export class GroupedListBase extends React.Component<IGroupedListProps, IGrouped
usePageCache={usePageCache}
onShouldVirtualize={onShouldVirtualize}
version={version}
renderEarly={true}
Copy link
Contributor

@spmonahan spmonahan Feb 6, 2024

Choose a reason for hiding this comment

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

I'm a bit concerned about changing this default behavior as doing so broke TilesList (which is why it's not enabled by default now). This change will also affect DetailsList as GroupedList is used internally for rendering in that control in "grouped" mode. I know we strive to be accessible by default so I like this change in principle -- how confident are you that enabling this won't break GroupedList usage?

Copy link
Contributor Author

@smhigley smhigley Feb 6, 2024

Choose a reason for hiding this comment

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

I suppose it's impossible to really be sure, since GroupedList seems to get some pretty custom usage.

The only other potential approach I can think of would be to manually apply tabIndex={0} to the first GroupHeader, though that's equally buggy since it wouldn't help folks using custom GroupHeaders. It also seems potentially a little less reliable than letting the FocusZone take care of applying tabIndex.

Thoughts on which is better?

Copy link
Contributor

@spmonahan spmonahan Feb 6, 2024

Choose a reason for hiding this comment

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

Maybe the answer here to allow people to opt out of early rendering?

renderEarly={typeof props.renderEarly === 'boolean' ? props.renderEarly : true}

(perhaps there is a less verbose way of writing that).

The early rendering is a better option as it addresses the underlying issue of the DOM just not being present but it is also much more likely to break something using GroupedList as it changes the rendering behavior. tabIndex is better in that it is highly unlikely to break rendering.

Allowing folks to opt out will at least give people an easy change should they need it. That said, there is an argument that this is a breaking change and we should rather require users to opt in.

Not a serious suggestion: GroupedListV3: now with early rendering?

... actually that last suggestion got me thinking: is this an issue with GroupedListV2? The rendering is quite different so it may not be (though it still ultimately uses List).

{...rootListProps}
/>
)}
Expand Down