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 G2: Mover interactions #20056

Merged
merged 7 commits into from
Feb 19, 2020
Merged

Try G2: Mover interactions #20056

merged 7 commits into from
Feb 19, 2020

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Feb 5, 2020

This (draft) PR aims to improve the interactions of the Mover for the G2 redesigned Toolbar.

Demo of initial attempt:

https://d.pr/v/gqcVs7

(Note: This update does not change how the drag/reorder UX works. It only focuses on the Toolbar Mover bit)

Interactions

The current implementation works like the following:

Show mover when:

  • Mouse enters/moves anywhere within the Toolbar
  • Focus is on any element within the Toolbar

Hide mover when:

  • Mouse leaves the Toolbar, after a specified debounced time (currently 300ms)
  • Focus jumps outside the Toolbar, and your mouse is not within the Toolbar

(Current) Gotchas:

  • Border radius of Toolbar container needs to be adjusted to work with independent Mover UI
  • Show/Hide logic is unaware of Dropdown item interactions (since the elements are Portal, and are technically not child elements of Toolbar)

Feedback

@youknowriad / @jasmussen Let me know how this feels! The code is definitely not final. I coded enough to test out this interaction, with as little mess as possible.

@ItsJonQ ItsJonQ added the Needs Design Feedback Needs general design feedback. label Feb 5, 2020
@ItsJonQ ItsJonQ self-assigned this Feb 5, 2020
@ItsJonQ ItsJonQ mentioned this pull request Feb 5, 2020
9 tasks
@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 5, 2020

Screen Shot 2020-02-05 at 16 33 23

Interaction question! Should the mover show if you move your mouse to the spot where they will appear?

Scenario:

  • Hover into a block
  • Toolbar shows (no mover)
  • Move mouse into where mover would show (without touching the Toolbar)
  • Show mover?

I suspect yes. The interaction pattern is similar to improved Dropdown movement paths.

P.S. I suspect this will be a bit tricky. Not impossible... but not super easy either, haha.

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 5, 2020

Additional thoughts! This is for future, but I think we should support a way to "always show movers". From my initial prototyping, @diegohaz left a great bit of feedback re: mouse movements being tricky for some folks (from an a11y perspective)

@mtias
Copy link
Member

mtias commented Feb 6, 2020

It'd be interesting to try showing it not when hovering the toolbar in general, but when hovering the block type button instead.

Should the mover show if you move your mouse to the spot where they will appear?

Yes, let's try it. We used to have a "left area" of the block that would trigger showing the movers. It was slightly convoluted if I remember correctly.

Show/Hide logic is unaware of Dropdown item interactions

If the version that only shows it when hovering the block type works, then this wouldn't be a concern since interacting with the rest of thee toolbar would minimize it.

@jasmussen
Copy link
Contributor

Thank you for working on this! This feels like a promising effort to find the balance between showing the movers when you need them, yet not weighing down the interface when you don't. I would echo Matías comment about just hovring the block indicator.

One more thing: can you work in the -48px offset so that the little triangle pointer now points correctly to the top left corner of the block footprint?

Like so:

Screenshot 2020-02-06 at 09 33 13

The tricky bit here is that it needs to still work on fullwide blocks, i.e. the positioning magic that happens to the detached toolbar needs to work. Speaking of, the movers are cropped off there now:

Screenshot 2020-02-06 at 09 35 45

So the "reserving of space" needs to be a little stronger there.

@ZebulanStanphill
Copy link
Member

Putting the mover so far away from the block makes drag-and-drop feel weirder, because where you drag your mouse isn't where you're going to drop the block.

@jasmussen
Copy link
Contributor

I think that's a fair point, but one that touches on drag and drop beyond just where the handle is. Or to put it differently, the issue is the same as what's shipping in master, just slightly exacerbated due to the longer distance.

I know various tickets have outlined grand dreams for a drag and drop refactor that makes drag and drop actually real time, and I see no reason those couldn't address that. More specifically, drag and drop right now isnt' very good... the block that's being moved is a semitransparent clone and it gets muddy — it might be interesting to actually pull the block out of the flow and puts it under the cursor.

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 6, 2020

Thanks for feedback all! I'm gonna fix the conflicts between this and try/g2 and apply some of the suggestions above 🙏

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 6, 2020

@mtias + @jasmussen 👋 Haiii!!

I just pushed an update!

  • -48px left offset has been added back
  • Mover shows when mousing over the Block type button
  • Mover shows when mousing over Mover area
  • Mover hides after mousing out of Block type button/Mover area after debounced time

Here's a video demo:
https://d.pr/v/dTSbS8

Considerations:

  • How would you folks handle tabbing? Should the mover show when something in the Toolbar is focused?

The tricky bit here is that it needs to still work on fullwide blocks, i.e. the positioning magic that happens to the detached toolbar needs to work. Speaking of, the movers are cropped off there now:

@jasmussen Yes, we'll need to apply some positioning magic there :).
With the negative left offset of the Toolbar... and movers... and the Toolbar touching the edge... how do you imagine the UI looking?

(Ignoring code implementation)

If there's a mock for these various stages, that would be great~!!

Update: Just noticed... that the Mover needs to be visible always if any of the Up/Down arrows are focused.

I think outlining various scenarios when/how the mover shows/hides would be handy! It looks like there's several moving parts (🙃 )

@jasmussen
Copy link
Contributor

Nice work! This is coming along. This is what I see.
movers

As far as I'm concerned, the mouse behavior is now solid, nice work!

How would you folks handle tabbing? Should the mover show when something in the Toolbar is focused?

Yes. Totally fine for them to be present when anything in the toolbar has focus.

Yes, we'll need to apply some positioning magic there :).
With the negative left offset of the Toolbar... and movers... and the Toolbar touching the edge... how do you imagine the UI looking?

Like so:

Screenshot 2020-02-07 at 09 14 36

The space on the left is reserved for the mover popping out. You may want to pair with @ellatrix if she has time, she's done some of the "undocking" of the toolbar which currently aligns it towards the left of the screen.

But I imagine there might also be a simpler CSS trick for it. Imagine the collapsed toolbar is 100px wide, and offset -48px to the left on most blocks. If this toolbar was actually 148px wide, with the leftmost part just being an invisible slot available for the movers to slot into. The idea being that if the space is permanently reserved, then the JS positioning would be simpler — zero pixels when fullwide, -96px when there's space for the offset, and so on. Makes sense?

I think outlining various scenarios when/how the mover shows/hides would be handy! It looks like there's several moving parts (🙃 )

For this particular heuristic I'm imagining the following:

  • Press shift tab when focus is in a paragraph to tab into the toolbar. When anything inside it has focus, the mover control is showing.
  • Hover the "Block indicator" to show the moer control.
  • When focus is in the mover control, it obviously stays open.

The only challenge here is the "resting focus" effect where maybe you selected some text, clicked "Bold", and now the bold button has focus and therefore has focus. Effectively this could result in the mover popping in, in a jarring fashion, just because you click bold. It might be even more jarring if you click a dropdown menu item, which briefly receives focus before focus moves into the popover, causing the mover control to pop in and then out.

Two ways we can handle that:

The mover control appearing when pressing "Bold" is either a non-issue, (which I think it is), and we'll want to make sure that even focus inside a popover keeps the mover control visible.

Or it's an issue, in which case we could consider making it so the mover control pops out only when the block indicator is hovered, focused, or toggled.

@mtias
Copy link
Member

mtias commented Feb 7, 2020

Yes. Totally fine for them to be present when anything in the toolbar has focus.

This would be weird if you interact with unrelated controls. I think they should show if the block type has focus, since these controls are an extension of it.

@jasmussen
Copy link
Contributor

I touched briefly on that with the "bold" example later, and suggested if it doesn't feel right, we can do this:

Or it's an issue, in which case we could consider making it so the mover control pops out only when the block indicator is hovered, focused, or toggled.

What do you think?

@SchneiderSam
Copy link

Just tested with gutenberg.run/20056. Just cool. I can't wait to use it live...

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 7, 2020

This would be weird if you interact with unrelated controls. I think they should show if the block type has focus, since these controls are an extension of it.

For this particular heuristic I'm imagining the following:....

@mtias + @jasmussen Thanks for feedback! Will refine the focus x mouse interactions now.

After that, I can look into the repositioning/docking handling 💪

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 7, 2020

@mtias + @jasmussen Oh boy! Have some updates for you all :)

Demo Videos
https://d.pr/v/zRBtRX

Interactions

Mover show/hide rules are as follows:

Show mover when...

  • Either Mover or Block Switcher is hovered
  • Mover placeholder space is hovered
  • Mover arrows or Block Switcher is focused

Hide mover when...

  • Mouse leaves Mover and Block Switcher, and neither elements are focused
  • Mover or Block Switcher is blurred, and neither are hovered

(Note: All closing interactions are debounced, and are cancellable by any show interaction)

There was quite a bit of sequencing/logic to get the above interaction right.

Code

Code wise... I've refactored so that all of the interaction complexities are abstracted into this file:
https://github.com/WordPress/gutenberg/blob/97386bb7058ac7584b8f5ac940bbb6b6ee0870b3/packages/block-editor/src/components/block-toolbar/utils.js

The main BlockToolbar component interfaces with it via a single hook (useShowMoversGestures)

There are some adjustments made to the BlockToolbar component, but it's mostly styles related (that need refactoring)

Let me know how it feels!

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 7, 2020

Screen Shot 2020-02-07 at 3 53 50 PM

Todo: Fix for mobile 🙈

Will tackle that once we feel 👍 with the interactions!

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 7, 2020

@jasmussen Positioning update!!

I experimented to see if I could get a feel of what we may want. I pushed it up for testing.

Demo:
https://d.pr/v/oCNofc

Basically, the Toolbar will always have a space of 48px (Mover) + 8px (buffer, so it doesn't touch the edge*). By edge, I made the edge of the Editor -> Sidebar, not the screen.

This positioning logic fires on (browser) resize as well*

(Currently not supported for mobile)

This is a very VERY experimental (and hacky) implementation. I just did it for prototype/feels purposes.


Overall, I think the logic/mechanisms for repositioning will follow similar principles.

  • Account for x of self (e.g. Toolbar)
  • Account for xof container (e.g. Gutenberg Editor)
  • MATH those together
  • Reposition if required
  • Retrigger reposition calculation, either via a loop (e.g. requestAnimationFrame) or via window resize`

The internal Popover component uses these mechanics, with more MATH involved.


@ellatrix 👋 Haiii!! Maybe we can catch up on "undocking" things :). I'm unfamiliar with that one 😊 . If you're swamped, lemme know I'm on the right track with my thoughts above <3

@jasmussen
Copy link
Contributor

GIF of what I'm seeing:

movers

Nice. This feels really right to me. It feels like it finds that balance of the movers being there and in a contextual way, while still not weighing down the UI the rest of the time. It works even for full-wide.

This positioning logic fires on (browser) resize as well*

It did not seem to fire when applying the fullwide property:

fullwide

That's not world-endy, but would be nice if fixable.

Basically, the Toolbar will always have a space of 48px (Mover) + 8px (buffer, so it doesn't touch the edge*). By edge, I made the edge of the Editor -> Sidebar, not the screen.

Did you try making that 96px instead of 48? So 48px for the offset block indicator control, and another 48 for the mover control? I'm very possibly missing something, but I was hoping we could reserve that space, and use negative margins, so that no position outside of the usual JS positioning the toolbar does, is necessary.

Haiii!! Maybe we can catch up on "undocking" things :). I'm unfamiliar with that one 😊 . If you're swamped, lemme know I'm on the right track with my thoughts above <3

Regarding the overall positioning that's already sort of working but buggy when on the right side, so long as we know it can and should be fixed, it's okay to not fix it urgently or in this branch. It needs fixing, but because it's so purely a bugfix, it shouldn't block the other G2 efforts.

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 10, 2020

@jasmussen Haii! Thanks for the feedback :)

This feels really right to me

Great!

It did not seem to fire when applying the fullwide property:

The positioning mechanism isn't that smart yet. For the next round, I can make adjustments to make it more responsive to changes, be it block width changes, browser resize, etc...

Regarding the overall positioning that's already sort of working but buggy when on the right side

😱Do you have an example if it rendering on the right side? Is that rendered via RTL?

Did you try making that 96px instead of 48? So 48px for the offset block indicator control, and another 48 for the mover control

I'm not too sure what you mean 😊. I created a diagram (below) of the current measurements for offsets (block type, mover, and buffer).

Screen Shot 2020-02-10 at 9 06 25 AM

Does that feel okay?

❤️

@jasmussen
Copy link
Contributor

That does feel okay, and by 96px I meant what it looks like you're showing above, 48px for the block type, 48px for the mover. My idea being that if the space for that was permanently reserved inside the popover, like a left padding, then the toolbar plus padding would be positioned, so you wouldn't have to do any extra positioning math than what's already there.

😱Do you have an example if it rendering on the right side? Is that rendered via RTL?

It's this, and it's not unique to this branch nor even the G2 branch:

Screenshot 2020-02-10 at 15 18 28

The benefit of an "undocked toolbar" is exactly that in a situation like the above, the right-most side of the toolbar can dock itself to the sidebar instead of being cropped by it.

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 10, 2020

My idea being that if the space for that was permanently reserved inside the popover, like a left padding, then the toolbar plus padding would be positioned, so you wouldn't have to do any extra positioning math than what's already there.

I'll give it a shot!

the right-most side of the toolbar can dock itself to the sidebar instead of being cropped by it.

Gotcha!! I'll factor that into the repositioning calculations :)

Stay tuned!

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 10, 2020

@jasmussen Haiii! I just pushed up some changes to improve the positioning mechanism. It should feel "smarter" now. It accounts for any positional/overflow changes, and adjusts (horizontally) to match.

Example: Align type change from center -> full
Screen Shot 2020-02-10 at 10 55 08 AM

Example: Right offset
Screen Shot 2020-02-10 at 10 54 50 AM

The code needs to be refined, but I wanted to get your feedback first :).

...so you wouldn't have to do any extra positioning math than what's already there.

I tried leaning more on CSS... it didn't work (I wish). Will still need positioning MATH 😂

@jasmussen
Copy link
Contributor

This is what I see:

mover

Fullwidth:

Screenshot 2020-02-11 at 08 36 29

The particular interaction:

alignment

The behavior here feels correct, still. Nice work. Notice that the positioning of the toolbar jumps a little bit when switching alignments. Is it possible to avoid this jumpiness? Alternately, I wonder if it would feel less jarring if the toolbar itself received a small transition property, so the positioning would at least be smoothed out — it might read as the toolbar realigning itself to the new position.

I tried leaning more on CSS... it didn't work (I wish). Will still need positioning MATH 😂

Totally fine, I trust you and defer to you. This works well.

I honestly think, outside of any refinements you might have, this is worth an accessibility sanity check.

If you'll forgive the direct ping, @MarcoZehe, if you have bandwidth to look at this I would very much appreciate it. Specifically, does the positioning and the tab order behavior of the mover control feel correct? Thank you.

@jasmussen jasmussen removed the Needs Design Feedback Needs general design feedback. label Feb 11, 2020
@youknowriad
Copy link
Contributor

Oh, so I don't have an issue in 2020 theme but I have it with 2019. The weird thing is that I don't have with any theme in the G2 branch only.

@youknowriad
Copy link
Contributor

btw, I noticed an issue when hovering the switcher in posts with just one block (so no movers), a border appears floating on the left.

@youknowriad
Copy link
Contributor

I didn't find a fix yet for the height thing but I noticed that if we remove the "block-editor-block-toolbar__block-switcher-wrapper" wrapper, it's fixed.

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 18, 2020

@youknowriad Thanks for investigating! I'm checking it out now :)

You're the best 🏆

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 18, 2020

@youknowriad I fixed the issue.

Cause: There's something in the 2019 theme modifies line-height in a global-ish manner, which affected the Block Switcher (which was display: inline-block prior to my update).

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 18, 2020

Do you think we should remove the useExperimentalToolbarPositioning hook as it's not used anymore?

@youknowriad Good idea. I'm going to remove it now

@@ -424,6 +424,9 @@
/**
* Block Toolbar when contextual.
*/
.block-editor-block-contextual-toolbar-wrapper {
padding-left: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the toolbar width, I think we have a variable for that.

@jasmussen
Copy link
Contributor

This is looking really nice. But one thing regressed. Toolbars are straight above the block now:

Screenshot 2020-02-19 at 09 43 48

They should be offset so that the block switcher points to the top left of the toolbar, like so:

Screenshot 2020-02-19 at 09 45 05

@youknowriad
Copy link
Contributor

youknowriad commented Feb 19, 2020

This is looking really nice. But one thing regressed. Toolbars are straight above the block now:

This was done on purpose because ensuring there's enough room on the left is no easy task and something that should be handled in Popover instead. I was hoping we could do this later with some help from @ellatrix maybe.

@jasmussen
Copy link
Contributor

This was done on purpose because ensuring there's enough room on the left is no easy task and something that should be handled in Popover instead

Understood, thanks for the clarification.

I think this particular offset is important for the intitial G2 launch, so the block switcher can remain the anchor point. In that vein, I would think we should go with a hacky solution, even if that breaks down in some edgecases, and track that edgecase to be followed up on. I'll take a quick look to see if I can find a good trick.

@jasmussen
Copy link
Contributor

I'm looking a a quick fix for the mover position, and at random (I think I was messing with a paragraph inside a column), I got this:

utils.js:20 Uncaught TypeError: Cannot read property 'matches' of null
    at getIsHovered (utils.js:20)
    at shouldHideMovers (utils.js:24)
    at utils.js:54

This improves the situation for the 90% of the time.
@jasmussen
Copy link
Contributor

jasmussen commented Feb 19, 2020

I pushed a little fix.

  1. improved the positioning, it was off by 1px.
  2. added a z-index to the popout mover control so it appears fromunderneath the toolbar.
  3. moved to using CSS variables.

The bigger fix is adding back the offset, which works well in most cases:

Screenshot 2020-02-19 at 10 51 01

fullwide

The edgecase we still have to fix, presumably in the popover itself, is to allow the positioning to have a "minimun left absolute position". Right now it positions perfectly, we "simply" need to have a buffer there, so that the left most position of the toolbar can never be less than 96px. I feel we can do that in a followup, since this is already better than what's shipping, where the toolbar is cropped left and rigth, and at the same time it allows us to ship with the intentional toolbar behavior.

This is a left paragraph in a full wide columns block:

Screenshot 2020-02-19 at 10 51 42

@@ -424,6 +424,9 @@
/**
* Block Toolbar when contextual.
*/
.block-editor-block-contextual-toolbar-wrapper {
padding-left: $grid-unit-60; // Provide space for the mover control on full-wide items.
Copy link
Contributor

Choose a reason for hiding this comment

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

A better variable name here is $block-toolbar-height as that's the width the icon buttons are assigned. It's not the only place where this is used directly in G2 but I believe we should change all of these sizes (that related to the toolbar height) with the $block-toolbar-height variable instead. Because that way, if we change the toolbar height, it changes all the related sizes accordingly.

@youknowriad
Copy link
Contributor

Thinking we should merge this branch into G2 now to have one less branch to work on. It's in a solid state and we can continue our last fixes on the G2 branch before landing it. Thoughts?

@jasmussen
Copy link
Contributor

Go for it. Should we do the same with g3?

@ItsJonQ
Copy link
Author

ItsJonQ commented Feb 19, 2020

Thinking we should merge this branch into G2 now to have one less branch to work on

@youknowriad + @jasmussen Sounds good to me! Just pushed a change to fix the util.js error you reported @jasmussen

@youknowriad
Copy link
Contributor

@jasmussen I'm considering it but still pondering and waiting for some feedback on my last comments there.

@youknowriad youknowriad merged commit 1b8dd4f into try/g2 Feb 19, 2020
@youknowriad youknowriad deleted the try/g2-movers branch February 19, 2020 14:08
youknowriad added a commit that referenced this pull request Feb 24, 2020
Co-authored-by: Joen Asmussen <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
youknowriad added a commit that referenced this pull request Feb 24, 2020
Co-authored-by: Joen Asmussen <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants