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 another approach to fixing the sibling inserter in Firefox #7530

Merged
merged 4 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@jasmussen
Contributor

jasmussen commented 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.

GIF:

sibling

Note, though, that when clicking, text focus isn't set on the newly created sibling block. This is something we'll want to fix before this can be merged.

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.

@jasmussen jasmussen requested a review from tofumatt Jun 25, 2018

@tofumatt

This could be DRY'd up, but since I'll take a shot at getting the focus to work I'll just do that ^_^

@@ -63,6 +74,7 @@ class BlockInsertionPoint extends Component {
icon="insert"
className="editor-block-list__insertion-point-button"
onClick={ this.onClick }
onMouseDown={ this.onMouseDown }

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

I would just reference this.onClick here (maybe rename it to insertNewBlock or whatever) rather than duplicating the handler's code.

This comment has been minimized.

@jasmussen

jasmussen Jun 25, 2018

Contributor

Feel free to commandeer the PR and push directly to it. ❤️

@jasmussen jasmussen requested review from youknowriad and aduth Jun 25, 2018

@youknowriad

LGTM 👍

Maybe we can add a comment clarifying why we need this duplication. Also, do you have any idea if we can replace the parent onMouseDown event with onClick? This could be more impactful but I wonder why we're using onMouseDown in the first place.

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.
@youknowriad

Just noticed the breakage in the e2e tests, we might want to take a look at those ;)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 26, 2018

Thanks for the fast review, and will look at tests in a second. Just wanted to clarify that I just pushed a change in 743c219 which changes it so the onFocusInserter is attached to onMouseDown as well, and this fixes the issue for Firefox and Safari, without us losing focus. Behold:

safari

The issue where the block toolbar briefly appears is still there, but this is an improvement in that it now works as intended.

Edit: It's also fixed in Firefox, but I figured I'd only record one GIF.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 26, 2018

Okay I think the latest two commits might have fixed the e2e tests. Final review?

The issues have been addressed, by yourself even :D

@jasmussen jasmussen merged commit 2043b11 into master Jun 26, 2018

2 checks passed

codecov/project 46.76% (-0.04%) compared to bf69697
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/alternate-sibling-inserter branch Jun 26, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 26, 2018

🎉

Thanks everyone! Good to have this in.

oxyc added a commit to generoi/gutenberg that referenced this pull request Jun 26, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg
* 'master' of https://github.com/WordPress/gutenberg: (69 commits)
  fix: Show permalink editor in editor (WordPress#7494)
  Fix text wrapping in Firefox. (WordPress#7472)
  Try another approach to fixing the sibling inserter in Firefox (WordPress#7530)
  fix: Improve "add block" text in NUX onboarding (WordPress#7511)
  Implement core style of including revisions data on Post response (WordPress#7495)
  Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284)
  Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877)
  Only save metaboxes when it's not an autosave (WordPress#7502)
  Fix broken links in documentation (WordPress#7532)
  Remove post type 'viewable' compatibility shim (WordPress#7496)
  Fix typo. (WordPress#7528)
  Blocks: Remove wrapping div from paragraph block (WordPress#7477)
  fix: change import for InnerBlocks (WordPress#7484)
  Polish library just a teeeeensy bit (WordPress#7522)
  feat: Add snapshot update script (WordPress#7514)
  Display server error message when one exists (WordPress#7434)
  Fix issues with gallery in IE11. (WordPress#7465)
  Polish region focus style (WordPress#7459)
  Fix IE11 formatting toolbar visibility (WordPress#7413)
  Update plugin version to 3.1. (WordPress#7402)
  ...
@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

I don't think the changes to package-lock.json were intended here? I see local changes anytime I run npm install.

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

See #7641

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

This is purely a styling issue: #7642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment