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

ephox-mogran · 2017-10-11T05:46:50Z

This is building on #2948

Description

Add some blur and focus handlers to the outer div of the title post so that we can detect focus has transferred outside of the container. Built on the previous PR by allowing the Copy Permalink button to be clicked.

How Has This Been Tested?

I have manually tested it but did not know how to write unit tests for it.

Types of changes

This change is a local change to the post-title component, as well as a small change to Permalink. It requires an additional prop for Permalinks, but they are only used in Titles.

Checklist:

My code is tested (manually)
My code follows the WordPress code style.
My code follows has proper inline documentation (there was no documentation in the file)

codecov · 2017-10-11T05:46:53Z

Codecov Report

Merging #2974 into master will increase coverage by 0.93%.
The diff coverage is 25.92%.

@@            Coverage Diff             @@
##           master    #2974      +/-   ##
==========================================
+ Coverage   34.42%   35.35%   +0.93%     
==========================================
  Files         196      197       +1     
  Lines        5796     6225     +429     
  Branches     1021     1122     +101     
==========================================
+ Hits         1995     2201     +206     
- Misses       3217     3389     +172     
- Partials      584      635      +51

Impacted Files	Coverage Δ
editor/post-title/index.js	`0% <0%> (ø)`	⬆️
editor/post-permalink/index.js	`0% <0%> (ø)`	⬆️
...omponents/higher-order/with-focus-outside/index.js	`50% <50%> (ø)`
editor/selectors.js	`95.25% <0%> (-1.66%)`	⬇️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/block-mover/index.js	`0% <0%> (ø)`	⬆️
blocks/api/registration.js	`100% <0%> (ø)`	⬆️
editor/block-settings-menu/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block-list.js	`0% <0%> (ø)`	⬆️
editor/sidebar/header.js	`0% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d715d61...6103641. Read the comment docs.

ephox-mogran · 2017-10-11T05:51:44Z

Summary of changes

added blur and focus handlers to the outer div, so that focusing something inside (like the button) does not fire a blur event
focused the textarea after clicking copy. On of the issue with the clipboard button is that it uses a library which creates a textarea and focuses it to copy to the clipboard. That textarea is then removed while it is the active element, which makes the browser fallback to making body the active element. This fired a blur and made the unselect fire. By focusing the title textarea after copying, we avoid this.
added the state: hasFocusWithin which must also be true (as well as isSelected which seems to revolve around also detecting typing) for the title 'block' decorations to appear.

I also spent a bit of time trying to make this a higher order component, or a separate component that just exposed the final blur and focus handlers, but because of the need to combine isSelected and hasFocusedWithin, I wasn't able to generalise it.

ephox-mogran · 2017-10-11T05:52:16Z

editor/post-title/index.js

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


We can probably delete this now, right?

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

ephox-mogran · 2017-10-11T05:52:37Z

editor/post-title/index.js

@@ -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 } );


Maybe isSelected should be something like isTyping?

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

aduth · 2017-10-11T18:42:05Z

editor/post-title/index.js

@@ -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 } );


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

aduth · 2017-10-11T18:44:43Z

editor/post-title/index.js

+				className={ className }
+				onBlur={ this.onOuterBlur }
+				onFocus={ this.onOuterFocus }>
+				{ isSelected && hasFocusWithin && <PostPermalink onLinkCopied={ this.focusText } /> }


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

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?

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?

aduth · 2017-10-11T18:46:50Z

editor/post-title/index.js

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


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.

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.

aduth · 2017-10-11T18:50:01Z

editor/post-title/index.js

 	}

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


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?

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.

aduth · 2017-10-11T18:52:15Z

editor/post-title/index.js

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


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

…lect-blur-B

…not lose focus

…ying from clipboard

ephox-mogran · 2017-10-12T06:46:06Z

I've had another go at this which is working reasonably well. Essentially it uses a similar approach to clickOutside but for focus. I ignore blur events, and just use focusOutside to simulate the same thing. Hoping to update the PR soon.

ephox-mogran · 2017-10-12T07:29:01Z

@aduth what about this? I don't have particularly extensive tests, but I'm not really sure how to write them. Ideally, I'd want to create a hierarchy of components and some outside the component, and shift focus around and assert when the handleFocusOutside method fires.

aduth

Hmm, I'm wondering if the withFocusOutside higher-order component is necessary, or if it might be sufficient to fix the clipboard button container and move the onBlur introduced in #2948 from the Textarea to the root wrapping div (accounting for focus within both the textarea and post permalink).

aduth · 2017-10-13T00:33:29Z

components/clipboard-button/index.js

-class ClipboardButton extends Component {
+// This creates a container to put the textarea in which isn't removed by react
+// If react removes the textarea first, then the clipboard fails when trying to remove it
+class ClipboardContainer extends Component {


Does this need to be a separate component, or can we assign the button of ClipboardButton as the container?

It needs to have shouldComponentUpdate return false so that the container with the clipboard textarea isn't removed from the DOM. The problem is if it gets redrawn, then the textarea that is inside gets removed, and the clipboard library throws an error when trying to remove it. We just need a component that can get things added to it, and isn't going to get recreated.

I had it as the button originally, and kept getting an error in the clipboard library when it tried to remove the textarea, because react had already removed it because the button had been redrawn. You would get this error if you tried clicking on the button after you had already clicked on it. Therefore, the idea was to make a component that could have anything inside it, and react wouldn't keep trying to remove it. It's a limitation of using a third party library which appends components into the DOM. Does that make sense?

If you don't supply container, then it defaults to body, which is fine normally, but it will fire a blur which will trigger focus outside. That's why I'm supplying container ... to keep the focus inside the post title block.

aduth · 2017-10-13T00:36:52Z

components/higher-order/with-focus-outside/index.js

+			this.__wrappedInstance = ref;
+			// eslint-disable-next-line react/no-find-dom-node
+			this.__domNode = findDOMNode( ref );
+			if ( wrappedRefCallback ) {


What is the purpose of the wrappedRefCallback? Is this just inherited from click outside? Probably best to avoid introducing features unless we foresee needing them.

Yeah, based on the click outside. I can remove it.

Removed this extra wrapping.

aduth · 2017-10-13T00:39:23Z

editor/post-title/index.js

+	setFocused( focused ) {
+		this.setState( { hasFocusWithin: focused } );
+		if ( focused ) {
+			this.props.clearSelectedBlock();


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).

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.

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

aduth · 2017-10-13T00:41:02Z

editor/post-title/index.js

+		}
+	}
+
+	handleFocusOutside( ) {


Styling note: There should be no spaces within the parentheses:

No filler spaces in empty constructs (e.g., {}, [], fn()).

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

I'll plan to look and see if we can enforce this by ESLint.

aduth and others added 4 commits October 9, 2017 20:30

Title: Unselect title by blur event

ca44284

Using blur and focus bubbling to unselect title

2974da7

Firing clear selected block

ea8f8a1

focus textarea after copying button

8f2d430

ephox-mogran mentioned this pull request Oct 11, 2017

Title: Unselect title by blur event #2948

Merged

ephox-mogran commented Oct 11, 2017

View reviewed changes

ephox-mogran changed the title ~~Fix/1390 title unselect blur (B)~~ Title: Unselect title by blur event (B) Oct 11, 2017

gziolo requested a review from aduth October 11, 2017 11:27

aduth reviewed Oct 11, 2017

View reviewed changes

ephox-mogran added 4 commits October 12, 2017 14:49

Merge remote-tracking branch 'origin/master' into fix/1390-title-unse…

e2150d4

…lect-blur-B

partially working focus outside

d5f69c2

working, but forcing update ... had to create clipboard container to …

7f8c125

…not lose focus

using focusOutside, and keeping focus inside post-title even when cop…

e30a814

…ying from clipboard

very basic testing

5747cdc

ephox-mogran added 3 commits October 12, 2017 17:39

cleanup

f5dd96b

JSDoc spacing

e3179f5

removing old timers

61b7608

aduth reviewed Oct 13, 2017

View reviewed changes

ephox-mogran added 2 commits October 13, 2017 13:51

lint and removing wrappedRefCallback

9390fcc

unneeded clear selected block call

6103641

ephox-mogran closed this Oct 24, 2017

ephox-mogran deleted the fix/1390-title-unselect-blur-B branch October 24, 2017 20:49

aduth mentioned this pull request Oct 30, 2017

Autocomplete: Implement and use focus outside utility for managing dismissal #3235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ephox-mogran commented Oct 11, 2017 •

edited

Loading

codecov bot commented Oct 11, 2017 •

edited

Loading

ephox-mogran commented Oct 11, 2017 •

edited

Loading

ephox-mogran Oct 11, 2017

aduth Oct 11, 2017

ephox-mogran Oct 11, 2017

aduth Oct 11, 2017

aduth Oct 11, 2017

aduth Oct 11, 2017

ephox-mogran Oct 11, 2017

ephox-mogran Oct 11, 2017 •

edited

Loading

aduth Oct 11, 2017

ephox-mogran Oct 11, 2017 •

edited

Loading

aduth Oct 11, 2017

ephox-mogran Oct 11, 2017

aduth Oct 11, 2017

ephox-mogran commented Oct 12, 2017

ephox-mogran commented Oct 12, 2017

aduth left a comment

aduth Oct 13, 2017

ephox-mogran Oct 13, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017

ephox-mogran Oct 13, 2017

aduth Oct 13, 2017

ephox-mogran Oct 13, 2017

ephox-mogran Oct 13, 2017

aduth Oct 13, 2017

ephox-mogran Oct 13, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017 •

edited

Loading

aduth Oct 13, 2017

ephox-mogran Oct 13, 2017

ephox-mogran Oct 13, 2017

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

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

Conversation

ephox-mogran commented Oct 11, 2017 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 11, 2017 • edited Loading

Codecov Report

ephox-mogran commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Oct 12, 2017

ephox-mogran commented Oct 12, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

ephox-mogran Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Oct 11, 2017 •

edited

Loading

codecov bot commented Oct 11, 2017 •

edited

Loading

ephox-mogran commented Oct 11, 2017 •

edited

Loading

ephox-mogran Oct 11, 2017 •

edited

Loading

ephox-mogran Oct 11, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017 •

edited

Loading