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 delaying sibling inserter hover events #17240

Closed
wants to merge 6 commits into from

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Aug 28, 2019

Closes #16646.

This picks up on an idea @mapk had: To have the sibling inserter display after a short delay. This allows for users to select controls that would otherwise be blocked by the inserter. For example, in the GIF below you'll see me resizing the spacer block. But if I stop and wait for 0.75s, it'll bring up the sibling inserter instead.

mover

In #17136, @mapk noted some issues around implementing this:

I wanted to try a delay on the Inserter, but animating the display attribute isn't possible with just CSS. I think that's why the Inserter uses the opacity attribute instead, but opacity still takes up space even when opacity is set to 0. This results in an element that still covers other UI elements.

Yeah. 😞 Also, we can't set display: none or visibility: hidden here because we want the sibling inserter to be selectable by keyboard users. That's the reason why it's technically always visible today.

To get around all that, I adopted a sort of weird approach:

  • I applied a delayed fade animation to the inserter parent.
  • I set the button itself to have pointer-events: none, so it ignores clicks when it's hidden, but is still focusable via the keyboard.
  • I added a second delayed animation to the button itself, which just activates pointer-events: auto after the same delay used for the parent's animation.

A couple caveats:

  • I've got this working in Chrome + FF on my end, but Safari won't let me click on the button after it fades into view.
  • I haven't tested in IE. It should work there (or at least, fall back to something like its prior behavior).

This is most definitely a hack approach, and I think we could probably come up with something better that uses javascript? But I figured I'd share anyway in case it helps nudge us along, or in case we're able to sort out the issues it has in Safari.

@kjellr kjellr added General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. labels Aug 28, 2019
@kjellr kjellr self-assigned this Aug 28, 2019
@kjellr kjellr closed this Aug 28, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Aug 28, 2019

(Whoops)

@kjellr kjellr reopened this Aug 28, 2019
@kjellr kjellr requested review from jasmussen and mapk August 28, 2019 16:33
@jasmussen
Copy link
Contributor

Love this, so good:

delay

This feels like a VASTLY better experience. Not just because it solves the problem of the resizer, but because it also makes the UI less jumpy and heavy. Two upsides that in my opinion strongly outweigh the theoretical extra time you have to wait if you only want to insert a block using the sibling inserter.

Now you got me looking though, and I noticed this:

Screenshot 2019-08-29 at 09 12 23

☝️ I used the inspector to make sure the on-select borders were always visible for all blocks. And in doing so it's clear that the sibling inserter isn't centered between two blocks. Can it be?

@@ -879,7 +880,17 @@

&:hover,
&.is-visible {
opacity: 1;
// Only show this inserter after a delay.
animation: block-editor-block-list__insertion-point-inserter-fade-in 0.2s ease-out 0.75s;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is ok, but is there a specific reason you’ve used animation rather than transition and transition-delay? Seems like you could accomplish the same thing with less code using transition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Hmm. Yeah — my first inclination was to use that mixin we have, but I bet this would work too (and maybe fix whatever weird bug is preventing the clicks from registering in Safari?). I'll give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the "reduce motion" thing suggest either approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually — trying this out now: the main issue is that we only want to apply that delay when this animates in. We still want it to fade out as soon as you mouse out.

Transition will apply to both in and out, which means the button will stick around for an extra second after the cursor leaves it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can apply different transition-delay to the base selector and the hover. That should do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch that — I got this working in db9d48a (after a quick DM with @shaunandrews. 😄).

The only caveat is that the pointer-events animation didn't work when I tried to move it to a transition instead. I kept that as an animation for now, but it still only works in Chrome and Firefox. I'd guess that pointer-events isn't generally the sort of property that's supposed to be animated, so its behavior isn't quite the same in each browser. 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd guess that pointer-events isn't generally the sort of property that's supposed to be animated

Exactly. Animating pointer-events is a hack, and not a reliable solution. This is a tough one to solve with CSS alone. I'd imagine some JS will be required to handle the delay and trigger the state change.

You might be able to rethink the way you're doing the transition to use a parent element to trigger the delay, which might fix the issue with pointer-events — but JS might be more succinct.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 29, 2019

I used the inspector to make sure the on-select borders were always visible for all blocks. And in doing so it's clear that the sibling inserter isn't centered between two blocks. Can it be?

Fixed in 7ee67d7.

Screen Shot 2019-08-29 at 9 51 32 AM

@talldan talldan added the Needs Accessibility Feedback Need input from accessibility label Aug 30, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Aug 30, 2019

I went down a rabbit hole with this one yesterday afternoon, and hope to spend a little time on it today with fresh eyes.

A general update:

  • The delayed hover appearance/functionality of the sibling inserter seems to be well received above.
  • As per discussions above, CSS-only may not be the best route for this one. I have a mostly working JS version that I'll push up if I can sort out the bugs.
  • Either way, we'll certainly make sure that the button is focusable via the keyboard, and appears immediately when it is focused.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 30, 2019

Alright. I've had lots of time to dig in, and even though I thought I was closer, I hit a wall and it turns out I'm even farther from a solution than originally thought. 😅

These inserters have 4 levels:

  1. First, there's a parent div, named editor-block-list__insertion-point
  2. Then there's a child div, called block-editor-block-list__insertion-point-inserter
  3. Then there's a child of that called block-editor-inserter
  4. Finally, inside of that of that is the button, called block-editor-inserter__toggle

All of these levels slightly overlap the preceding block. So in this screenshot, the insertion point area that's highlighted belongs to the paragraph block:

Screen Shot 2019-08-30 at 9 39 58 AM

That's what we all end up clicking when we reach for the movers. Even if we were to rip out all of the levels below that, the top parent would still be blocking our clicks.

I'd been operating under the impression that the final level (the button itself) was the element that was intercepting hovers + clicks. But that's not the case! All of those levels sit on top of the preceding block, so all of them block hovers/clicks.

We'd actually need to disable pointer events on that top parent div (and all children) in order to still use the resize controller like normal. But that won't work either: if we turn off pointer events on the parent, then we'll have nothing hover on, which means we'll never end up showing the toggle button in the first place. I haven't yet found a decent solution (css or js) that allows us to let clicks through, but will still trigger the hovers. I've tried some workarounds, but they usually result in two competing hover states firing off, which isn't great:

uh-oh

I'm open to solutions to that if anyone has them! But in the meantime I think this PR is likely dead. 😞 To solve the overlap issue, I think the only simple solution would be to narrow the height of the inserter area (the one in that screenshot above), so that it interferes less with the resize controller. Unfortunately, that'd mean that mouse users would have to be a bit more fiddly when they want to activate that inserter: they'd end up with a smaller hover area.

Anyway — if nobody has any good suggestions, I think we're good to close this PR in the meantime.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 30, 2019

cc @youknowriad or @ellatrix since you two seem to have lots of creative solutions to these sorts of things. 🙂

In general: we'd like to stop the sibling inserter from blocking clicks, but still keep it hoverable in some way — so that if you were to leave your mouse in between blocks, we would still surface the inserter after a little delay.

@ZebulanStanphill
Copy link
Member

#17136 just got merged, which sort of "fixes" the issue where you can't reach the drag handle.

Ultimately, I think the sibling inserter should never overlap the block that you are editing. It should appear outside of the block's borders. I made #13571, which suggests an alternative design.

@mapk
Copy link
Contributor

mapk commented Sep 5, 2019

Anyway — if nobody has any good suggestions, I think we're good to close this PR in the meantime.

So frustrating! Sorry about that, Kjell.

@ZebulanStanphill has an interesting solution that I think may be worth looking into. My favorite part is moving the sibling inserter below the block rather than above it. Could that help resolve some of the overlap? I realize it would overlap in a different way, but it might be in a more intuitive way.

@paaljoachim
Copy link
Contributor

paaljoachim commented Sep 18, 2019

A spontaneous thought.:)
What if the spacer block has bottom margin built into the backend block so that the resizer would not be covered up by the inserter for the next block?

@kjellr
Copy link
Contributor Author

kjellr commented Sep 24, 2019

This one seems blocked for now — I'm going to close, and we can open a new PR if we find a JS-based solution that works for us.

@kjellr kjellr closed this Sep 24, 2019
@youknowriad youknowriad deleted the try/block-mover-appear-on-delay branch September 25, 2019 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spacer block: Can't grab the block's height adjuster
7 participants