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.

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

Prevent focus steal on clicking the inserter #18379

Merged
merged 4 commits into from Dec 3, 2019

Conversation

@draganescu
Copy link
Contributor

draganescu commented Nov 7, 2019

Description

Fixes #18329

The problem was that the inserter sometimes stole focus from the currently selected block depending on which appender it used. The insertion point appender handled it correctly:

inserter-weirdness

I have copied the same technique to all the other situations by creating a new appender exposed by InnerBlocks named ... InnerBlocks.UnfocusableButtonBlockAppender.

solved-appender

How has this been tested?

Tested locally

Types of changes

All the appenders need to have their focus event stopped from propagating and their tab index set to -1.

Changes to navigation:

I have been unable to keep the movers from stealing the focus of RichText using the same event technique from above, so as a solution (temporary?) I found the keepPlaceholderOnFocus option for RichText and the result is that we no longer have an issue.

I also think it looks better :) @karmatosed what do you think?

Before:

mover-focus-before

After:

mover-focus-after

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Nov 8, 2019

Interesting approach @draganescu, very creative!

I think it needs some design feedback. The thing I spotted is that I'd expect the button to either be aligned with the menu items or have the same margin from top and right of the block.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 8, 2019

@talldan fixed the position, thanks for spotting it :)

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 8, 2019

The problem is that when the menu item is selected its UI grows and when it gets deselected it shrinks. When one clicks the appender the menu item is deselected and the UI shrinks and the appender flows in and the focus event on it is not triggered anymore.

By positioning it absolutely it will not move based on the size of the menu item UI and it will work.

We should get to the bottom of the focus issue, not work around it by hacking element positioning.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 8, 2019

This behaviour is consistent to how other focus is lost when clicking the appender in some scenarios, see PR description image above.
I'll explore what makes the middle appender better at not stealing focus.

cc @mcsf @mtias

@draganescu draganescu force-pushed the fix/double-click-to-insert-menu-item branch from 2b30d02 to 3885a7e Nov 8, 2019
@draganescu draganescu requested review from ellatrix and youknowriad as code owners Nov 8, 2019
@draganescu draganescu changed the title Fixes double clicking needed to add menu item Prevent focus steal on clicking the inserter and the movers Nov 8, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 8, 2019

I have redone this PR and used the same technique which the insertion point uses to prevent stealing focus: stop propagation of the focus event and set tab index to -1.

I will set the same behavior for the movers in the same PR, I have also renamed it as it is no longer about the navigation block.

@mtias @mcsf

Copy link
Contributor

talldan left a comment

I'm curious as to why it required so many changes across various files, I would've thought there would be only one appender used in the block.

Anyway, when testing, I spotted one main issue. The button appender is no longer appearing full-width in the group or columns blocks, probably due to the wrapping element:
Screen Shot 2019-11-11 at 4 26 08 pm

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 12, 2019

I'm curious as to why it required so many changes across various files, I would've thought there would be only one appender used in the block.

It required so many changes because I kept discovering new places that stole focus :)) of course, like you suggested, there is indeed just one place to update. I did just that and added an explanatory comment copied over from the insertion point component.

Copy link
Contributor

tellthemachines left a comment

Tested across several browsers (IE included) and this is working very well

Copy link
Contributor

talldan left a comment

Sorry, spotted another issue. It looks like the button appender in the group block now requires two clicks (if the block is unselected, it first requires a click to select the block). Before this change it only needed one:
group-double-click

I notice the original issue is also still happening on the social links block.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 14, 2019

I may have to add the focus preservation behaviour conditionally if the focus is on the same parent block ... Thinking ...

(edit)

I does fire but these appenders (social, group) also show the inserter which disappears because the block gets focused.

(2nd edit)
Ok, so, @talldan here is the current status:

  1. It seems to be working for menu, group, social icons, normal inserter, columns etc.
  2. I had to prevent the input in the inserter menu to send focus up the tree but then
  3. I had to manually select the block where the inserter is contained.

We'll see how E2E feels about this, b/c who knows what may be broken by this approach.

Kinda intricate but now it works. The only visual glitch is the inserter menu jumping a bit:

inserter-jumps

@draganescu draganescu force-pushed the fix/double-click-to-insert-menu-item branch 2 times, most recently from 27bdaa9 to f43e4a4 Nov 15, 2019
@draganescu draganescu requested a review from mkaz as a code owner Nov 19, 2019
Copy link
Contributor

getdave left a comment

👍 I like the way we've now isolated this via a prop rather than trying a fix-all. Seems logical.

Here's me testing on the Nav Block. Makes the experience feel SO much better. Thank you!

Screen Capture on 2019-11-19 at 13-50-23

Looks like you need a rebase on packages/block-library/src/navigation-menu/edit.js.

packages/block-library/src/column/edit.js Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@ function BlockListAppender( {
canInsertDefaultBlock,
isLocked,
renderAppender: CustomAppender,
__experimentalPreventAppenderFocus,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 19, 2019

Contributor

It's not specific to this PR but I'm seeing the trend of adding experimental props/APIs a lot as a way to escape backward compatibility concerns. And sometimes we do this to avoid thinking a lot about the implications and what would it take to do the stable API right (documentation, dev notes, deprecations...) (I also did that some times so I include myself in this trend)

This is concerning a little bit as for most of these, there doesn't seem to be a plan to how/when we make these stable? Can we stop doing this unless we really think it's something subject to change? We should force ourselves to think about the APIs more.

Maybe we could solve this by making a comment in the code to explain why this has been made experimental and how to we plan to make it stable?

This comment has been minimized.

Copy link
@getdave

getdave Nov 19, 2019

Contributor

there doesn't seem to be a plan to how/when we make these stable?

I assumed there was a plan!

I feel this needs to be discussed in next Core Editor meeting. Do you agree?

Perhaps we could have some automation which can tell us how long a prop has been __experimental and highlight those which should be considered for prime time. Relying on humans being diligent and remembering to go back and "upgrade" the experiments seems like it could easily go wrong.

Maybe we could solve this by making a comment in the code to explain why this has been made experimental and how to we plan to make it stable?

Once the comment has is this a blocker for this PR?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 19, 2019

Contributor

No, not a blocker when documented.

@noisysocks noisysocks dismissed talldan’s stale review Nov 19, 2019

PR has changed significantly since this review

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 19, 2019

Problem: the menu block has an appender. When a menu item is selected and one clicks the appender, the menu item is deselected because the appender steals the focus, and the appender moves away from under the mouse and does not receive the focus, so nothing happens. You then have to click the appender again to insert a new menu item.

Research: the appender steals the focus in many other situations as well, and this double click issue is also present in the social links block. Also for example in the columns block clicking the appender without the block selected selects the block, clicking the appender again opens the inserter menu. However in the group block clicking the appender without the block selected opens the inserter directly.

Observation: the insertion point appender does not remove the selection of the currently selected block.

Solution: apply the technique used for the insertion point of focus preservation, selectively and optionally when we want the flow to be smoother, such as for the navigation block or the social links block.

Other ways tried: trying to solve this for all the inserter/appender instances messed with the default selection mechanisms in various places (reusable blocks for example, or dynamic allowed blocks) and many tests failed. The core problem is that when we prevent the focus being set on the appender it also doesn't bubble up so the parent block of the appender is also not focused, and hence not selected. Manually selecting it is complicated.

@draganescu draganescu changed the title Prevent focus steal on clicking the inserter and the movers Prevent focus steal on clicking the inserter Nov 21, 2019
@draganescu draganescu force-pushed the fix/double-click-to-insert-menu-item branch from 06c7a71 to 82b33f6 Nov 21, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 21, 2019

After some trial and error @noisysocks and me (@draganescu) along with @youknowriad 's review concluded that __experimentalPreventAppenderFocus was not the best solution.

We opted to solve it on a case by case fashion and we're starting with the NavigationMenu.

This new InnerBlocks.UnfocusableButtonBlockAppender exists because other blocks might want to implement it such as the SocialLinks block which is having the same double click issue.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 21, 2019

The fix is small and comprehensible, that said, I'm still not sure it's the best path forward. I wonder what's the root issue here. I guess it's related to this observation.

thee appender moves away from under the mouse and does not receive the focus, so nothing happens

I'd personally prefer if we find a consistent way to fix this for all inserters.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 22, 2019

@youknowriad fixing it for all inserters is very complicated and the problem isn't really Gutenberg's it's some bad browser behaviour.

I would love to have something merged to make the Navigation and Social Links feel better sooner, as the interaction is very clunky with that double click.

I have attempted general fixes using the same approach (the only one that seems to work) but it interferes with default block selection, so i becomes much more complicated and harder to test.

Could we come back to it and have this fix for the navigation block 1st?

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Nov 22, 2019

I noticed yesterday that we ignore the nested focus event for the DefaultBlockAppender:

<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<DefaultBlockAppender
rootClientId={ rootClientId }
lastBlockClientId={ last( blockClientIds ) }
/>
</IgnoreNestedEvents>

Could we do this for the ButtonBlockAppender as well?

@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Nov 22, 2019

@noisysocks done adding IgnoreNestedEvents to ButtonBlockAppender

…y focuses right after
@draganescu draganescu force-pushed the fix/double-click-to-insert-menu-item branch from b4d8594 to 5c0654e Dec 1, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Dec 1, 2019

This finally works and it has a perfectly elegant solution, thanks to @noisysocks 's great efforts into investigating Firefox quirks. Here is an explanation by the man himself:

So there’s two separate fixes to two separate but similar bugs: one where ButtonBlockAppender needs to be clicked twice and one where Inserter::defaultRenderToggle needs to be clicked twice

For ButtonBlockAppender:

Cause: The default browser behaviour is that the mousedown on the button triggers a focus event which propagates up until it reaches the Navigation block’s BlockList component. This component then changes the currently selected block to be the Navigation block meaning that the appender is remounted and no longer clicked.

Fix: preventDefault() on the mousedown prevents the default browser behaviour of triggering a focus event. click (when focused) and focus (when selected via keyboard) still fire as normal

For Inserter::defaultRenderToggle:

Cause: Here, the default browser behaviour differs. In Chrome, the mousedown triggers a focus event on the appender button as it should and then the click event follows. In Firefox, though, the mousedown triggers a focus event on the .edit-post-visual-editor element. I can’t for the life of me figure out why this happens. This focus event causes BlockSelectionClearer to clear the block selection meaning that the appender is remounted and no longer clicked.

Fix: preventDefault() on the mousedown event prevents the default browser focus event of triggering a focus event, but we still want the appender button to to be focused or else pressing Esc in the inserter will not return focus to the appender. Thus, we preventDefault() and manually focus the appender button.

…tBlock, removes the preventDefault for the buttonBlock and adds a regresion test to Navigation block
@draganescu draganescu requested review from nerrad and ntwb as code owners Dec 2, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

draganescu commented Dec 3, 2019

Thanks for the review @noisysocks I went through your obs, and fixed accordingly.
Will merge this as it appears to be the most stable and widest reaching solution. 🍾

@draganescu draganescu merged commit cb074bd into master Dec 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@draganescu draganescu deleted the fix/double-click-to-insert-menu-item branch Dec 3, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 13, 2019

I believe this PR may have broken the ability to select a parent block by clicking it.
click parent

I found this in latest shipping master, then worked through the commit history:

  • git checkout cb074bdf3f4130977aba07b7b63ccccb74f5c4c5 (this branch merged to master) doesn't work
  • git checkout 9280775f7679481b8a1a82b7571347deaadb9336 does work

Here's how it looks when it works:

click parent working

Not sure which bit of code caused this.

CC: @draganescu @noisysocks @youknowriad

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 13, 2019

I suspect it's also the cause of #18928

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 13, 2019

Follow-up at #19135 should fix the issues described in #18379 (comment) and at #18928.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.