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

Editor: Ensure there is no blinking Toolbar when inserting blocks between lines #6276

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 19, 2018

Description

When testing other PRs I noticed a very subtle bug where Block Toolbar blinks after you click on the in-between line inserter. This happens when you reach a very specific area close to the top and bottom edges. See screenshots.

This PR fixes this issue by preventing propagation of focus event to the BlockContextualToolbar. I'm not convinced this is the best solution in the long run, so happy to update if you have a better idea how to fix it.

How has this been tested?

Manually

Screenshots

unexpected-toolbar-up
unexpected-toolbar

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Apr 19, 2018
@gziolo gziolo added this to the 2.8 milestone Apr 19, 2018
@gziolo gziolo self-assigned this Apr 19, 2018
@aduth
Copy link
Member

aduth commented Apr 19, 2018

Related: #5658 (comment)

Specifically also the latter note here:

Edit: One of the first things I tried was to stop propagation of the focus event handled by the button, which is itself not ideal (stopping propagation never is), and resulted in another strange effect of the hover effects of the block never being dismissed.

This is also why the clickAtRightish function exists in the adding-blocks end-to-end` test, and should be removed if it can be fixed:

/**
* Given a Puppeteer ElementHandle, clicks around the center-right point.
*
* TEMPORARY: This is a mild hack to work around a bug in the application
* which prevents clicking at center of the inserter, due to conflicting
* overlap of focused block contextual toolbar.
*
* @see Puppeteer.ElementHandle#click
*
* @link https://github.com/WordPress/gutenberg/pull/5658#issuecomment-376943568
*
* @param {Puppeteer.ElementHandle} elementHandle Element handle.
*
* @return {Promise} Promise resolving when element clicked.
*/
async function clickAtRightish( elementHandle ) {
await elementHandle._scrollIntoViewIfNeeded();
const box = await elementHandle._assertBoundingBox();
const x = box.x + ( box.width * 0.75 );
const y = box.y + ( box.height / 2 );
return page.mouse.click( x, y );
}

@gziolo
Copy link
Member Author

gziolo commented Apr 23, 2018

@youknowriad, what was the reason why we moved BlockInsertionPoint inside BlocksListBlock component? It seems like this is the root cause why BlockToolbar gets focus in the first place.

@youknowriad
Copy link
Contributor

@gziolo It was moved their to be shown properly for floated blocks (relative to the block's position)

@aduth
Copy link
Member

aduth commented Apr 25, 2018

I'm not inclined to add even more nested nodes to what already exists for a block, but one option is to apply the floating styles as a separate wrapper element which is not susceptible to the block's onFocus handling, where the block inserter would be a child to take advantage of the float, but not incur the block selection.

(There are some particular intricacies of this with wrapper tabIndex, getBlockFocusableWrapper)

@aduth
Copy link
Member

aduth commented Apr 25, 2018

There was also the idea of using the nested block's concept of layout for floated content. Last it was discussed, there was some various concerns expressed about how well it would cooperate with surrounding unfloated content, but off the top of my head it doesn't seem like any more of an issue than what exists today. The advantage here is that layout is one level above a block, which achieves a similar goal to the idea in #6276 (comment) (not having inserter be susceptible to block onFocus handling).

@aduth aduth modified the milestones: 2.8, 2.9 May 3, 2018
@gziolo gziolo modified the milestones: 2.9, 3.0 May 16, 2018
@gziolo
Copy link
Member Author

gziolo commented May 27, 2018

It's no longer an issue. There was a change introduced which completely refines the way this inserter works. You can click only on the plus icon. Closing this PR.

@gziolo gziolo closed this May 27, 2018
@gziolo gziolo deleted the fix/insertion-point-blink branch May 27, 2018 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants