Blocks: Unselect the block when clicking outside the current block

[youknowriad]

closes #723

This PR changes the way we trigger the "deselect" action, leveraging the react-click-outside library instead of the onBlur event handler.

This should fix the issues seen in #723
The fix here also fix the auto focus on link input closes #679

[jasmussen]

Wow, you work fast!

I'm still seeing some issues though, let me know if you can't reproduce them:

https://cloudup.com/cCb3hTHs7xh

[youknowriad]

@jasmussen I'm not seeing these errors. Which browser is this?

[jasmussen]

Woot you're right — it was aggressive cache on my part. Can confirm that this seems to fix it! 👍

[aduth]

This creates a global click handler for every block visible on the page. When you click anywhere on the page, the Redux store is flooded with TOGGLE_BLOCK_SELECTED. At the very least I think we should only call onDeselect if the block is currently selected.

Good catch @aduth Should be fixed now

[aduth]

On second review, I'm worried that detecting click outside is flawed in how it's moving further away from reacting to the focus behaviors themselves. For example, try clicking the borders of a block and tabbing, or focusing the first block and then shift-tabbing. Focus is no longer transitioned cleanly because it relies on the click behavior (though sometimes it does because of the next block "stealing" selected).

I think we should try to understand the challenges with focus that led to the issues observed in #723 and see if we can resolve them while still using focus events instead of click events.

[youknowriad]

@aduth I couldn't find why the "blur" event is not triggered in the current approach in master. Should we rely on both "onBlur" and "onClickOutside"?

[aduth]

I managed to track it to focus shifting to body after clicking up causes the mover button to become disabled (because it's first in the list). I'm not sure why this is the behavior (especially without calling blur on the block) but "faking" the disable seems to allow focus to stay within the block so that the next click outside causes a real blur:

diff --git a/editor/components/block-mover/index.js b/editor/components/block-mover/index.js
index e872113..fe484c9 100644
--- a/editor/components/block-mover/index.js
+++ b/editor/components/block-mover/index.js
@@ -17,13 +17,13 @@ function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
                                className="editor-block-mover__control"
                                onClick={ onMoveUp }
                                icon="arrow-up-alt2"
-                               disabled={ isFirst }
+                               aria-disabled={ isFirst }
                        />
                        <IconButton
                                className="editor-block-mover__control"
                                onClick={ onMoveDown }
                                icon="arrow-down-alt2"
-                               disabled={ isLast }
+                               aria-disabled={ isLast }
                        />
                </div>
        );
diff --git a/editor/components/block-mover/style.scss b/editor/components/block-mover/style.scss
index a93bcef..a7b6970 100644
--- a/editor/components/block-mover/style.scss
+++ b/editor/components/block-mover/style.scss
@@ -19,9 +19,10 @@
                color: $dark-gray-900;
        }
 
-       &[disabled] {
+       &[aria-disabled="true"] {
                cursor: default;
                color: $light-gray-300;
+               pointer-events: none;
        }
 
        .dashicon {

[youknowriad]

@aduth Awesome, how did you track this :P

In the second commit, I've fixed the "link" focus issue by auto-focusing the input (which was required in #679 ). I had to disable a lint error for this.

About jsx-a11y I'm starting to question whether we really need it because it doesn't detect the "real" accessibility issues.

Addressed

[notnownikki]

Works well for me 👍

[aduth]

@aduth Awesome, how did you track this :P

With some not-so-clever hackery 😄 I figured the focus must be moving somewhere if the next click on the body after moving the block up wasn't unselecting it. Since focus is fickle, I added some basic logging to see which element was active along the way:

setInterval( () => console.log( document.activeElement ), 500 );

Based on what I saw, it seems that because we cause the button to become disabled while it has focus, it reverts the page to an unfocused state. What is still baffling to me however is that this doesn't cause a blur event to fire on the block. Unsure if this is expected DOM behavior or perhaps a React bug.

[aduth]

About jsx-a11y I'm starting to question whether we really need it because it doesn't detect the "real" accessibility issues.

For me at least, I find it helps surface better understanding of where pain points with accessibility can arise. Following the rule doc to the guideline warning, it seems to be an issue of disorientation from moving focus around. In this case, the focus change is a result of an intentional action (activating the link button), so I'd imagine the user should expect the change. Maybe we can also hint to this on the button (aria-haspopup?).

[aduth]

In fd2658f I documented why we're using a non-standard approach to disabling the button, and also changed onClick behaviors to account for the disabled effect. I wasn't able to recreate it, but I worried that if the button still had focus when disabled, even if we prohibit pointer events, keyboard action could still occur.