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

Try fixing the sibling inserter #7525

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

This PR partially fixes #7508.

In #7220 we introduced a new behavior for the sibling inserter, which requires a click on the plus in the center as opposed to just clicking between two blocks.

Turns out it was sort of a race condition between onClick and onMouseDown, the latter which fires first. So in a way, the Firefox and Safari behavior of selecting the block (which is selected onMouseDown) as opposed to clicking the sibling inserter was the correct one.

This PR "fixes" it by also making the sibling inserter use onMouseDown. But it's still labelled a try branch, as I'm not sure this is a kosher approach. If it is, we need to fix a regression this branch introduces on its own, which is that it doesn't set focus properly on the newly inserted block.

Thoughts?

@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Status] In Progress Tracking issues with work in progress labels Jun 25, 2018
@jasmussen jasmussen added this to the 3.2 milestone Jun 25, 2018
@jasmussen jasmussen self-assigned this Jun 25, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying this locally now...

@@ -21,7 +21,7 @@ class BlockInsertionPoint extends Component {

this.onBlurInserter = this.onBlurInserter.bind( this );
this.onFocusInserter = this.onFocusInserter.bind( this );
this.onClick = this.onClick.bind( this );
this.onMouseDown = this.onMouseDown.bind( this );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My immediate thought is: how does this work with a keyboard user? I forget if mouseDown is fired for keyboard events, but I feel like it wouldn't be. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, interesting. This doesn't work with a keyboard, but click does. (ノ°Д°)ノ︵ ┻━┻

So, in lieu of that — do you have any other ideas on how to address this? I.e. is there a way to prevent the onClick on the sibling inserter to propogate into a mouseDown event also?

I also have a plan B idea, but worth exploring the above first it seems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to do to fix this exactly, but I know changing the render() method in editor/components/block-list/insertion-point.js to:

// editor/components/block-list/insertion-point.js
render() {
	const { isInserterFocused } = this.state;
	const { showInsertionPoint, showInserter } = this.props;

	return (
		<div className="editor-block-list__insertion-point">
			{ true && <div className="editor-block-list__insertion-point-indicator" /> }
			{ true && (
				<div className={ classnames( 'editor-block-list__insertion-point-inserter', { 'is-visible': true } ) }>
					<IconButton
						icon="insert"
						className="editor-block-list__insertion-point-button"
						onClick={ this.onClick }
						label={ __( 'Insert block' ) }
						onFocus={ this.onFocusInserter }
						onBlur={ this.onBlurInserter }
					/>
				</div>
			) }
		</div>
	);
}

fixes it. I'll look through the event propagation chain but it's quite a maze! 😃

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We chatted in Slack about this a bit: https://wordpress.slack.com/archives/C02QB2JS7/p1529923657000479

Basically this approach isn't keyboard-accessible, and it seems in general there are a few issues with the sibling inserter in Firefox–maybe some need follow-up issues.

@jasmussen
Copy link
Contributor Author

👍 👍

Just to reiterate the desired behavior:

  • The sibling inserter exists between blocks
  • You should be able to click it or tab to it
  • The sibling inserter should never be covered by the block toolbar, so if the block toolbar is present, the sibling inserter shouldn't be

Looking at other options now.

@jasmussen jasmussen closed this Jun 25, 2018
@jasmussen jasmussen deleted the try/fix-sibling-inserter-safari-firefox branch June 25, 2018 11:20
jasmussen added a commit that referenced this pull request Jun 25, 2018
This PR partially fixes #7508. It is an alternate version of #7525, though much the same.

In #7220 we introduced a new behavior for the sibling inserter, which requires a click on the plus in the center as opposed to just clicking between two blocks.

Turns out it was sort of a race condition between onClick and onMouseDown, the latter which fires first. So in a way, the Firefox and Safari behavior of selecting the block (which is selected onMouseDown) as opposed to clicking the sibling inserter was the correct one.

This PR "fixes" it by also making the sibling inserter use onMouseDown. But in addition to this, it uses onClick as well, so it's still keyboardable.

The net effect is that both work, with the added benefit that in Firefox and Safari, the block that you're hovering isn't briefly "selected" when you're clicking.
jasmussen added a commit that referenced this pull request Jun 26, 2018
* Try another approach to fixing the sibling inserter in Firefox

This PR partially fixes #7508. It is an alternate version of #7525, though much the same.

In #7220 we introduced a new behavior for the sibling inserter, which requires a click on the plus in the center as opposed to just clicking between two blocks.

Turns out it was sort of a race condition between onClick and onMouseDown, the latter which fires first. So in a way, the Firefox and Safari behavior of selecting the block (which is selected onMouseDown) as opposed to clicking the sibling inserter was the correct one.

This PR "fixes" it by also making the sibling inserter use onMouseDown. But in addition to this, it uses onClick as well, so it's still keyboardable.

The net effect is that both work, with the added benefit that in Firefox and Safari, the block that you're hovering isn't briefly "selected" when you're clicking.

* Use the onClick handler for mouseDown

* Fix issues in Firefox and Safari

Turns out this issue has been present in some hidden form for a long time, and _partially fixed_ in Chrome. But there has always been a race condition, it looks like, between onClick, onFocus and onMouseDown.

This commit simply adds the "onFocusInserter" action to the onMouseDown event as well, which seems to solve the issue.

You still briefly see the block being selected and the toolbar appearing in Firefox and Safari, but at least this can work as a hotfix pending further investigation or improvements.

* Add comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sibiling Inserter doesn't insert in Firefox and Safari
2 participants