Inserter: Fix the insertion point #994

youknowriad · 2017-06-02T11:08:27Z

There were some regressions to the insertion point feature due to multi-selection #896

The behaviour of this PR:

When we have a multi-selection, the block is inserted after the multi-selection
When clicking outside the editor, the selection is not cleared (multi or mono). I thought it's good to keep a consistent behaviour (Though we can decide to clear the multi-selection only)

Open questions:

I think we should unify the concept of selected block and multi-selection into a unique state

jasmussen · 2017-06-02T11:13:27Z

Can confirm the inserter works great now, with no JS breakage that I can see.

mtias · 2017-06-02T11:26:16Z

editor/modes/visual-editor/block-list.js

 		onMultiSelect( { start, end } ) {
 			dispatch( { type: 'MULTI_SELECT', start, end } );
 		},
 	} )
-)( clickOutside( VisualEditorBlockList ) );


This means we can't deselect a block by clicking on the canvas now.

Yes, and I think you explicitly suggested this somewhere? Am I wrong?

What I meant is that focused and selected should be two different states, where focus also means selected. Clicking on the canvas should deselect the block, what I meant before is that clicking on the inserter should unfocus but not deselect (same for sidebar inspector).

Ok, so we shouldn't leverage onClickOutside for this but maybe onClick on the canvas

Should be fixed now in aba596f

Hm 🤔 Making a selection doesn't seem like the sort of action you take when you want to insert something. If anything, that would mean, delete the selection and insert something instead, but then I'd rather have it explicitly separate.

The inspector toggle is currently hidden when the multi-select header takes over, not sure if we want to keep something there, but both the inserter and inspector would then be actions tied to multi-selection and would not remove the selection on click.

To me is seems better that clicking or moving focus anywhere outside the UI that has to do with multi-selection should deselect.

I'd think of the multi-select header as having tabIndex="-1" and being some weird kind of modal. It would contain the focus, and if you click outside of it, or press escape, the selection is cleared, just like a modal would be closed. As long as the selection is there, backspace always deletes the selection content.

Does this need a decision somehow? I'm a bit confused on the details, but I'd like to help work through the flow if need be, and if someone can clarify the conflict.

Yep we definitely need a decision here. So for now on master, we're considering the browser selection (and focus) as separate from the selectedBlock and multiSelectedBlocks (which should probably be merged into one unique concept).

We're doing so because we want to be able to open the inserter and the sidebar without losing the selection (note that clicking buttons moves the focus to the button).

But multi-selection allows us to delete the selected blocks by hitting "backspace" which has no sense if the selection is not focused (backspace should work on the focused input or text area ...)

also Ella is saying

The inspector toggle is currently hidden when the multi-select header takes over

This was not the case when this PR was merged.

So should we tie the selection to the focus which means clicking anywhere deselect everything or should we find a middle-ground where we do so only for multi-selection?

This is complex, and I think I'm not getting all the nuance, so please correct me if I'm wrong here — it seems like we have two types of selection:

selection when you click the block

selection when a block is selected via multi select

1 is important for the inspector, as we need to be able to adjust parameters there without deselecting the block. So far so good.

But multi-selection allows us to delete the selected blocks by hitting "backspace" which has no sense if the selection is not focused (backspace should work on the focused input or text area ...)

I feel very daft here, the behavior in master feels pretty okay with me:

I select across blocks, press backspace, and the blocks are deleted

I select ... oh I think I see what you're getting at.

If I click inside a text block and press backspace, I delete text. If I then click on the border, the text is unfocused and it seems like the block is focused. Then backspace deletes the block. This is especially important, I think, for the bigger blocks, like image, where you almost always select the block.

If I understand you correctly, the big challenge here is that there's confusion between selecting blocks, setting focus on text, and selecting across blocks, correct?

It seems like you're saying selecting a block (clicking on the border), should highlight the block the same way as multi select does, because it's technically almost the same? Let's imagine what would happen if we did that — would this be desirable or not? Hmm. Thinking.

mtias · 2017-06-02T13:13:09Z

This is working well for me. Let's merge.

We may want to tweak the appearance of the selected block when hovering within the inserter (maybe fainter outlines, maybe hiding the block toolbar) as further explorations. @jasmussen do you have any ideas for adding clarity?

aduth

Let's remove the stopPropagation introduced in #836 .

aduth · 2017-06-02T13:09:39Z

editor/modes/visual-editor/index.js

+	}
+
+	onClick( event ) {
+		if ( event.target === this.container ) {


Between this and #836, I think I like this approach better. But it also means we probably don't need the stopPropagation anymore.

aduth · 2017-06-02T13:13:33Z

editor/inserter/menu.js

@@ -68,10 +68,15 @@ class InserterMenu extends wp.element.Component {
 	}

 	hoverBlock() {
+		const { lastMultiSelectedBlock, selectedBlock } = this.props;
+		let insertionPoint = null;


In a subsequent pull request, we should think about consolidating or simplifying some of this shared logic between the menu and the root component. I still think it's a bit bizarre how we assign insertion point in the hover intent, but IIRC the reasoning was to show the indicator in the post.

Yes, right! Actually, I thought about passing a prop from the inserter to themenu to void recomputing.

aduth

Conflicts introduced by #992 but otherwise LGTM 👍

mtias · 2017-06-02T13:24:45Z

Hah, sorry :)

jasmussen · 2017-06-05T07:48:17Z

We may want to tweak the appearance of the selected block when hovering within the inserter (maybe fainter outlines, maybe hiding the block toolbar) as further explorations. @jasmussen do you have any ideas for adding clarity?

@mtias not sure what the purpose is. The inserter insertertion indicator feels pretty good to me. But I could be missing something?

youknowriad added the [Type] Bug An existing feature does not function as intended label Jun 2, 2017

youknowriad self-assigned this Jun 2, 2017

youknowriad requested review from mtias, jasmussen and ellatrix June 2, 2017 11:08

mtias reviewed Jun 2, 2017

View reviewed changes

youknowriad force-pushed the fix/insertion-point branch 2 times, most recently from 02b9fdf to a10a8b8 Compare June 2, 2017 12:34

aduth requested changes Jun 2, 2017

View reviewed changes

aduth approved these changes Jun 2, 2017

View reviewed changes

youknowriad added 5 commits June 2, 2017 14:23

Inserter: Fix the insertion point styling

ecbee44

Inserter: Avoid clearing the selection if we click outside the editor

1df84c7

Inserter: Allow inserting blocks after multi-selection

2186d41

Blocks: Clear selection when clicking on the canvas

a2e90db

Blocks: drop stopPropagation when clicking on blocks

3d16e47

youknowriad force-pushed the fix/insertion-point branch from 209842c to 3d16e47 Compare June 2, 2017 13:27

youknowriad merged commit d874d3c into master Jun 2, 2017

youknowriad deleted the fix/insertion-point branch June 2, 2017 13:32

snyk-bot mentioned this pull request Apr 25, 2023

[Snyk] Fix for 1 vulnerabilities MaxMood96/gutenberg#95

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inserter: Fix the insertion point #994

Inserter: Fix the insertion point #994

youknowriad commented Jun 2, 2017

jasmussen commented Jun 2, 2017

mtias Jun 2, 2017

youknowriad Jun 2, 2017

mtias Jun 2, 2017

youknowriad Jun 2, 2017

youknowriad Jun 2, 2017

ellatrix Jun 4, 2017 •

edited

Loading

ellatrix Jun 4, 2017

jasmussen Jun 5, 2017

youknowriad Jun 5, 2017

jasmussen Jun 5, 2017

mtias commented Jun 2, 2017

aduth left a comment

aduth Jun 2, 2017

youknowriad Jun 2, 2017

aduth Jun 2, 2017 •

edited

Loading

youknowriad Jun 2, 2017 •

edited

Loading

aduth left a comment

mtias commented Jun 2, 2017

jasmussen commented Jun 5, 2017

Inserter: Fix the insertion point #994

Inserter: Fix the insertion point #994

Conversation

youknowriad commented Jun 2, 2017

jasmussen commented Jun 2, 2017

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

ellatrix Jun 4, 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

mtias commented Jun 2, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

youknowriad Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

mtias commented Jun 2, 2017

jasmussen commented Jun 5, 2017

ellatrix Jun 4, 2017 •

edited

Loading

aduth Jun 2, 2017 •

edited

Loading

youknowriad Jun 2, 2017 •

edited

Loading