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

Selecting Parent Blocks: Try clickthrough #15537

Merged
merged 11 commits into from Jun 6, 2019

Conversation

@jasmussen
Copy link
Contributor

commented May 9, 2019

This PR takes a stab at using existing code by @aduth that is implemented on mobile, and expanding it to the desktop breakpoint as well. Specifically, it enables what we in #9628 refer to as "click-through".

With "click-through", whenever you interact with a block that has innerblocks, a click first select the parent, and then it selects direct descendants. For example you first click the Cover block, then you click the Paragraph block inside. Or you first click the Columns block, then you get to select the Column (singular) block, and then you can select content inside. GIF:

clickthrough

This branch is a work in progress. There are issues to refine and work out.

Click-through is inspired by how many desktop applications handle "groups". There, you commonly have to double-click a group to be able to select items inside. This PR is the same, except you click only once to "open" the group. There are a great deal of benefits to this approach:

  1. It's very simple. It makes it trivial to select each layer of even complex blocks as they are today.
  2. It's safe, in that it can accommodate almost any complex block you can throw at it, with a few exceptions I'll get into in a bit.
  3. It does not require fine motor skills to select a parent block. The hit areas are most literally as big as the block itself.

The first thing you might think is: what about situations where you want to edit text quickly?

That specific question is the primary reason it's taken until now to test this out in practice. My assumption was that this would create unnecessary friction to editing text. However:

  • In testing this actual PR, that feels like a complete non-issue. I invite you to test this PR out, even in this buggy early form.
  • The behavior feels completely intuitive in actual practice. Even if you select the parent first, you get visual feedback all along the way, and you'll quickly learn that if you mean to edit text inside the Cover block quickly, you can just double-click — one click to select the Cover block, another to select the text you're pointing at.

Issues

  1. The current behavior "cycles through" the layers if there are more than two levels of nesting, i.e. as it the case in the Columns block:

cycle

This is the code: ! isSelectedBlockInRoot — we'll likely need an additional variable that is aware of immediate parent and not just the root ancestor.

  1. There is an issue with blocks being inserted using the slash command — those are still "locked" behind the click-through, even though it is keyboard selected. This will likely be similar to if you set focus inside a child block with the keyboard: however you selected the child block, if it's selected, it should be "ungrouped".

Other considerations

For now this is a draft PR, but I strongly urge you to try it out and not just look at the GIF. It feels better in actual practice, than it looks, and with things like #15499 on the horizon, this could be a good baseline improvement to improve the situation with selecting parent and child blocks.

It also doesn't preclude additional and separate improvements from taking place, such as:

  • #14961 which adds dashed outlines to show the presence of parent or child blocks.
  • Refactors of blocks such as columns to maybe be closer to the Media & Text block in terms of how child blocks are surfaced to the user. For example it seems like it should be possible to create a columns block with just 2 levels of nesting, rather than 3.

For now, though, I would really appreciate your thoughts on this PR. Please, more than anything, take it for a spin!

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

🎉 This is an excellent exploration. Like @jasmussen, I originally thought this would seem cumbersome, but in practice it feels really natural. I also agree that it could live alongside a change like #14961: this PR makes it easy to click down the tree of nested blocks, but the other PR would make it easier to click back up the tree as well. I actually tried smashing these PRs together, and in general it seems quite usable to me:

borders-too

That said, I'm not sure the additional borders are totally necessary if we went this route? This does feel fairly natural as-is.

I 100% agree that clicking 3 times to get into the columns block is annoying, but that block needs some reworking anyway. I think it'd be fine to build in a special case solution for it in the meantime (like maybe the second click bypasses the individual column block).

Aside from the little hiccups Joen noted above there are a couple other aspects of this that need a bit of consideration:


  1. When a parent block is full-aligned, the hover state is easily missable. (It'll be even more missable if we end up removing the block breadcrumb like we've proposed in other tickets). Since this is the case, it's a little unclear why the child blocks aren't highlighted when you hover over them:

full-width

This is obviously remedied by just clicking once, so I'm not sure it's a huge deal though.


  1. When working in the toolbar of a block that immediately follows a nesting block, toolbar clicks can sometimes select the preceding block, instead of the toolbar item. This appears to happen when the hover state is active for the preceding block:

select

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Also, a quick suggestion: in many design apps, you can directly select a child of a group by holding a modifier key when clicking (often the command key). It'd be cool to enable that here too for power users.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Thanks so much Kjell, your thoughts are much appreciated, and they are all solid. I definitely think the outlines branch and this PR could work well together. It is a very complex UI issue, and multiple approaches are sound. We might even find, if this branch works well, that the outlines branch can become slightly subtler than it is now — thinking about it, it seems perhaps more important to show the outlines when the parent block is selected, given the behavior suggested in this branch.

Your observation on full-wide is solid. I would suspect that the outlines suggested right in the previous paragraph is one way in which this can become more visible. Another might be to actually show a color overlay when hovering to indicate the boundaries. But this is probably best explored separately.

The 2nd bug you mention, yep I can reproduce it. Great catch, we'll want to fix that for sure. I've asked @gziolo for some help with some of these things, and hopefully we can fix this up together.

Also, a quick suggestion: in many design apps, you can directly select a child of a group by holding a modifier key when clicking (often the command key). It'd be cool to enable that here too for power users.

That's a killer tip, I love it. In addition to being able to just double-click, that will be a nice detail!

@gziolo

This comment has been minimized.

Copy link
Member

commented May 13, 2019

It's better with effe011 but I still need to further investigate how to make this behavior more consistent:

clickthrough

I guess it's fine to optimize it with Column block being the one which intercepts the click as it will have to be covered with nested Group blocks regardless. We can remove the overlay behavior from Column block later. That's probably the easiest part.

@gziolo

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Also, a quick suggestion: in many design apps, you can directly select a child of a group by holding a modifier key when clicking (often the command key). It'd be cool to enable that here too for power users.

I like this proposal a lot. If you look at my screencast you can see that double-click and triple-click trigger the default browser behavior: selecting a word and a group of text.

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I'm loving this PR! Here's some observations:

  1. The clickthrough on columns might be a bit confusing to clickthrough 3 times (Columns -> Column -> Content) to get to the content for new users. I think it's something people can grok, but at first clicking to get the column, and not just the content was a bit odd... although I believe it's needed.

  2. A BUG: Clicking through to the image in a Columns block pushed me directly to the "Edit" image placeholder. I don't think this should happen, correct?

clickthrough

The clickthroughs feel right to me. It's so much better than trying to fidget with the cursor to grab just the parent block. Thanks for this exploration!

@gziolo

This comment has been minimized.

Copy link
Member

commented May 14, 2019

In 5ee69d0 I added background colors to make it easier to debug why a single Column block gets selected from time to time:

Screen Shot 2019-05-14 at 09 10 37

It looks like overlays don't fit properly to the are they should cover. I'm also wondering whether their order shouldn't be reversed. That would explain why deeply nested blocks sometimes get selected instead of Column block.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Thanks everyone for the feedback. I will look a bit today at some fixes to what you've noted:

  • Should be able to doubleclick to quickly "unlock" text inside nesting,
    without also selecting text. Maybe possible with some pointer-events magic.
  • Will explore an overlay color for the contexts where the hover state is otherwise not obvious, like fullwide.
  • Adjacent toolbars have a lower z index than the overlay — Kjell's reported toolbar issue.

@mapk the Edit Image placeholder thing appears to happen in master too! And as far as I can tell, only in Safari:

master

Can you open a new issue for this? It's possible that "double-click" is intended to go to that edit mode, though that feels a bit weird.

I would also agree that it's a bit annoying that you have to click three times on columns. But I would also suggest that this is a more fundamental issue with the columns block itself, and something that's worth exploring improvements to separately. If at all possible it'd be nice if individual columns didn't have to be selected individually, but that options for resizing and aligning were available directly in the top level columns block. Much like "Media & Text", in other words.

Given a change was recently merged to make the individual column block selectable again, because it can now be resized, I think we should explore reducing the amount of nesting for this block as a separate effort. What do you think?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

In 84a13f7 I fixed the issue where when you try to click the toolbar of a block that's adjacent to a block with the clickthrough overlay, you'd select the overlay.

But I'm surfacing it here because it feels a bit wrong to be giving the contextual toolbar such a high z-index... but it has to be higher than the click overlay. @aduth can you recall why the click overlay was given a z-index of 120? Do you think 2 could be sufficient?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I spent some time investigating whether it was possible to not select words when doubleclicking, and select all text when triple clicking. As it turns out because this is operating system level behavior, it's not really possible to disable that. You can use various pointer-events and user-select to prevent selection of text, but once the actual editable text is finally unlocked, you'll still have a triple-click "queued up" which instantly fires to select it. Here's a codepen showing it. Try doubleclicking the text.

Barring a hack I cannot think of, this aspect is not fixable without JavaScript. That is — if we truly loathe that quickly double clicking to edit text inside a nested block also selects that text — we'll have to unset the text selection at the end of the operation. Which we should probably avoid for now, both since it's rare, and because it's a disruptive fix we can explore separately.

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Performance benchmarks

This branch

Average time to load: 5503ms
Average time to DOM content load: 4805ms
Average time to type character: 98.98ms
Slowest time to type character: 244ms
Fastest time to type character: 72ms
Average time to load: 5831ms
Average time to DOM content load: 5278ms
Average time to type character: 110.195ms
Slowest time to type character: 266ms
Fastest time to type character: 77ms
Average time to load: 5801ms
Average time to DOM content load: 5142ms
Average time to type character: 99.555ms
Slowest time to type character: 218ms
Fastest time to type character: 72ms

Master:

Average time to load: 5573ms
Average time to DOM content load: 5068ms
Average time to type character: 103.945ms
Slowest time to type character: 225ms
Fastest time to type character: 78ms
Average time to load: 5664ms
Average time to DOM content load: 5122ms
Average time to type character: 99.325ms
Slowest time to type character: 203ms
Fastest time to type character: 79ms
Average time to load: 5666ms
Average time to DOM content load: 5001ms
Average time to type character: 100.795ms
Slowest time to type character: 212ms
Fastest time to type character: 71ms

Result

Those branches are nearly the same for the existing benchmark when you compare median values:

Name Before After
Average time to load 5664ms 5801ms
Average time to type character 100.795ms 99.555ms
@gziolo

gziolo approved these changes Jun 6, 2019

Copy link
Member

left a comment

It looks good code-wise. I added most of the JavaScript changes so I would appreciate confirmation from @youknowriad given his expertise around the performance implications related to changes applied. Benchmark results look good but I might miss something important.

useSelect,
} from '@wordpress/data';

const BlockAsyncModeProvider = ( { children, clientId, isBlockInSelection } ) => {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 6, 2019

Contributor

Nit: Conceptually speaking and if there's no impact on performance it would be better to avoid the isBlockInSelection as a prop and compute it here :).

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 6, 2019

Member

b0649e1 - I had it before but I assumed it could impact performance. I can try with the previous commit.

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 6, 2019

Member

I don't thin that benchmark agrees with this idea:

Average time to load: 5724ms
Average time to DOM content load: 5219ms
Average time to type character: 187.28ms 😅 
Slowest time to type character: 319ms 🐢 
Fastest time to type character: 158ms 🐢 
Average time to load: 7533ms 🐢 
Average time to DOM content load: 7011ms 🐢 
Average time to type character: 177.2ms 🐢 
Slowest time to type character: 321ms 🐢 
Fastest time to type character: 145ms 🐢 

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 6, 2019

Contributor

:)

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Hooraay! I see a green button and approvals. I will wait for final sanity checks by Riad, and advice on when it's best to merge. But I'm excited to see this land. It solves numerous ux issues around block nesting, and although it takes a little getting used to first time you use it, the PR that @kjellr has pending shows deftly that it is possible to improve the UX further, with visual feedback. Thanks everyone.

@youknowriad
Copy link
Contributor

left a comment

Run the performance tests as well and I have similar results as well.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Thanks everyone for your hard work on this PR 👏

@jasmussen jasmussen merged commit c5e8784 into master Jun 6, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jasmussen jasmussen deleted the try/clickthrough branch Jun 6, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Hooray! Thank you all for the help

Now that it's in, I will work with Kjell to refine the experience.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

This PR rocks! Thanks for continued push to merge. You all did fantastic here! 🎉

@niklasp

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

nice. i want to use it now.

@ktmn

This comment has been minimized.

Copy link

commented Jun 13, 2019

@jasmussen Thanks, I hate it :)

I can see this feature being useful for the average user, but as a "power user" it slows me down and confuses me immensely. In some instances I need to click up to 5 times just to edit some text. I guess everyone else only tested it with core blocks and 1-2 levels of nesting, in which case it's fine.

Additionally now it's hard to select an element with the Dev Tools since it won't let you select it unless your editor focus is in the right place.

Definately needs a toggle in my opinion.

I would prefer a feature such as holding down Alt and scrolling up to select parent block and scrolling down to cycle between children blocks.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Thanks for your feedback. There are a number of refinements to this underway, including some outlines to show parent and child elements and their relationship when a block is selected, and it's also been suggested to implement a keyboard shortcut, so when you hold for example ⌘ and click, you click the nested block under your mouse directly.

A per-block opt-in or opt-out has also been discussed. This would be set by the block author, to opt in or out of the click overlays. So for example if the author of the Media & Text block might opt out of the overlay because it's not necessary for the operation of the block. However for other blocks such as the Columns block, it's virtually impossible to select the individual columns without this interface, so it's important to have a baseline tool for making that possible even for casual users that don't know about keyboard shortcuts or toolbar buttons.

I'm personally excited about the keyboard shortcut. It feels like a good balance between a userfriendly interface for the casual user, and a tool for powerusers to know if they want to click right through it all. @gziolo do you have any thoughts on how easy/hard such a keyboard shortcut would be to build? Essentially the clickthrough layer needs to be aware of whether the button is pressed down, and then simply behave as if it's not there (pointer-events: none;).

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I'm personally excited about the keyboard shortcut. It feels like a good balance between a userfriendly interface for the casual user, and a tool for powerusers to know if they want to click right through it all. @gziolo do you have any thoughts on how easy/hard such a keyboard shortcut would be to build? Essentially the clickthrough layer needs to be aware of whether the button is pressed down, and then simply behave as if it's not there (pointer-events: none;).

We use a similar approach for multi-selecting blocks when you hold down Shift key and click on blocks. I'm sure we can replicate a similar behavior for the clickthrough layer. I think this is the existing logic:

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/writing-flow/index.js#L301-L319

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I did a quick check with the following code:

diff --git a/packages/block-editor/src/components/inner-blocks/index.js b/packages/block-editor/src/components/inner-blocks/index.js
index 75a6731f4..bc59bd92f 100644
--- a/packages/block-editor/src/components/inner-blocks/index.js
+++ b/packages/block-editor/src/components/inner-blocks/index.js
@@ -29,9 +29,12 @@ class InnerBlocks extends Component {
 	constructor() {
 		super( ...arguments );
 		this.state = {
+			clickthroughEnabled: true,
 			templateInProcess: !! this.props.template,
 		};
 		this.updateNestedSettings();
+		this.onKeyDown = this.onKeyDown.bind( this );
+		this.onKeyUp = this.onKeyUp.bind( this );
 	}
 
 	getTemplateLock() {
@@ -102,20 +105,42 @@ class InnerBlocks extends Component {
 		}
 	}
 
+	onKeyDown( event ) {
+		if ( event.metaKey ) {
+			console.log( event.metaKey );
+			this.setState( {
+				clickthroughEnabled: false,
+			} );
+		}
+	}
+
+	onKeyUp( event ) {
+		if ( event.metaKey ) {
+			console.log( event.metaKey );
+			this.setState( {
+				clickthroughEnabled: true,
+			} );
+		}
+	}
+
 	render() {
 		const {
 			clientId,
 			hasOverlay,
 			renderAppender,
 		} = this.props;
-		const { templateInProcess } = this.state;
+		const { clickthroughEnabled, templateInProcess } = this.state;
 
 		const classes = classnames( 'editor-inner-blocks block-editor-inner-blocks', {
-			'has-overlay': hasOverlay,
+			'has-overlay': hasOverlay && clickthroughEnabled,
 		} );
 
 		return (
-			<div className={ classes }>
+			<div
+				className={ classes }
+				onKeyDown={ this.onKeyDown }
+				onKeyUp={ this.onKeyUp }
+			>
 				{ ! templateInProcess && (
 					<BlockList
 						rootClientId={ clientId }

I don't think it works as expected, this might need to have to be controlled from WritingFlow as well as other keyboard events. It definitely won't work with Shift as it conflicts with multiselection, but it might be fine to use ⌘ and click as suggested above 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Awesome, thanks so much for looking.

We definitely want to use ⌘ on a Mac, as that's the standard keyboard shortcut for this behavior. Figma, Sketch and Illustator are examples of this behavior, where if you have a group, a single click selects it, but a ⌘ click selects the item inside the group directly.

I can't recall what the corresponding Windows shortcut key is, it's probably Alt, but perhaps @kjellr can recall?

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

I can't recall what the corresponding Windows shortcut key is, it's probably Alt, but perhaps @kjellr can recall?

Alt sounds right to me, but I don't have a windows machine around to double check at the moment.

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Selecting Parent Blocks: Try clickthrough (WordPress#15537)
* Selecting parents: Try clickthrough.

Clickthrough has you select the parent before you can select the child. This is already in place on the mobile breakpoints, this just expands it to desktop as well.

It is a work in progress, right now it is not working as intended: once you have "unlocked" the deepest level, it becomes immediately locked and you have to click through the layers again to unlock it again. The deepest layer should always be unlocked until you deselect all blocks again.

* Render overlay on top of inner block only when none of the nested blocks is selected

* Fix overlay, fix breadcrumb, polish.

* Remove click-overlay.

* Fix the selection of inner blocks for reusable blocks template

* Disable async mode for parent blocks when nested block selected

* Refactor BlockListBlock to use AsyncModeProvider

* Make the reusable blocks save button clickable. i

At least this means you can edit and save reusable blocks.

* Fix so reusable blocks with nesting are editable.

* Fix movers.

The z-index was too low. It had to be higher to accommodate the other z-index changes.

* Bring the behavior closer to what we have as of today
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.