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.

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

Remove focus on multi-select stop #3253

Merged
merged 2 commits into from
Oct 31, 2017
Merged

Remove focus on multi-select stop #3253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf075fb
Remove focus on multi-select stop
ellatrix Oct 31, 2017
1efd3cc
WritingFlow must listen to global events
ellatrix Oct 31, 2017
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
2 changes: 2 additions & 0 deletions editor/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,14 @@ export function blockSelection( state = { start: null, end: null, focus: null, i
return {
...state,
isMultiSelecting: false,
focus: state.start === state.end ? state.focus : null,
};
case 'MULTI_SELECT':
return {
...state,
start: action.start,
end: action.end,
focus: state.isMultiSelecting ? state.focus : null,
};
case 'SELECT_BLOCK':
if ( action.uid === state.start && action.uid === state.end ) {
Expand Down
24 changes: 22 additions & 2 deletions editor/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,17 @@ describe( 'state', () => {
} );

it( 'should set multi selection', () => {
const original = deepFreeze( { focus: { editable: 'citation' }, isMultiSelecting: false } );
const state = blockSelection( original, {
type: 'MULTI_SELECT',
start: 'ribs',
end: 'chicken',
} );

expect( state ).toEqual( { start: 'ribs', end: 'chicken', focus: null, isMultiSelecting: false } );
} );

it( 'should set continuous multi selection', () => {
const original = deepFreeze( { focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
type: 'MULTI_SELECT',
Expand All @@ -761,13 +772,22 @@ describe( 'state', () => {
expect( state ).toEqual( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: true } );
} );

it( 'should end multi selection', () => {
it( 'should end multi selection with selection', () => {
const original = deepFreeze( { start: 'ribs', end: 'chicken', focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
type: 'STOP_MULTI_SELECT',
} );

expect( state ).toEqual( { start: 'ribs', end: 'chicken', focus: { editable: 'citation' }, isMultiSelecting: false } );
expect( state ).toEqual( { start: 'ribs', end: 'chicken', focus: null, isMultiSelecting: false } );
} );

it( 'should end multi selection without selection', () => {
const original = deepFreeze( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: true } );
const state = blockSelection( original, {
type: 'STOP_MULTI_SELECT',
} );

expect( state ).toEqual( { start: 'ribs', end: 'ribs', focus: { editable: 'citation' }, isMultiSelecting: false } );
} );

it( 'should not update the state if the block is already selected', () => {
Expand Down
20 changes: 11 additions & 9 deletions editor/writing-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ class WritingFlow extends Component {
this.verticalRect = null;
}

componentDidMount() {
document.addEventListener( 'keydown', this.onKeyDown );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these events to be monitored for the whole document? Or just this.container ?

Do we risk bugs with having onKeyDown now running for any keypress in the document?

Copy link
Contributor

@ephox-mogran ephox-mogran Nov 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do we risk bugs with having onKeyDown now running for any keypress in the document?

Based on testing master, Yes.

Due to this change, there are massive issues with keyboard navigation. Essentially, pressing up or down anywhere will activate this code (even in the toolbar). That is because the react events are being bound to the document as well, so this is firing even if the component handlers themselves call stopPropagation.

There is a post explaining how this works:
https://stackoverflow.com/questions/24415631/reactjs-syntheticevent-stoppropagation-only-works-with-react-events

This is forcing component developers to write event.nativeEvent.stopImmediatePropagation, otherwise their event handler AND this handler will fire. And WritingFlow is pretty much always on the screen, so this handler interferes with almost everything.

Now that we have UI testing, we need to have some basic user navigation tests setup so this kind of thing will be easier to catch in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

😱

document.addEventListener( 'mousedown', this.clearVerticalRect );
}

componentWillUnmount() {
document.removeEventListener( 'keydown', this.onKeyDown );
document.addEventListener( 'mousedown', this.clearVerticalRect );
}

bindContainer( ref ) {
this.container = ref;
}
Expand Down Expand Up @@ -156,19 +166,11 @@ class WritingFlow extends Component {
render() {
const { children } = this.props;

// Disable reason: Wrapper itself is non-interactive, but must capture
// bubbling events from children to determine focus transition intents.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ this.bindContainer }
onKeyDown={ this.onKeyDown }
onMouseDown={ this.clearVerticalRect }
>
<div ref={ this.bindContainer }>
{ children }
</div>
);
/* eslint-disable jsx-a11y/no-static-element-interactions */
}
}

Expand Down