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

Title: Unselect title by blur event (B) #2974

Closed
wants to merge 14 commits into from
Closed

Title: Unselect title by blur event (B) #2974

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ca44284
Title: Unselect title by blur event
aduth Oct 10, 2017
2974da7
Using blur and focus bubbling to unselect title
ephox-mogran Oct 11, 2017
ea8f8a1
Firing clear selected block
ephox-mogran Oct 11, 2017
8f2d430
focus textarea after copying button
ephox-mogran Oct 11, 2017
e2150d4
Merge remote-tracking branch 'origin/master' into fix/1390-title-unse…
ephox-mogran Oct 12, 2017
d5f69c2
partially working focus outside
ephox-mogran Oct 12, 2017
7f8c125
working, but forcing update ... had to create clipboard container to …
ephox-mogran Oct 12, 2017
e30a814
using focusOutside, and keeping focus inside post-title even when cop…
ephox-mogran Oct 12, 2017
5747cdc
very basic testing
ephox-mogran Oct 12, 2017
f5dd96b
cleanup
ephox-mogran Oct 12, 2017
e3179f5
JSDoc spacing
ephox-mogran Oct 12, 2017
61b7608
removing old timers
ephox-mogran Oct 12, 2017
9390fcc
lint and removing wrappedRefCallback
ephox-mogran Oct 13, 2017
6103641
unneeded clear selected block call
ephox-mogran Oct 13, 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/post-permalink/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class PostPermalink extends Component {
showCopyConfirmation: false,
} );
}, 4000 );

this.props.onLinkCopied();
}

render() {
Expand Down
50 changes: 43 additions & 7 deletions editor/post-title/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
*/
import { connect } from 'react-redux';
import Textarea from 'react-autosize-textarea';
import clickOutside from 'react-click-outside';
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { Component, findDOMNode } from '@wordpress/element';
import { keycodes } from '@wordpress/utils';

/**
Expand All @@ -35,17 +34,27 @@ class PostTitle extends Component {
this.onSelect = this.onSelect.bind( this );
this.onUnselect = this.onUnselect.bind( this );
this.onSelectionChange = this.onSelectionChange.bind( this );
this.onOuterBlur = this.onOuterBlur.bind( this );
this.onOuterFocus = this.onOuterFocus.bind( this );
this.setFocused = this.setFocused.bind( this );
this.focusText = this.focusText.bind( this );

this.state = {
isSelected: false,
hasFocusWithin: false,
};
this.blurTimer = null;
}

componentDidMount() {
document.addEventListener( 'selectionchange', this.onSelectionChange );
// eslint-disable-next-line react/no-find-dom-node
Copy link
Member

Choose a reason for hiding this comment

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

The lint rule is correct here: findDOMNode should be discouraged, and this specific behavior can be recreated with a ref. More ideally, we'd not rely on the DOM to determine how to assign focus state (the other way around being the preference, state -> DOM).

Also worth noting that this will always cause a re-render immediately after the first render, even if state is not changing (setState will always cause render, even if the hasFocusWithin is and stays false).

When would we expect this element to be the active element after a mount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm new to React so I wasn't sure if this would ever be true. I guess it would be true if the input that appeared inside it had autofocus, but maybe not exactly on mounting. I'm happy to set it to false on startup and have nothing here.

this.setFocused( findDOMNode( this ).contains( document.activeElement ) );
}

componentWillUnmount() {
document.removeEventListener( 'selectionchange', this.onSelectionChange );
clearTimeout( this.blurTimer );
}

bindTextarea( ref ) {
Expand All @@ -62,6 +71,10 @@ class PostTitle extends Component {
}
}

focusText() {
this.textareaContainer.textarea.focus();
}

onChange( event ) {
const newTitle = event.target.value.replace( REGEXP_NEWLINES, ' ' );
this.props.onUpdate( newTitle );
Expand All @@ -76,6 +89,26 @@ class PostTitle extends Component {
this.setState( { isSelected: false } );
}

setFocused( focused ) {
this.setState( { hasFocusWithin: focused } );
if ( focused ) {
this.props.clearSelectedBlock();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Ideally the title focus handler shouldn't need to deal with block selection (i.e. it should be block focus leave events clearing its selected state).

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

When I was testing it before, that wasn't working. But that was an earlier version. I'm happy to remove it and try and trust the blur of the other blocks.

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Removed this as it was not required. I think it was from an earlier version where I wasn't keeping isSelected separate.

}
}

onOuterFocus() {
clearTimeout( this.blurTimer );
this.setFocused( true );
}

onOuterBlur( e ) {
const target = e.currentTarget;
clearTimeout( this.blurTimer );
this.blurTimer = setTimeout( () => {
Copy link
Member

Choose a reason for hiding this comment

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

setTimeout is a code smell in components and can often lead to difficult-to-maintain components. It's often a signal that we don't have a full understanding of the natural flow of events and deferring is a convenient waiting mechanism. I'm wondering if there is a better way to implement this.

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I hate the setTimeout too, however I can't see a current way around it. Maybe you can. I'll explain what I think are the sequence of events

a) you focus inside the post title (fires focus event)
b) you click the copied button

  • fires a blur first on the post title (which would cause unselect)
  • then fires a focus on the copy button (based on the mousedown)
  • then fires a click on the copy button (which is what clipboard library is relying on)
  • this then creates a fake textarea and focuses it, does the copying magic and then removes it from the DOM
  • then I return focus to the textarea, because now the focused element is removed from the DOM. I think you are right, though, that we could use the returnFocus HOC.

The big problem is that blur fires before the next DOM element is focused. This is an issue with most frameworks. Some browsers might supply a relatedTarget on the blur event, but not all of them ... which has typically led to setTimeout solutions abounding like https://medium.com/@jessebeach/dealing-with-focus-and-blur-in-a-composite-widget-in-react-90d3c3b49a9b and https://gist.github.com/pstoica/4323d3e6e37e8a23dd59 and https://stackoverflow.com/questions/11592966/get-the-newly-focussed-element-if-any-from-the-onblur-event/11592974#11592974.

However, there might be a way. In our previous dealings with these issues, we've often had to resort to a setTimeout. Hopefully, you're aware of a better option.

this.setFocused( target.contains( document.activeElement ) );
}, 0 );
}

handleClickOutside() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can probably delete this now, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, clickOutside relies on this, so if we're removing it, it's likely become unused.

this.setState( { isSelected: false } );
}
Expand All @@ -88,12 +121,15 @@ class PostTitle extends Component {

render() {
const { title } = this.props;
const { isSelected } = this.state;
const className = classnames( 'editor-post-title', { 'is-selected': isSelected } );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe isSelected should be something like isTyping?

Copy link
Member

Choose a reason for hiding this comment

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

The naming had confused me as well when I first encountered it. I think isTyping would be an improvement, yes.

const { isSelected, hasFocusWithin } = this.state;
const className = classnames( 'editor-post-title', { 'is-selected': isSelected && hasFocusWithin } );

return (
<div className={ className }>
{ isSelected && <PostPermalink /> }
<div
className={ className }
onBlur={ this.onOuterBlur }
onFocus={ this.onOuterFocus }>
{ isSelected && hasFocusWithin && <PostPermalink onLinkCopied={ this.focusText } /> }
Copy link
Member

Choose a reason for hiding this comment

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

You mentioned the clipboard button library being the cause that led to this implementation to focus text again after copying. Is that something that can be (or perhaps is already) fixed at the library? Or fixed on the clipboard button? Maybe using our "withFocusReturn" higher-order component could work as a solution?

https://github.com/WordPress/gutenberg/blob/master/components/higher-order/with-focus-return/index.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that might work, but now that I think about it, doesn't this only work on componentDidUnmount? The only react controlled component is the copy button (the textarea that gets focus is created by the clipboard library itself), and I thought the copy button wasn't being removed from the DOM after clicking on it?

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There is also an option in the clipboard library of passing through trigger, and then calling clearSelection (which will focus trigger and then removes the selection), but it would be just calling focus outside of the react in the exact same way. Is that actually preferable?

<h1>
<Textarea
ref={ this.bindTextarea }
Expand Down Expand Up @@ -126,4 +162,4 @@ export default connect(
},
};
}
)( clickOutside( PostTitle ) );
)( PostTitle );