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

Lighter block DOM: contextual toolbar in popover #18779

Merged
merged 10 commits into from Dec 23, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 27, 2019

Description

Edit: I decided to break this down in more steps, so this PR now only focuses on the contextual toolbar.

Fixes #7962.

This PR tries to use the Popover component for the contextual toolbar and breadcrumb (navigation mode).

Why?

We already use Popover for a lot of UI (dropdown, inline toolbar, tooltips...), and the component continues to get better. For this PR, we'll need to make some adjustments to it so that the controls position correctly.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@jasmussen
Copy link
Contributor

I love the purpose of this PR. Outside of making the DOM simpler (which vastly simplifies block editor block styling), it solves numerous issues where the toolbar is cropped in nested contexts, or just weirdly offset to the right. This is not counting all the other benefits you mention.

If we could get the PR to a state where basically everything looks the same in the branch as it does in master, just with cleaner DOM, this would be great to ship.

Future improvements could inform efforts in #18667.

@ellatrix
Copy link
Member Author

ellatrix commented Nov 27, 2019

Right, I'll work on making everything look the same.

  • I need to adjust Popover with some more positioning options.
  • Make sure Popover repositions smoothly on scroll. Fixed
  • Test on mobile and different screen sizes.
  • Use Popover for the insertion points and buttons.

@jasmussen
Copy link
Contributor

It does not necessarily need to be 100% accurate, and please let me know if you'd like me to do some of the heavy CSS lifting.

@ellatrix
Copy link
Member Author

@jasmussen Thanks! I'm pretty sure I'll need your help to clean up the CSS. I'll let you know when it's otherwise done.

@ZebulanStanphill
Copy link
Member

Nice! One possible benefit of this change is that it could make #7962 easier to fix.

Some things I've noticed on this branch (I know it's WIP; just posting to point them out):

  • Drag-and-drop is broken.
  • When scrolling, the block toolbar lags behind slightly. I'm not sure if there's any way to fix this without reducing performance, and it isn't really that important, but I do kind of miss the buttery smooth movement of the current toolbar.
  • Similar to the previous issue: when changing selection from a block that has a short block inspector to another block that has an inspector tall enough to create a scroll bar, the block toolbar appears and then a moment later shifts to the left slightly due to the scroll bar shrinking the editor canvas slightly.

@ellatrix ellatrix changed the base branch from try/popover-parameters to master November 27, 2019 19:47
@ellatrix ellatrix force-pushed the try/block-controls-popover branch 3 times, most recently from e522411 to ef48862 Compare November 27, 2019 21:55
@ellatrix
Copy link
Member Author

I fixed drag and drop of blocks.
The scrolling issues can be fixed.

@jasmussen
Copy link
Contributor

What a journey!

First:

The only thing I'm not sure if I can get to work is drag and drop, because the drag handle is in a popover. If it were the block itself it would be easier.

... and then shortly after:

I fixed drag and drop of blocks.

You should get a medal. 🥇

@ellatrix
Copy link
Member Author

@jasmussen Turned out one line was blocking drag 😅

@ellatrix ellatrix mentioned this pull request Dec 20, 2019
6 tasks
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Let's try it. I'm still not very confident with the SlotFill changes. I'd rather find a way to avoid these.

@ellatrix
Copy link
Member Author

There's a small remaining glitch with the toolbar that only happens in Chrome in some conditions. It happens when the content area is an odd number of pixels and as a result, in Chrome only, the reported block position and actual position have a difference of half a pixel, causing the toolbar position to be off by half a pixel.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 23, 2019

Okay, I took everything for a last round of testing.

I ensured that:

  • The toolbar is rendered and positioned correctly for:
    • All alignments (none, left, right, wide, and full)
    • Multi-selection
    • Nested blocks
    • Different screen sizes
    • RTL
    • Chrome, Safari, Firefox, Edge and IE11
  • It doesn't affect the mobile block toolbar
  • Moving blocks happens smoothly
  • Scrolling is smooth
  • Popovers anchored within the toolbar are positioned correctly
  • Other popovers have no regressions
  • You can drag and drop a block
  • You can tab in and out of the toolbar
  • Alt+F10 works

If I missed anything, I’ll fix it in a follow-up PR.

To recap, this PR puts the block controls (contextual toolbar and movers) in a popover.

  • It fixes sticky positioning for multiple selected blocks.
  • It reduces the amount of CSS needed to position the toolbar.
  • It allows us to position the toolbar better for narrow blocks. I have not done this yet as the movers still visually appear at the side of the block.
  • It’s a big step towards lightening the block DOM, which eventually will help theme authors, remove our alignment hacks, and could allow us to nest blocks that need a semantic relationship.

This PR introduces some unstable props on Popover, allowing us to figure out how to best stabilise this as the use of Popover grows. This PR has already brought many fixes to Popover benefitting all uses elsewhere.

@ellatrix ellatrix merged commit c73b6b7 into master Dec 23, 2019
@ellatrix ellatrix deleted the try/block-controls-popover branch December 23, 2019 15:24
@chrisvanpatten
Copy link
Member

Congrats on getting this merged in @ellatrix — super excited to see what kinds of improvements this unlocks! 👏

}

.block-editor-block-contextual-toolbar[data-align="full"] {
margin-left: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ellatrix do you know why we need this? With G2 it's causing a bug (can't click the movers) but I'm not sure if it's needed for something else.

@aduth
Copy link
Member

One of the unfortunate consequences of this change is that attempting to scroll the page while the cursor is atop the block toolbar will not scroll the block list. It's because the DOM hierarchy of the toolbar is no longer the same as the block list.

In others instances, it might be the sort of thing we could use pointer-events: none to allow the scroll event to occur on whatever is behind the element. However, this would of course conflict with how the cursor should interact with toolbar items.

If I recall correctly, this implementation had customized where the slot rendered in the page. Is that right? Is it the sort of thing where we can be more intentional with rendering it as part of the scrollable block list, in order to address the issue?

@ellatrix
Copy link
Member Author

ellatrix commented Jun 3, 2020

It's intentionally outside the scrollable container, so maybe in the future it can be unified with the top toolbar and so maybe the scrollable container can be iframed. Of course it's not nice that you cannot scroll while over a toolbar... :/ I can't immediately think of a solution.

@afercia
Copy link
Contributor

Worth also noting that since the introduction of the first "popover" implementation, the accessibility team feedback pointed out it's a pattern that's far from ideal for accessibility.

We always pointed out that parts of the UI that "appear" after some user action should be in close proximity in the DOM with the control that was activated. For example: click a button, a menu opens, the menu should be immediately after the button in the DOM.

Keeping moving important UIs far, far, away in the DOM is far from ideal for accessibility, even if focus is programmatically handled.

Add to that this is also breaking a native, fundamental, browser feature like scrolling a page and it is pretty obvious any accessibility specialist can't support this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block formatting toolbar goes off-screen when you scroll down past the first block in multiple block selection
10 participants