Skip to content

Commit

Permalink
Fix cell focusing logic (#1352)
Browse files Browse the repository at this point in the history 
* Remove the hack to preserve scroll position
Focus the selection mask instead of parent node

* Fix unit tests
  • Loading branch information
@amanmahajan7 @malonecj
amanmahajan7 authored and malonecj committed Oct 31, 2018
1 parent 08e500b commit 7b5ad6f
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 54 deletions.
12 changes: 0 additions & 12 deletions packages/react-data-grid/src/Canvas.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,6 @@ class Canvas extends React.PureComponent {
);
};

onFocusInteractionMask = (focus) => {
const { scrollTop, scrollLeft } = this._scroll;
const { pageXOffset, pageYOffset } = window;
focus();
if (this.canvas) {
this.canvas.scrollTop = scrollTop;
this.canvas.scrollLeft = scrollLeft;
}
window.scroll(pageXOffset, pageYOffset);
};

onScroll = (e) => {
if (this.canvas !== e.target) {
return;
Expand Down Expand Up @@ -418,7 +407,6 @@ class Canvas extends React.PureComponent {
onCellCopyPaste={this.props.onCellCopyPaste}
onGridRowsUpdated={this.props.onGridRowsUpdated}
onDragHandleDoubleClick={this.props.onDragHandleDoubleClick}
onBeforeFocus={this.onFocusInteractionMask}
onCellSelected={this.props.onCellSelected}
onCellDeSelected={this.props.onCellDeSelected}
onCellRangeSelectionStarted={this.props.onCellRangeSelectionStarted}
Expand Down
9 changes: 6 additions & 3 deletions packages/react-data-grid/src/masks/CellMask.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@ const setMaskStyle = ({ left, top, width, height, zIndex, position }) => {
zIndex,
position: position || 'absolute',
pointerEvents: 'none',
transform: `translate(${left}px, ${top}px)`
transform: `translate(${left}px, ${top}px)`,
outline: 0
};
};

const CellMask = ({ width, height, top, left, zIndex, children, position, ...rest }) => (
const CellMask = ({ width, height, top, left, zIndex, children, position, innerRef, ...rest }) => (
<div
style={setMaskStyle({ left, top, width, height, zIndex, position })}
data-test="cell-mask"
ref={innerRef}
{...rest}
>
{children}
Expand All @@ -28,7 +30,8 @@ CellMask.propTypes = {
top: PropTypes.number.isRequired,
left: PropTypes.number.isRequired,
zIndex: PropTypes.number.isRequired,
children: PropTypes.node
children: PropTypes.node,
innerRef: PropTypes.func
};

export default CellMask;
58 changes: 29 additions & 29 deletions packages/react-data-grid/src/masks/InteractionMasks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
isSelectedCellEditable,
selectedRangeIsSingleCell
} from '../utils/SelectedCellUtils';
import {isFunction} from 'common/utils';
import { isFunction } from 'common/utils';
import * as columnUtils from '../ColumnUtils';
import * as keyCodes from '../KeyCodes';
import { CellNavigationMode, EventTypes } from 'common/constants';
Expand Down Expand Up @@ -65,7 +65,6 @@ class InteractionMasks extends React.Component {
onCellRangeSelectionCompleted: PropTypes.func,
onCellsDragged: PropTypes.func,
onDragHandleDoubleClick: PropTypes.func.isRequired,
onBeforeFocus: PropTypes.func.isRequired,
scrollLeft: PropTypes.number.isRequired,
prevScrollLeft: PropTypes.number.isRequired,
scrollTop: PropTypes.number.isRequired,
Expand Down Expand Up @@ -397,16 +396,16 @@ class InteractionMasks extends React.Component {
};

isFocused = () => {
return document.activeElement === this.node;
return document.activeElement === this.selectionMask;
};

isFocusedOnBody = () => {
return document.activeElement === document.body;
};

focus = () => {
if (this.node && !this.isFocused()) {
this.props.onBeforeFocus(() => this.node.focus());
if (this.selectionMask && !this.isFocused()) {
this.selectionMask.focus();
}
};

Expand Down Expand Up @@ -579,22 +578,35 @@ class InteractionMasks extends React.Component {
this.closeEditor();
};

setSelectionMaskRef = (node) => {
this.selectionMask = node;
};

getSelectionMaskProps = () => {
const { columns, getSelectedRowHeight, getSelectedRowTop, scrollLeft, scrollTop, prevScrollLeft, prevScrollTop } = this.props;
const { prevSelectedPosition } = this.state;

return {
columns,
scrollTop,
scrollLeft,
getSelectedRowHeight,
getSelectedRowTop,
prevScrollLeft,
prevScrollTop,
prevSelectedPosition,
isGroupedRow: this.isGroupedRowSelected(),
innerRef: this.setSelectionMaskRef
};
};

getSingleCellSelectView = () => {
const { columns, getSelectedRowHeight, getSelectedRowTop, scrollLeft, scrollTop, prevScrollLeft, prevScrollTop} = this.props;
const { selectedPosition } = this.state;
return (
!this.state.isEditorEnabled && this.isGridSelected() && (
<SelectionMask
selectedPosition={selectedPosition}
columns={columns}
isGroupedRow={this.isGroupedRowSelected()}
scrollTop={scrollTop}
scrollLeft={scrollLeft}
getSelectedRowHeight={getSelectedRowHeight}
getSelectedRowTop={getSelectedRowTop}
prevScrollLeft={prevScrollLeft}
prevScrollTop={prevScrollTop}
prevSelectedPosition={this.state.prevSelectedPosition}
{...this.getSelectionMaskProps()}
>
{this.dragEnabled() && (
<DragHandle
Expand All @@ -609,7 +621,7 @@ class InteractionMasks extends React.Component {
};

getCellRangeSelectView = () => {
const { columns, rowHeight, getSelectedRowHeight, getSelectedRowTop, scrollLeft, scrollTop, prevScrollLeft, prevScrollTop } = this.props;
const { columns, rowHeight } = this.props;
return [
<SelectionRangeMask
key="range-mask"
Expand All @@ -620,15 +632,7 @@ class InteractionMasks extends React.Component {
<SelectionMask
key="selection-mask"
selectedPosition={this.state.selectedRange.startCell}
columns={columns}
rowHeight={rowHeight}
scrollLeft={scrollLeft}
scrollTop={scrollTop}
getSelectedRowHeight={getSelectedRowHeight}
getSelectedRowTop={getSelectedRowTop}
prevScrollLeft={prevScrollLeft}
prevScrollTop={prevScrollTop}
prevSelectedPosition={this.state.prevSelectedPosition}
{...this.getSelectionMaskProps()}
/>
];
};
Expand All @@ -640,10 +644,6 @@ class InteractionMasks extends React.Component {
const columns = getSelectedRowColumns(selectedPosition.rowIdx);
return (
<div
ref={node => {
this.node = node;
}}
tabIndex="0"
onKeyDown={this.onKeyDown}
onFocus={this.onFocus}
>
Expand Down
16 changes: 9 additions & 7 deletions packages/react-data-grid/src/masks/SelectionMask.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import CellMask from './CellMask';
import * as columnUtils from '../ColumnUtils';
import zIndexes from 'common/constants/zIndexes';

const isFrozenColumn = (columns, {idx}) => columnUtils.isFrozen(columnUtils.getColumn(columns, idx));
const isFrozenColumn = (columns, { idx }) => columnUtils.isFrozen(columnUtils.getColumn(columns, idx));

const isScrollingHorizontallyWithoutCellChange = ({scrollTop, prevScrollTop, scrollLeft, prevScrollLeft, selectedPosition, prevSelectedPosition}) => {
const isScrollingHorizontallyWithoutCellChange = ({ scrollTop, prevScrollTop, scrollLeft, prevScrollLeft, selectedPosition, prevSelectedPosition }) => {
return scrollLeft !== prevScrollLeft && (scrollTop === prevScrollTop) && selectedPosition.idx === prevSelectedPosition.idx;
};

Expand All @@ -19,17 +19,17 @@ const getLeftPosition = (isFrozen, cellLeft, props) => {


export const getCellMaskDimensions = (props) => {
const { selectedPosition, columns, getSelectedRowHeight, getSelectedRowTop} = props;
const { selectedPosition, columns, getSelectedRowHeight, getSelectedRowTop } = props;
const column = columnUtils.getColumn(columns, selectedPosition.idx);
const height = getSelectedRowHeight(selectedPosition.rowIdx);
const top = getSelectedRowTop(selectedPosition.rowIdx);
const frozen = isFrozenColumn(columns, selectedPosition);
const zIndex = frozen ? zIndexes.FROZEN_CELL_MASK : zIndexes.CELL_MASK;
const zIndex = frozen ? zIndexes.FROZEN_CELL_MASK : zIndexes.CELL_MASK;
const left = getLeftPosition(frozen, column.left, props);
return {height, top, width: column.width, left, zIndex};
return { height, top, width: column.width, left, zIndex };
};

function SelectionMask({children, ...rest}) {
function SelectionMask({ children, innerRef, ...rest }) {
const dimensions = getCellMaskDimensions(rest);
const frozen = isFrozenColumn(rest.columns, rest.selectedPosition);
const position = frozen && isScrollingHorizontallyWithoutCellChange(rest) ? 'fixed' : 'absolute';
Expand All @@ -38,6 +38,8 @@ function SelectionMask({children, ...rest}) {
{...dimensions}
className="rdg-selected"
position={position}
innerRef={innerRef}
tabIndex="0"
>
{children}
</CellMask>
Expand All @@ -47,7 +49,7 @@ function SelectionMask({children, ...rest}) {
SelectionMask.propTypes = {
selectedPosition: PropTypes.object.isRequired,
columns: PropTypes.array.isRequired,
rowHeight: PropTypes.number.isRequired
innerRef: PropTypes.func
};

export default SelectionMask;
8 changes: 5 additions & 3 deletions packages/react-data-grid/src/masks/__tests__/InteractionMasks.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ describe('<InteractionMasks/>', () => {
enableCellSelect: true,
cellNavigationMode: CellNavigationMode.NONE,
eventBus,
onBeforeFocus: jasmine.createSpy().and.returnValue(() => null),
getSelectedRowHeight: () => 50,
getSelectedRowTop: () => 0,
getSelectedRowColumns: jasmine.createSpy().and.callFake(() => columns),
Expand Down Expand Up @@ -191,9 +190,12 @@ describe('<InteractionMasks/>', () => {

it('should give focus to InteractionMasks once a selection has ended', () => {
// We have to use mount, rather than shallow, so that InteractionMasks has a ref to it's node, used for focusing
const { props } = setup(undefined, undefined, mount);
const { props, wrapper } = setup(undefined, undefined, mount);
props.eventBus.dispatch(EventTypes.SELECT_START, { idx: 2, rowIdx: 2 });
const { selectionMask } = wrapper.instance();
spyOn(selectionMask, 'focus');
props.eventBus.dispatch(EventTypes.SELECT_END);
expect(props.onBeforeFocus).toHaveBeenCalled();
expect(selectionMask.focus).toHaveBeenCalled();
});
});
});
Expand Down

0 comments on commit 7b5ad6f

Please sign in to comment.