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

Blocks: Don't show the block toolbar if we press ctrl, shift... on Safari #1365

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

youknowriad
Copy link
Contributor

closes #1349

It appears that safari trigger an "mousemove" event when we click on the meta keys (shift, ctrl...) even if the mouse didn't move. Some details on the weird mousemove event in Safari here https://transitory.technology/mouse-move-in-safari/

The workaround here is to check that the mouse position effectively changed before showing the toolbar.

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Jun 22, 2017
@youknowriad youknowriad self-assigned this Jun 22, 2017
@youknowriad youknowriad requested review from mtias and aduth June 22, 2017 12:29
@mtias
Copy link
Member

mtias commented Jun 22, 2017

Lovely Safari.

@youknowriad youknowriad force-pushed the fix/safari-toolbar-flickering branch from 51a7cdd to 9765b32 Compare June 22, 2017 12:49
@ellatrix
Copy link
Member

Where is the logic that normally prevents it for meta keys?

@youknowriad
Copy link
Contributor Author

@iseulde We just check the mouse position compared to the previous mouse position. that way, we ensure we're effectively moving the mouse

@ellatrix
Copy link
Member

@youknowriad No I mean, in other browsers, currently in master, what prevents the UI from showing up on shift or ctrl etc.?

@youknowriad
Copy link
Contributor Author

On other browsers this event is not triggered at all when we hit shift, ctrl...

@ellatrix
Copy link
Member

Ok, I see. Allow me to quickly see if there's no other solution.

@ellatrix
Copy link
Member

You're right, I thought maybe we could prevent default behaviour somehow.

this.props.onStopTyping();
this.lastClientX = event.clientX;
this.lastClientY = event.clientY;
Copy link
Member

@ellatrix ellatrix Jun 22, 2017

Choose a reason for hiding this comment

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

How about this to avoid duplicate assignment?

stopTypingOnMouseMove( { clientX, clientY } ) {
	const { lastClientX, lastClientY } = this;

	// We need to check that the mouse really moved
	// Because Safari trigger mousemove event when we press shift, ctrl...
	if ( lastClientX && lastClientY && ( lastClientX !== clientX || lastClientY !== clientY ) ) {
		this.props.onStopTyping();
	}

	this.lastClientX = clientX;
	this.lastClientY = clientY;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM updating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolbar appears when shift is pressed in Safari on macOS
3 participants