Skip to content
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.

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

**Feature:** Tree a11y #1168

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
45 changes: 37 additions & 8 deletions src/Tree/ChildTree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const ChildTree: React.SFC<Props> = ({
droppableProps,
onClick,
onContextMenu,
onDoubleClick,
onRemove,
cursor,
searchWords,
Expand All @@ -33,25 +34,52 @@ const ChildTree: React.SFC<Props> = ({
}) => {
const [isOpen, setIsOpen] = React.useState(Boolean(initiallyOpen))
const hasChildren = Boolean(childNodes && childNodes.length)
const clickCount = React.useRef(0)
const timeoutForDoubleClick = 200
TejasQ marked this conversation as resolved.
Show resolved Hide resolved

const onNodeContextMenu = React.useMemo(
() =>
!disabled && onContextMenu
? (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
event.preventDefault()
onContextMenu(event)
}
: undefined,
[disabled, onContextMenu],
)

const onNodeClick =
!disabled && (hasChildren || onClick)
? (e: React.MouseEvent<HTMLDivElement>) => {
e.stopPropagation()
if (hasChildren) {
setIsOpen(!isOpen)
}
if (onClick) {
onClick()
if (e.altKey && onNodeContextMenu) {
onNodeContextMenu(e)
return
}
clickCount.current++
setTimeout(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to cancel previous timeout before launching new one?

() => {
if (clickCount.current === 1) {
if (hasChildren) {
TejasQ marked this conversation as resolved.
Show resolved Hide resolved
setIsOpen(!isOpen)
Copy link
Contributor

Choose a reason for hiding this comment

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

This theoretically can be triggered after component unmount, which will trigger the error

}
if (onClick) {
onClick()
}
}
clickCount.current = 0
},
onDoubleClick ? timeoutForDoubleClick : 0,
)
}
: undefined

const onNodeContextMenu = React.useMemo(
const onNodeDoubleClick = React.useMemo(
() =>
!disabled && onContextMenu
!disabled && onDoubleClick
? (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
event.preventDefault()
onContextMenu(event)
onDoubleClick(event)
}
: undefined,
[disabled, onContextMenu],
Expand All @@ -64,6 +92,7 @@ const ChildTree: React.SFC<Props> = ({
searchWords={searchWords}
onNodeClick={onNodeClick}
onNodeContextMenu={onNodeContextMenu}
onNodeDoubleClick={onNodeDoubleClick}
highlight={Boolean(highlight)}
hasChildren={hasChildren}
isOpen={isOpen}
Expand Down
19 changes: 18 additions & 1 deletion src/Tree/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,22 @@
### Usage

The tree component renders a tree structure with collapsable nodes in a filetree-like design. Defined items in the tree can have a custom click and a context-click handler.

### Usage
The tree component is also keyboard accessible:

- <kbd>Tab</kbd> navigates to the next node
- <kbd>Shift</kbd>+<kbd>Tab</kbd> navigates to the previous node
- <kbd>Space</kbd> triggers the `onClick` handler, usually expanding a node
- <kbd>Enter</kbd> triggers the `onDblClick` action
- <kbd>Alt</kbd>+<kbd>Enter</kbd> triggers the right-click action
- <kbd>Delete</kbd> or <kbd>Backspace</kbd> triggers the "remove" action, if available

Try it yourself!

- "Region" has a double click/<kbd>Enter</kbd> handler
Copy link
Contributor

@mpotomin mpotomin Jul 30, 2019

Choose a reason for hiding this comment

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

Region reacts to single "Enter" keystroke with the double-click handler for me. Hence, I also can not close/open that part of the tree with my keyboard (single clicking with mouse still works as expected though)

Screenshot 2019-07-30 at 09 34 56

Copy link
Contributor Author

@TejasQ TejasQ Jul 30, 2019

Choose a reason for hiding this comment

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

Space should handle expand and collapse, while enter triggers the “action” on double click. This is outlined in the README.md. 😉

I made sure to include it. https://github.com/contiamo/operational-ui/pull/1168/files#diff-da0c8b02385f203827104fc313ccb636R10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I could reword things to make them a little clearer. Or do you have an idea to make the behavior more like what you expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have another question. The item "Legal Entity" right below the "Region" reacts to "Enter" key with opening/closing. It feels like a pretty inconsistent behaviour to me. I would expect to use Space key only everywhere for opening/closing the tree items, especially since there is no visual sing or marker, telling me, that "Region" has a handler for a double-click/enter attached. What's your take on this @TejasQ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from the click/space/enter inconsistency, the rest looks good to me, I would leave it as it is 👍

Copy link
Contributor Author

@TejasQ TejasQ Jul 30, 2019

Choose a reason for hiding this comment

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

I think we could leave that up to the users. I'm also not that convinced about the behavior. It is currently:

  • if it's foldable, space toggles, enter double-clicks
    • if no double-click specified, enter falls back to click
      • if no click specified, #yolo

I totally understand what you're talking about but I figure we'll have an issue about it from a more concrete use case in the future if its truly confusing. I think it's a little hard to say at this stage.

- "Country" has a click/<kbd>Space</kbd> handler
- "Country" has a right-click/<kbd>Alt</kbd>+<kbd>Enter</kbd> handler
- "Country" has a remove/<kbd>Backspace</kbd>/<kbd>Delete</kbd> handler

```jsx
import * as React from "react"
Expand All @@ -14,6 +30,7 @@ import { Tree, OlapIcon } from "@operational/components"
{
label: "Region",
initiallyOpen: true,
onDoubleClick: () => alert("woah you double clicked region amazing"),
childNodes: [
{
label: "City",
Expand Down
1 change: 1 addition & 0 deletions src/Tree/Tree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ interface BaseTree {
iconColor?: string
onClick?: () => void
onContextMenu?: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void
onDoubleClick?: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void
cursor?: string
onRemove?: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void
forwardRef?: (element?: HTMLElement | null) => any
Expand Down
169 changes: 110 additions & 59 deletions src/Tree/TreeItem.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,29 @@
import React from "react"
import React, { useCallback } from "react"
import NameTag from "../NameTag/NameTag"
import { darken } from "../utils"
import { darken, lighten } from "../utils"
import styled from "../utils/styled"
import { ChevronRightIcon, NoIcon, ChevronDownIcon, IconComponentType } from "../Icon/Icon"
import Highlighter from "react-highlight-words"
import constants from "../utils/constants"

interface TreeItemProps {
level: number
highlight: boolean
searchWords?: string[]
hasChildren: boolean
isOpen: boolean
label: string
tag?: string
icon?: IconComponentType
iconColor?: string
color?: string
cursor?: string
onNodeClick?: (e: React.MouseEvent<HTMLDivElement>) => void
onNodeDoubleClick?: (e: React.MouseEvent<HTMLDivElement>) => void
onNodeContextMenu?: (e: React.MouseEvent<HTMLDivElement>) => void
onRemove?: (e: React.MouseEvent<HTMLDivElement>) => void
}

const Header = styled("div")<{
highlight: boolean
onClick?: (e: React.MouseEvent<HTMLDivElement>) => void
Expand All @@ -20,18 +38,24 @@ const Header = styled("div")<{
padding: ${({ theme }) => theme.space.base}px;
border-radius: 2px;
padding-left: ${({ theme, level }) => theme.space.element * level}px;
color: ${({ theme }) => theme.color.text.dark};

:hover {
background-color: ${({ theme, highlight }) =>
highlight ? darken(theme.color.highlight, 20) : theme.color.background.lighter};
}

:focus {
outline: none;
color: ${({ theme }) => theme.color.primary};
background-color: ${({ theme }) => lighten(theme.color.primary, 50)};
}
`

const Label = styled("div")<{ hasChildren: boolean }>`
overflow-wrap: break-word;
font-size: ${({ theme }) => theme.font.size.small}px;
font-weight: ${({ theme, hasChildren }) => (hasChildren ? theme.font.weight.bold : theme.font.weight.medium)};
color: ${({ theme }) => theme.color.text.dark};
`

const DeleteNode = styled("div")`
Expand All @@ -50,23 +74,6 @@ const DeleteNode = styled("div")`
}
`

interface TreeItemProps {
level: number
highlight: boolean
searchWords?: string[]
hasChildren: boolean
isOpen: boolean
label: string
tag?: string
icon?: IconComponentType
iconColor?: string
color?: string
cursor?: string
onNodeClick?: (e: React.MouseEvent<HTMLDivElement>) => void
onNodeContextMenu?: (e: React.MouseEvent<HTMLDivElement>) => void
onRemove?: (e: React.MouseEvent<HTMLDivElement>) => void
}

const TreeItem: React.SFC<TreeItemProps> = ({
highlight,
tag,
Expand All @@ -75,51 +82,95 @@ const TreeItem: React.SFC<TreeItemProps> = ({
label,
color,
onNodeClick,
onNodeDoubleClick,
onNodeContextMenu,
onRemove,
hasChildren,
isOpen,
level,
cursor,
searchWords = [],
}) => (
<Header
level={level}
onClick={onNodeClick}
onContextMenu={onNodeContextMenu}
highlight={Boolean(highlight)}
cursor={cursor}
>
{hasChildren &&
React.createElement(isOpen ? ChevronDownIcon : ChevronRightIcon, {
size: 11,
left: true,
color: "color.text.action",
})}
{tag && (
<NameTag condensed left color={color}>
{tag}
</NameTag>
)}
{icon &&
React.createElement(icon, {
size: 12,
color: iconColor || "color.text.lighter",
style: { marginLeft: 0, marginRight: 8, flex: "0 0 15px" },
})}
<Label hasChildren={hasChildren}>
<Highlighter
textToHighlight={label}
highlightStyle={{ color: constants.color.text.action, backgroundColor: "transparent", fontWeight: "bold" }}
searchWords={searchWords}
/>
</Label>
{onRemove && (
<DeleteNode onClick={onRemove}>
<NoIcon size={12} />
</DeleteNode>
)}
</Header>
)
}) => {
const handleKeyDown = useCallback(
e => {
switch (e.key) {
case "Enter":
e.preventDefault()
if (e.altKey && onNodeContextMenu) {
onNodeContextMenu(e)
return
}

if (onNodeDoubleClick) {
onNodeDoubleClick(e)
return
}

if (onNodeClick) {
onNodeClick(e)
}
return
case " ":
case "Space": // the platform™
e.preventDefault()
if (onNodeClick) {
onNodeClick(e)
}
return
case "Delete":
case "Backspace":
e.preventDefault()
if (onRemove) {
onRemove(e)
}
return
}
},
[onNodeContextMenu, onNodeDoubleClick, onNodeClick],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[onNodeContextMenu, onNodeDoubleClick, onNodeClick],
[onNodeContextMenu, onNodeDoubleClick, onNodeClick, onRemove],

we need linter for this

)

return (
<Header
level={level}
onClick={onNodeClick}
onContextMenu={onNodeContextMenu}
onDoubleClick={onNodeDoubleClick}
onKeyDown={handleKeyDown}
highlight={Boolean(highlight)}
cursor={cursor}
tabIndex={0}
>
{hasChildren &&
React.createElement(isOpen ? ChevronDownIcon : ChevronRightIcon, {
size: 11,
left: true,
color: "color.text.action",
})}
{tag && (
<NameTag condensed left color={color}>
{tag}
</NameTag>
)}
{icon &&
React.createElement(icon, {
size: 12,
color: iconColor || "color.text.lighter",
style: { marginLeft: 0, marginRight: 8, flex: "0 0 15px" },
})}
<Label hasChildren={hasChildren}>
<Highlighter
textToHighlight={label}
highlightStyle={{ color: constants.color.text.action, backgroundColor: "transparent", fontWeight: "bold" }}
searchWords={searchWords}
/>
</Label>
{onRemove && (
<DeleteNode onClick={onRemove}>
<NoIcon size={12} />
</DeleteNode>
)}
</Header>
)
}

export default TreeItem