Title: Unselect title by blur event

[aduth]

Fixes #1390

This pull request seeks to resolve an issue where the post title retains selected styles when tabbing away, since the title had only become unselected when explicitly clicking outside. The changes here capture focus being lost by blur event, thereby capturing both click outside and tabbing.

Testing instructions:

1. Navigate to Gutenberg > New Post
2. Click the title field
3. Press tab
4. Note that title focus styles are reset (note: your cursor may still be hovering the title from step 2, so hover styles may still be visible)

Future tasks:

There are a few other easy wins in the title component:

• Move <PostTitle /> to be handled within <WritingFlow /> to allow arrow navigation to move downward within content (Allow going down to the editor from the post title using keyboard #2052) Edit: Was already included in Framework: Extract arrow navigation to its own component #2424
• Consider either handling Enter press from <PostTitle /> or <WritingFlow /> to create a new default block after (Pressing return in the title field does not create a block #1650)

[gziolo]

LGTM

[youknowriad]

The problem with this change (I've already tried it) is that we can't click on the "Copy" permalink button anymore.

It's not an easy task to get right. I was hoping we could move Permalink Modal outside of the Post Title at some point to solve this issue

[ephox-mogran]

I've tried an approach to get the copy button to work as well. It is available here:#2974

The key parts are:

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

[codecov]

Codecov Report

Merging #2948 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2948      +/-   ##
==========================================
- Coverage   32.31%   32.25%   -0.06%     
==========================================
  Files         216      216              
  Lines        6137     6148      +11     
  Branches     1079     1080       +1     
==========================================
  Hits         1983     1983              
- Misses       3505     3515      +10     
- Partials      649      650       +1

Impacted Files	Coverage Δ
editor/post-permalink/index.js	0% <ø> (ø)	⬆️
editor/post-title/index.js	0% <0%> (ø)	⬆️
components/clipboard-button/index.js	0% <0%> (ø)	⬆️

---

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 b538874...8ef869d. Read the comment docs.

[aduth]

@ephox-mogran I cherry-picked some of your revisions in #2974 back into this pull request, specifically around assigning a custom container for the clipboard button textarea. Where this differs from #2974 is:

• Did not introduce withFocusOutside. This could be a handy convenience for a ! this.node.contains( event.relatedTarget ) pattern, but I'm not sure it needs to be adding a global event listener as clickOutside does
• Does not introduce a separate hasFocusWithin state
• Doesn't shift focus back to the title after copying. I could maybe see this being a convenience, but potentially also confusing for keyboard users who would have their focus suddenly changed

Future revisions I think we should make:

• Remove the onClick from the textarea for reactivating title selection, or at least offer a keyboard equivalent

[ephox-mogran]

If react makes relatedTarget supported in all browsers that you need to support, that's a great solution. I was worried that it wouldn't be ... but maybe that information is out-of-date. The only reason I was doing the blurOutside HOC was because in the past when you get the blur event, you don't always know what the newly focused element is. If you do now, you can definitely do it this way.

[aduth]

relatedTarget should match browser support (IE11+):

https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent/relatedTarget#Browser_compatibility

...but I do recall at least one note about it being buggy in a specific version of Safari? In any case, we are using relatedTarget in many other places in the application, so we'd have a fair amount of refactoring to do to move away from it.

[ephox-mogran]

Yeah, it looks like all relevant browsers might support it now.

[aduth]

Another incidental benefit of this change is that the title no longer re-renders when clicking anywhere on the page, as it does currently on master (caused by setState in handleClickOutside):

[jasmussen]

Perhaps this is tangential, and it's certainly a bit late. But the permalink showing as a Title block toolbar is arguably a failed experiment (I'm allowed to say that as it was my idea). If we move the permalink to the Status bar, perhaps the newly merged publish dropdown, we can remove it from here.

[youknowriad]

I'm still seeing the "copy" button bug where we can't click the button. But If we think we're moving this elsewhere after this PR, I'm ok with the simplest solution we had with the first commit.

[aduth]

I'm still seeing the "copy" button bug where we can't click the button.

Hmm, are you sure you're running the latest version of the branch? Which browser are you using?

[aduth]

Even if we simplify to remove the permalink from the title, I think some of the additional changes such as the clipboard button container are worth keeping to avoid future unexpected bugs.

[youknowriad]

@aduth yep confirmed it's working properly on Chrome but not Firefox

[aduth]

@aduth yep confirmed it's working properly on Chrome but not Firefox

😩 Might just be better off making the effort to move the permalink elsewhere.

[aduth]

Okay, tracked it down to Firefox not assigning the correct relatedTarget on the div wrapper's blur event. On a hunch adding a negative tabIndex, the relatedTarget is assigned as expected, presumably because this makes the div "focusable" (but not "tabbable").

Should be working okay after 553f546 . Also did a rebase, so you may need to delete your local branch.

[ephox-mogran]

I'd agree with @aduth , keeping the code to preserve the focus inside an expected container will save you trouble down the path. If you don't specify that container, then the clipboard library will transfer focus to a temporary textarea on the body, and that could active some of your blur/focus handling logic, and make it harder to track the flow of focus.

[ephox-mogran]

Just another note on what seems to be happening with that clipboard library. We're now working against its expected use case again. There is a click handler that is put on container, that is responsible for removing the textarea that it creates. Now, because of the change to make that container a div that is beside the button that we are clicking to trigger it, that click on the button never bubbles to the container, so the textarea is not removed until the button is clicked again.

It doesn't seem to be designed to work in the react ecosystem out of the box. Perhaps this library would be a better fit: https://github.com/nihey/react-clipboard.js (I haven't looked into it), though potentially it doesn't care about focus.

We essentially need:

• To put the textarea inside a component that is not going to get redrawn by react
• To put the textarea inside a component that is going to receive a click event bubbling up from the button
• To put the textarea inside a component that is not going to affect focus calls

Or we could leave it like this, and just accept that the textarea will stay in the DOM until the next click event on the button. It's not visible, so that might be the best compromise. Or, potentially, look into another library.

[aduth]

Nice investigative work @ephox-mogran . This is indeed tricky to handle. The library you reference doesn't seem to be much different than what we already have; in fact, it doesn't do anything about handling the container, so almost identical to the implementation prior to this pull request.

As an alternative to letting the click event bubble to the container for clean-up, we might be able to manually handle the clean-up ourselves by binding to the success and error events and calling removeFake, which is otherwise handled by the click bubbling you reference.

[ephox-mogran]

Yeah, I was worried it would be more of the same. If you have access to the removeFake, that seems a good way to do it. Is it a public API?

[aduth]

I was looking earlier today and I was having trouble accessing it. It should be available on this.clipboard.clipboardAction.removeFake, but this is not assigned at the time the success event is fired. I'll plan to take another pass later.

[ephox-mogran]

So the other (theoretical) option is dispatching a simulated click event on the container after we've received the click on the button. What? Too hacky ;) ?

[aduth]

I tried a number of approaches here, including the mouse event, each not working exactly right. One of the main issues I didn't discover until later was that the removal of the fake textarea causes a blur event which was intercepted by the PostTitle field, causing it to trigger its unselection logic.

In the latest approach I decided to revert closer to what exists in the current implementation, except with an added wrapping component to serve as the container for the fake textarea to be appended. In this way, the button click event still bubbles to the container and allows for the fake textarea cleanup. I think the original concern with this was whether React reconciliation would cause conflict with Clipboard's handlers, but I don't think there should ever be a case where React's reconciliation is destructive to manually assigned event handlers, and since Clipboard will immediately clean-up any of its own inserted textarea children, React should never observe any of these unexpected children.

[ephox-mogran]

Shouldn't this mean that the textarea blurring if filtered out? Why are you needing to stop propagation in the clipboard container? Is the relatedTarget not set?

[ephox-mogran]

It turns out it isn't. I thought relatedTarget was fragile. It may be that it is set in situations where something else has received a specific focus() call, and isn't set in situations where the focused element has been suddenly removed from the page and the body gets focus by default. In this situation, the active element seems to be body, but relatedTarget is nothing (when the textarea is removed).

[ephox-mogran]

Aside from being confused that the blurOutside isn't making a blur inside the textarea not call unselect, this looks good to me - but I'm probably just missing something.

However, without the stop propagation, it doesn't work. The blurIfOutside isn't even called.

Right, relatedTarget is null. This approach will work then. Because we can at least stop the blur propagating because we are controlling the container.

[aduth]

@ephox-mogran Great feedback. Yes, the stopPropagation was originally meant to handle the case where the textarea was suddenly removed and focus lost without a relatedTarget. However, I found that this also meant that because focus really had left the title field, blurIfOutside from PostTitle would never again trigger unless the user focused the title again first.

The better solution is to return focus to the triggering button before the fake textarea is removed, ensuring that it never leaves the title field. Digging through Clipboard's source, I find that they expose this directly via the clearSelection function on the success callback argument:

https://github.com/zenorocha/clipboard.js/blob/0c3bce265f6e8bbe6dfd63ec7b01c6e9394c8c93/src/clipboard-action.js#L133-L142

Why this is not the default behavior I'm unsure, but it was simple enough for me to add this into the success callback and drop the stopPropagation altogether.

[ephox-mogran]

Excellent.

[youknowriad]

Nice 🚢 it