Skip to content

Commit

Permalink
fix(form-builder): move the handling of ensuring parent item visibili…
Browse files Browse the repository at this point in the history
…ty to the inputs that needs it. (#2329)

When a document node value is being edited in a dialog we want to make sure the input/preview of the parent/enclosing is scrolled into view in the background. Originally we solved this by responding to changes in the first element of the focus path (e.g. the top most document field name), and whenever this changed, we would scroll to the dom element with this as the value for `data-focus-path`. As a consequence, opening an item for edit would scroll the editor to the path of the _parent input_ into view, and this would sometimes trigger a scroll jump actually scrolling the field out of view because scrollIntoView would ensure the top edge of the parent input was in view. Instead, what we really want to ensure here is that the edited _item_ is in the view.

This fixes the issue by moving the logic for keeping the item in view to the individual input components that needs this behavior. For now, it is only the ArrayOfObjectsInput and the PortableTextInput that requires this. Technically, ImageInput and FileInput also has this behavior, but these don't support deep linking, so it's less of a pressing issue here.

As a bonus, this PR also adds a subtle focus ring on the background element that is currently being edited.
  • Loading branch information
bjoerge committed Feb 23, 2021
1 parent 4f85843 commit 8f946da
Show file tree
Hide file tree
Showing 15 changed files with 149 additions and 42 deletions.
1 change: 0 additions & 1 deletion packages/@sanity/desk-tool/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
"react-popper": "^2.2.4",
"react-tiny-virtual-list": "^2.0.5",
"rxjs": "^6.5.3",
"scroll-into-view-if-needed": "^2.2.26",
"shallow-equals": "^1.0.0",
"styled-components": "^5.2.1"
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import scrollIntoView from 'scroll-into-view-if-needed'

import {useDocumentPresence} from '@sanity/base/hooks'
import {FormBuilder} from 'part:@sanity/form-builder'
import documentStore from 'part:@sanity/base/datastore/document'
Expand Down Expand Up @@ -49,14 +46,6 @@ export const EditForm = memo((props: Props) => {
type,
} = props

const startSegment = focusPath[0]
useEffect(() => {
const el = document.querySelector(`[data-focus-path="${startSegment}"]`)
if (el) {
scrollIntoView(el, {scrollMode: 'if-needed', block: 'nearest'})
}
}, [startSegment])

useEffect(() => {
subscriptionRef.current = documentStore.pair
.documentEvents(props.id, props.type.name)
Expand Down
1 change: 1 addition & 0 deletions packages/@sanity/form-builder/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"react-datepicker": "^1.8.0",
"react-measure": "^2.3.0",
"rxjs": "^6.5.3",
"scroll-into-view-if-needed": "^2.2.26",
"shallow-equals": "^1.0.0",
"speakingurl": "^13.0.0",
"styled-components": "^5.2.1"
Expand Down
32 changes: 32 additions & 0 deletions packages/@sanity/form-builder/src/hooks/useDidUpdate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* eslint-disable no-shadow */
// eslint is giving false positives no-shadow for typed arguments here

import {useEffect} from 'react'
import {usePrevious} from './usePrevious'

/**
* A hook for doing side effects as a response to a change in a hook value between renders
* Usage:
* ```js
* useDidUpdate(hasFocus, (hadFocus, hasFocus) => {
* if (hasFocus) {
* scrollIntoView(elementRef.current)
* }
* })
* ```
* @param current The value you want to respond to changes in
* @param didUpdate Callback to run when the value changes
*/
export function useDidUpdate<T>(current: T, didUpdate: (previous: T, current: T) => void): void
export function useDidUpdate<T>(
current: T,
didUpdate: (previous: T, current: T | undefined) => void
): void {
const previous = usePrevious<T>(current)

useEffect(() => {
if (previous !== current) {
didUpdate(previous, current)
}
}, [didUpdate, current, previous])
}
14 changes: 14 additions & 0 deletions packages/@sanity/form-builder/src/hooks/usePrevious.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {useEffect, useRef} from 'react'

/**
* A hook that returns the previous value of a component variable
* This might be provided by React in the future (https://reactjs.org/docs/hooks-faq.html#how-to-get-the-previous-props-or-state)
* @param value The value to track. Will return undefined for first render
*/
export function usePrevious<T>(value: T): T {
const ref = useRef<T>()
useEffect(() => {
ref.current = value
}, [value])
return ref.current
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {useCallback} from 'react'
import scrollIntoView from 'scroll-into-view-if-needed'
import {useDidUpdate} from './useDidUpdate'

const SCROLL_OPTIONS = {scrollMode: 'if-needed'} as const

/**
* A hook to help make sure the parent element of a value edited in a dialog (or "out of band") stays
visible in the background
* @param elementRef The element to scroll into view when the proivided focusWithin changes from true to false
* @param hasFocusWithin A boolean indicating whether we have has focus within the currently edited value
*/
export function useScrollIntoViewOnFocusWithin(
elementRef: {current?: HTMLElement},
hasFocusWithin: boolean
): void {
return useDidUpdate(
hasFocusWithin,
useCallback(
(hadFocus) => {
if (!hadFocus) {
scrollIntoView(elementRef.current, SCROLL_OPTIONS)
}
},
[elementRef]
)
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@
background: var(--component-bg);
border-radius: var(--border-radius-small);
padding: calc(var(--extra-small-padding) - 1px);
transition: box-shadow 100ms;
transition: box-shadow 250ms;

@nest :global(.ArrayInput__moving) & {
box-shadow: 0 0 0 1px var(--hairline-color), 0 8px 17px 2px var(--shadow-color-umbra),
0 3px 14px 2px var(--shadow-color-penumbra), 0 5px 5px -3px var(--shadow-color-ambient);
}
@nest [aria-selected='true'] & {
box-shadow: 0 0 0 1px var(--selected-item-color);
}
}

.innerWithError {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ import {resolveTypeName} from '../../../utils/resolveTypeName'
import ConfirmButton from '../ConfirmButton'
import {ItemValue} from '../typedefs'
import InvalidItem from '../InvalidItem'
import {hasFocusInPath, isEmpty, pathSegmentFrom} from './helpers'

import {hasFocusAtPath, hasFocusWithinPath} from '../../../utils/focusUtils'
import {EMPTY_ARRAY} from '../../../utils/empty'
import {isEmpty} from './helpers'
import styles from './ArrayInputGridItem.css'

interface ArrayInputGridItemProps {
Expand Down Expand Up @@ -60,14 +61,14 @@ export class ArrayInputGridItem extends React.PureComponent<ArrayInputGridItemPr
componentDidMount() {
const {focusPath, value} = this.props

if (value._key && hasFocusInPath(focusPath, value)) {
if (value._key && hasFocusAtPath(focusPath, value)) {
this.focus()
}
}

componentDidUpdate(prevProps: ArrayInputGridItemProps) {
const hadFocus = hasFocusInPath(prevProps.focusPath, prevProps.value)
const hasFocus = hasFocusInPath(this.props.focusPath, this.props.value)
const hadFocus = hasFocusAtPath(prevProps.focusPath, prevProps.value)
const hasFocus = hasFocusAtPath(this.props.focusPath, this.props.value)

if (!hadFocus && hasFocus) {
this.focus()
Expand Down Expand Up @@ -241,7 +242,7 @@ export class ArrayInputGridItem extends React.PureComponent<ArrayInputGridItemPr
})
})
.filter(Boolean)
const hasItemFocus = PathUtils.isExpanded(pathSegmentFrom(value), focusPath)
const isEditingItem = hasFocusWithinPath(focusPath, value)
const memberType = this.getMemberType()

if (!memberType) {
Expand All @@ -250,7 +251,7 @@ export class ArrayInputGridItem extends React.PureComponent<ArrayInputGridItemPr

return (
<ChangeIndicatorScope path={[value._key ? {_key: value._key} : index]}>
<ContextProvidedChangeIndicator compareDeep disabled={hasItemFocus}>
<ContextProvidedChangeIndicator compareDeep disabled={isEditingItem}>
<div className={styles.inner}>
<div
tabIndex={0}
Expand All @@ -270,7 +271,7 @@ export class ArrayInputGridItem extends React.PureComponent<ArrayInputGridItemPr

{!readOnly && (
<div className={styles.presenceContainer}>
<FieldPresence presence={hasItemFocus ? [] : presence} maxAvatars={1} />
<FieldPresence presence={isEditingItem ? EMPTY_ARRAY : presence} maxAvatars={1} />
</div>
)}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {ArraySchemaType, Marker, Path} from '@sanity/types'
import React from 'react'
import PatchEvent from '../../../PatchEvent'
import {ItemValue} from '../typedefs'
import {useScrollIntoViewOnFocusWithin} from '../../../hooks/useScrollIntoViewOnFocusWithin'
import {hasFocusWithinPath} from '../../../utils/focusUtils'
import {ArrayInputGridItem} from './ArrayInputGridItem'
import {ArrayInputListItem} from './ArrayInputListItem'

Expand All @@ -25,11 +27,23 @@ interface ArrayInputItemProps {
}

export function ArrayInputItem(props: ArrayInputItemProps) {
const elementRef = React.useRef<HTMLDivElement>()
const hasFocusWithin = hasFocusWithinPath(props.focusPath, props.value)
useScrollIntoViewOnFocusWithin(elementRef, hasFocusWithin)

const options = props.type.options || {}

if (options.layout === 'grid') {
return <ArrayInputGridItem {...props} />
return (
<div ref={elementRef} aria-selected={hasFocusWithin}>
<ArrayInputGridItem {...props} />
</div>
)
}

return <ArrayInputListItem {...props} />
return (
<div ref={elementRef} aria-selected={hasFocusWithin}>
<ArrayInputListItem {...props} />
</div>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@
background: var(--component-bg);
border-radius: var(--border-radius-small);
padding: calc(var(--extra-small-padding) - 1px);
transition: box-shadow 100ms;
transition: box-shadow 250ms;

@nest :global(.ArrayInput__moving) & {
box-shadow: 0 0 0 1px var(--hairline-color), 0 8px 17px 2px var(--shadow-color-umbra),
0 3px 14px 2px var(--shadow-color-penumbra), 0 5px 5px -3px var(--shadow-color-ambient);
}

@nest [aria-selected='true'] & {
box-shadow: 0 0 0 1px var(--selected-item-color);
}
}

.innerWithError {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import {resolveTypeName} from '../../../utils/resolveTypeName'
import ConfirmButton from '../ConfirmButton'
import {ItemValue} from '../typedefs'
import InvalidItem from '../InvalidItem'
import {hasFocusInPath, isEmpty, pathSegmentFrom} from './helpers'
import {hasFocusAtPath, hasFocusWithinPath} from '../../../utils/focusUtils'
import {EMPTY_ARRAY} from '../../../utils/empty'
import {isEmpty} from './helpers'

import styles from './ArrayInputListItem.css'

Expand Down Expand Up @@ -62,14 +64,14 @@ export class ArrayInputListItem extends React.PureComponent<ArrayInputListItemPr
componentDidMount() {
const {focusPath, value} = this.props

if (value._key && hasFocusInPath(focusPath, value)) {
if (value._key && hasFocusAtPath(focusPath, value)) {
this.focus()
}
}

componentDidUpdate(prevProps: ArrayInputListItemProps) {
const hadFocus = hasFocusInPath(prevProps.focusPath, prevProps.value)
const hasFocus = hasFocusInPath(this.props.focusPath, this.props.value)
const hadFocus = hasFocusAtPath(prevProps.focusPath, prevProps.value)
const hasFocus = hasFocusAtPath(this.props.focusPath, this.props.value)

if (!hadFocus && hasFocus) {
this.focus()
Expand Down Expand Up @@ -244,7 +246,7 @@ export class ArrayInputListItem extends React.PureComponent<ArrayInputListItemPr
return {...marker, item: marker.item.cloneWithMessage(`Contains ${level}`)}
})

const hasItemFocus = PathUtils.isExpanded(pathSegmentFrom(value), focusPath)
const isEditingItem = hasFocusWithinPath(focusPath, value)
const memberType = this.getMemberType()

if (!memberType) {
Expand All @@ -253,7 +255,7 @@ export class ArrayInputListItem extends React.PureComponent<ArrayInputListItemPr

return (
<ChangeIndicatorScope path={[value._key ? {_key: value._key} : index]}>
<ContextProvidedChangeIndicator compareDeep disabled={hasItemFocus}>
<ContextProvidedChangeIndicator compareDeep disabled={isEditingItem}>
<div className={styles.inner} ref={this.setInnerElement}>
{isSortable && <DragHandle />}

Expand All @@ -277,7 +279,7 @@ export class ArrayInputListItem extends React.PureComponent<ArrayInputListItemPr
<div className={styles.functions}>
{!readOnly && (
<div className={styles.presenceContainer}>
<FieldPresence presence={hasItemFocus ? [] : presence} maxAvatars={1} />
<FieldPresence presence={isEditingItem ? EMPTY_ARRAY : presence} maxAvatars={1} />
</div>
)}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
import * as PathUtils from '@sanity/util/paths'
import {IGNORE_KEYS} from './constants'

export function pathSegmentFrom(value) {
return {_key: value._key}
}

export function hasFocusInPath(path, value) {
return path.length === 1 && PathUtils.isSegmentEqual(path[0], pathSegmentFrom(value))
}

export function isEmpty(value) {
return Object.keys(value).every((key) => IGNORE_KEYS.includes(key))
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {useZIndex} from '@sanity/base/components'
import {ChangeIndicatorWithProvidedFullPath} from '@sanity/base/lib/change-indicators'
import ActivateOnFocus from 'part:@sanity/components/utilities/activate-on-focus'
import PatchEvent from '../../PatchEvent'
import {EMPTY_ARRAY} from '../../utils/empty'
import styles from './PortableTextInput.css'
import {BlockObject} from './Objects/BlockObject'
import {InlineObject} from './Objects/InlineObject'
Expand Down Expand Up @@ -246,6 +247,7 @@ export default function PortableTextInput(props: Props) {
editor={editor}
markers={blockMarkers}
onChange={handleFormBuilderEditObjectChange}
focusPath={focusPath || EMPTY_ARRAY}
onFocus={onFocus}
readOnly={readOnly}
type={blockType}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable react/prop-types */
import React, {FunctionComponent, SyntheticEvent, useMemo} from 'react'
import React, {FunctionComponent, SyntheticEvent, useMemo, useRef} from 'react'
import classNames from 'classnames'
import {Path, Marker, isValidationErrorMarker} from '@sanity/types'
import {
Expand All @@ -11,6 +11,9 @@ import {
import {FOCUS_TERMINATOR} from '@sanity/util/paths'

import {PatchEvent} from '../../../PatchEvent'

import {useScrollIntoViewOnFocusWithin} from '../../../hooks/useScrollIntoViewOnFocusWithin'
import {hasFocusWithinPath} from '../../../utils/focusUtils'
import {BlockObjectPreview} from './BlockObjectPreview'
import styles from './BlockObject.css'

Expand All @@ -20,6 +23,7 @@ type Props = {
markers: Marker[]
onChange: (patchEvent: PatchEvent, path: Path) => void
onFocus: (path: Path) => void
focusPath: Path
readOnly: boolean
type: Type
value: PortableTextBlock
Expand All @@ -29,11 +33,16 @@ export const BlockObject: FunctionComponent<Props> = ({
attributes: {focused, selected, path},
editor,
markers,
focusPath,
onFocus,
readOnly,
type,
value,
}): JSX.Element => {
const elementRef = useRef()

useScrollIntoViewOnFocusWithin(elementRef, hasFocusWithinPath(focusPath, value))

const errors = markers.filter(isValidationErrorMarker)
const classnames = classNames([
styles.root,
Expand Down Expand Up @@ -78,7 +87,7 @@ export const BlockObject: FunctionComponent<Props> = ({
)
}, [value, readOnly])
return (
<div className={classnames} onDoubleClick={handleClickToOpen}>
<div className={classnames} ref={elementRef} onDoubleClick={handleClickToOpen}>
<div className={styles.previewContainer} style={readOnly ? {cursor: 'default'} : {}}>
{blockPreview}
</div>
Expand Down
18 changes: 18 additions & 0 deletions packages/@sanity/form-builder/src/utils/focusUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {isKeyedObject, isKeySegment, Path, PathSegment} from '@sanity/types'

// Tests whether a keyed value matches a given keyed pathSegment
function matchesSegment(segment: PathSegment, value: unknown) {
return isKeyedObject(value) && isKeySegment(segment) && value._key === segment._key
}

// Utility to check if the given focusPath terminates at the given keyed value
// E.g. focus is on the value itself and not a child node
export function hasFocusAtPath(path: Path, value: unknown): boolean {
return path.length === 1 && matchesSegment(path[0], value)
}

// Utility to check if the given focusPath terminates at a child node of the given keyed value
// E.g. focus is on a child node of the value and not the value itself
export function hasFocusWithinPath(path: Path, value: unknown): boolean {
return path.length > 1 && matchesSegment(path[0], value)
}

0 comments on commit 8f946da

Please sign in to comment.