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

Add/drag handle #9569

Merged
merged 30 commits into from Sep 14, 2018

Conversation

@nosolosw
Member

nosolosw commented Sep 3, 2018

This PR adds the drag handle to the up/down mover.

peek 2018-09-03 20-02

TODO

  • Change icon to drag state.
  • Add current BlockDraggable conditions.
  • Call block's onDragStart / onDragEnd.
  • Add isDragHandleVisible prop.
  • Add aria roles.
  • Fix overflow for one-liner paragraph blocks.
  • Update tests.
  • Show drag handle while dragging.
  • Hide classic toolbar while dragging.
  • Refactor BlockDraggable?

Test

Regular drag-n-drop operation (including child blocks, and embeds).

@nosolosw nosolosw changed the base branch from master to add/with-draggable Sep 3, 2018

@nosolosw nosolosw requested a review from jasmussen Sep 3, 2018

@nosolosw nosolosw self-assigned this Sep 3, 2018

@mtias mtias added this to the 3.8 milestone Sep 3, 2018

@nosolosw nosolosw changed the base branch from add/with-draggable to master Sep 3, 2018

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 3, 2018

Member

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block.

In the spirit of doing one thing at a time, I wouldn't have changed that behavior in this PR if it wasn't for this: the grey box was a dedicated div that acted as a drag handle and that was made visible when the dragging process started; now that we've removed the dedicated drag handle we no longer have that div node to play with. Also: the opacity seems nicer to me in this case. If we still consider the grey box a preferred UI, I can figure out how to maintain that behavior.

Member

nosolosw commented Sep 3, 2018

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block.

In the spirit of doing one thing at a time, I wouldn't have changed that behavior in this PR if it wasn't for this: the grey box was a dedicated div that acted as a drag handle and that was made visible when the dragging process started; now that we've removed the dedicated drag handle we no longer have that div node to play with. Also: the opacity seems nicer to me in this case. If we still consider the grey box a preferred UI, I can figure out how to maintain that behavior.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Very nice work here.

I pushed a little polish in 4128f21:

screen shot 2018-09-04 at 10 26 36

That is, buttons are now slightly smaller so as to better fit on a single paragraph height, which is the smallest block we can have. The paragraph is actually 56px tall, and the buttons are now 72px (3*24px). So I've compensated by vertically centering them. Here's with a little debug help:

screen shot 2018-09-04 at 10 19 06

This is before:

screen shot 2018-09-04 at 09 55 46

I'm going to next take a stab at tweaking the hover style of the drag handle. It shouldn't be a button, the hand cursor itself should be mostly sufficient, perhaps with a color change on hover.

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block

I think it's okay to visit this, it's a bit challenged as is.

The thing is, you can now style the editor... you can change the text color and the background color using editor styles. But unless @youknowriad with his new CSS parser can do magic here, we can't know which background color to show on a block that's being dragged. Until now we've simply assumed white.

Ideally we'd know what the color would be, it's a little hard to see that you're dragging a block right now since it has no background. But I think it's good to surface this discussion on what to do here.

Another option is, instead of showing a clone of the block, showing a small icon of the block. Like, imagine as you "lift the block off the page", it transforms into a small block next to your cursor, and expands again when placed. That way we'd cover less of the page. But I know @mtias has some opinions here we should listen to.

Contributor

jasmussen commented Sep 4, 2018

Very nice work here.

I pushed a little polish in 4128f21:

screen shot 2018-09-04 at 10 26 36

That is, buttons are now slightly smaller so as to better fit on a single paragraph height, which is the smallest block we can have. The paragraph is actually 56px tall, and the buttons are now 72px (3*24px). So I've compensated by vertically centering them. Here's with a little debug help:

screen shot 2018-09-04 at 10 19 06

This is before:

screen shot 2018-09-04 at 09 55 46

I'm going to next take a stab at tweaking the hover style of the drag handle. It shouldn't be a button, the hand cursor itself should be mostly sufficient, perhaps with a color change on hover.

Also for design feedback: note that this PR also changes how Gutenberg indicates which block is being dragged: it uses the block's opacity instead of showing a grey box above the block

I think it's okay to visit this, it's a bit challenged as is.

The thing is, you can now style the editor... you can change the text color and the background color using editor styles. But unless @youknowriad with his new CSS parser can do magic here, we can't know which background color to show on a block that's being dragged. Until now we've simply assumed white.

Ideally we'd know what the color would be, it's a little hard to see that you're dragging a block right now since it has no background. But I think it's good to surface this discussion on what to do here.

Another option is, instead of showing a clone of the block, showing a small icon of the block. Like, imagine as you "lift the block off the page", it transforms into a small block next to your cursor, and expands again when placed. That way we'd cover less of the page. But I know @mtias has some opinions here we should listen to.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

In 8c575d2 I committed a change I'd like a sanity check on. From the commit message:

Try: Remove hover style and make un-tabbable

This needs a sanity check and is a separate commit for that reason, so easier to revert if it isn't kosher.

This commit removes the hover and focus styles for the drag handle.

It also zeroes out the aria label, makes aria hidden, and unfocusable using the keyboard. The idea being that you cannot at all interact with the drag handle using just the keyboard, we have the movers for that, and thus this item shouldn't be announced to either screen readers or be part of the tab index as that's simply useless.

Thoughts?

Here's how this looks when mousing:

dragging now

Here's how this looks when keyboarding:

tabbing

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

Contributor

jasmussen commented Sep 4, 2018

In 8c575d2 I committed a change I'd like a sanity check on. From the commit message:

Try: Remove hover style and make un-tabbable

This needs a sanity check and is a separate commit for that reason, so easier to revert if it isn't kosher.

This commit removes the hover and focus styles for the drag handle.

It also zeroes out the aria label, makes aria hidden, and unfocusable using the keyboard. The idea being that you cannot at all interact with the drag handle using just the keyboard, we have the movers for that, and thus this item shouldn't be announced to either screen readers or be part of the tab index as that's simply useless.

Thoughts?

Here's how this looks when mousing:

dragging now

Here's how this looks when keyboarding:

tabbing

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

Some further info on this, the copy cursor is supposed to show up in the file system while you're dragging a file and holding ⌥ at the same time.

Ironically, I'm experiencing the inverse behavior both in master and in this branch: when I'm holding ⌥, the copy cursor disappears and the correct grabbing cursor is present.

The behavior is correct in Safari — when I drag normally, I'm seeing the correct grabbing cursor. When I hold ⌥ in Safari, I'm seeing the copy cursor.

So it seems as though Chrome is either buggy, or my system is buggy, or some part of our code makes Chrome think that by default a drag action is a copy/duplicate action as opposed to a move action.

Contributor

jasmussen commented Sep 4, 2018

Note by the way that in Chrome on Mac I'm seeing a "copy" cursor as soon as I start dragging. I should be seeing the grabbing cursor instead. Any insights as to why that might be?

Some further info on this, the copy cursor is supposed to show up in the file system while you're dragging a file and holding ⌥ at the same time.

Ironically, I'm experiencing the inverse behavior both in master and in this branch: when I'm holding ⌥, the copy cursor disappears and the correct grabbing cursor is present.

The behavior is correct in Safari — when I drag normally, I'm seeing the correct grabbing cursor. When I hold ⌥ in Safari, I'm seeing the copy cursor.

So it seems as though Chrome is either buggy, or my system is buggy, or some part of our code makes Chrome think that by default a drag action is a copy/duplicate action as opposed to a move action.

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 4, 2018

Member

In a96bb4b we experimented with setting the dnd event to move to see if that affected how chrome on mac styled things. It seems it does. It shows the default cursor instead (note how it briefly shows the move cursor and then the default one when the dragover event starts):

cursor

Change reverted in 3e595c5.

Member

nosolosw commented Sep 4, 2018

In a96bb4b we experimented with setting the dnd event to move to see if that affected how chrome on mac styled things. It seems it does. It shows the default cursor instead (note how it briefly shows the move cursor and then the default one when the dragover event starts):

cursor

Change reverted in 3e595c5.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Removed the hover color from the drag handle, and also made it bg color friendly.
screen shot 2018-09-04 at 11 35 17

Contributor

jasmussen commented Sep 4, 2018

Removed the hover color from the drag handle, and also made it bg color friendly.
screen shot 2018-09-04 at 11 35 17

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 4, 2018

Member

Created an issue to track the cursor issue at #9610

Member

nosolosw commented Sep 4, 2018

Created an issue to track the cursor issue at #9610

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 4, 2018

Contributor

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Error: Attribute focusable not allowed on element button at this point.

Contributor

afercia commented Sep 4, 2018

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Error: Attribute focusable not allowed on element button at this point.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 5, 2018

Contributor

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Good catch, thanks. Is this implicit approval of the assertion that the drag handle should not be part of the tab order/be keyboardable?

Contributor

jasmussen commented Sep 5, 2018

The focusable attribute rendered in the HTML is invalid HTML and should be omitted:

Good catch, thanks. Is this implicit approval of the assertion that the drag handle should not be part of the tab order/be keyboardable?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2018

Contributor

Yup, I'd agree taking it out from the tab sequence and hiding it with aria-hidden=true makes sense, as it can't be used with a keyboard.

Contributor

afercia commented Sep 5, 2018

Yup, I'd agree taking it out from the tab sequence and hiding it with aria-hidden=true makes sense, as it can't be used with a keyboard.

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 5, 2018

Member

@jasmussen in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div for the drag handle. The reason is: Firefox won't make buttons draggable. Would you mind giving it a sanity check and verify if it unintentionally broke any of your modifications?

Member

nosolosw commented Sep 5, 2018

@jasmussen in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div for the drag handle. The reason is: Firefox won't make buttons draggable. Would you mind giving it a sanity check and verify if it unintentionally broke any of your modifications?

@youknowriad youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2018

Contributor

in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div

Then, I'd suggest to remove tabIndex, as a div is not focusable by default. Also onFocus / onBlur I guess. And the empty aria-label="" is a bit pointless.

Contributor

afercia commented Sep 5, 2018

in 8ee6afa I've changed the IconButton (which ended up adding a button element) by a simple div

Then, I'd suggest to remove tabIndex, as a div is not focusable by default. Also onFocus / onBlur I guess. And the empty aria-label="" is a bit pointless.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

I removed the aria label and tabindex.

I would honestly prefer we remove the tooltip as well — the drag handle feels obvious to me, and because it is an addition for mouse users to the clickable up/down arrows, it doesn't feel necessary to me.

On the flipside, it feels intrusive, and often invokes even when it shouldn't:

screen shot 2018-09-06 at 08 44 01

— at this point I'm dragging, so the tooltip should disappear. In other words, if we are to have a tooltip, we need to make sure it disappears as soon as you start dragging.

I also feel like we should consider doing something about the transparent background while dragging. I'm going to try something, but not sure it'll work.

Contributor

jasmussen commented Sep 6, 2018

I removed the aria label and tabindex.

I would honestly prefer we remove the tooltip as well — the drag handle feels obvious to me, and because it is an addition for mouse users to the clickable up/down arrows, it doesn't feel necessary to me.

On the flipside, it feels intrusive, and often invokes even when it shouldn't:

screen shot 2018-09-06 at 08 44 01

— at this point I'm dragging, so the tooltip should disappear. In other words, if we are to have a tooltip, we need to make sure it disappears as soon as you start dragging.

I also feel like we should consider doing something about the transparent background while dragging. I'm going to try something, but not sure it'll work.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 6, 2018

Contributor

Not sure why onFocus and onBlur are kept, as this <div> can't be receive focus and can't be "blurred".

Contributor

afercia commented Sep 6, 2018

Not sure why onFocus and onBlur are kept, as this <div> can't be receive focus and can't be "blurred".

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

@nosolosw I'll let you respond to the onfocus thoughts.

For now, I've made a change to the visual styling of the dragondrop:

changes

I know this is kind of like how it used to be. I'd like to explain why it is this way. Most importantly: we are not cloning a block when you are dragging, we are moving, so it's important the block you move is not seen as a clone. Hence the gray BG.

Also, I hid the block toolbar and mover on the "block that's left behind", it didn't make sense to keep those there. But I am hiding them on the dragged block.

I tried unhiding the editor-block-mover on the draggable clone — because otherwise the grip you're holding disappears when you are dragondropping, that's a little weird. But doing so makes dragondrop not work. Any idea why?

Contributor

jasmussen commented Sep 6, 2018

@nosolosw I'll let you respond to the onfocus thoughts.

For now, I've made a change to the visual styling of the dragondrop:

changes

I know this is kind of like how it used to be. I'd like to explain why it is this way. Most importantly: we are not cloning a block when you are dragging, we are moving, so it's important the block you move is not seen as a clone. Hence the gray BG.

Also, I hid the block toolbar and mover on the "block that's left behind", it didn't make sense to keep those there. But I am hiding them on the dragged block.

I tried unhiding the editor-block-mover on the draggable clone — because otherwise the grip you're holding disappears when you are dragondropping, that's a little weird. But doing so makes dragondrop not work. Any idea why?

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 6, 2018

Member

Removed the onFocus / onBlur and the Tooltip. Being the drag handler not focusable it doesn't make a lot of sense to have those.

Member

nosolosw commented Sep 6, 2018

Removed the onFocus / onBlur and the Tooltip. Being the drag handler not focusable it doesn't make a lot of sense to have those.

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Sep 6, 2018

Member

I tried unhiding the editor-block-mover on the draggable clone — because otherwise the grip you're holding disappears when you are dragondropping, that's a little weird. But doing so makes dragondrop not work. Any idea why?

I'll take a look. Perhaps a race condition: hidden nodes won't take events, so depending on how we do that (before/after connecting the event, etc) the visibility change may be messing up with the DnD event.

Member

nosolosw commented Sep 6, 2018

I tried unhiding the editor-block-mover on the draggable clone — because otherwise the grip you're holding disappears when you are dragondropping, that's a little weird. But doing so makes dragondrop not work. Any idea why?

I'll take a look. Perhaps a race condition: hidden nodes won't take events, so depending on how we do that (before/after connecting the event, etc) the visibility change may be messing up with the DnD event.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

I'll take a look. Perhaps a race condition: hidden nodes won't take events, so depending on how we do that (before/after connecting the event) the visibility change may be messing up with the DnD event.

I tried adding pointer-events: none; and even z-index: 0 to make the mover/handle visible on the drag clone even if not interactable. But this didn't work.

Contributor

jasmussen commented Sep 6, 2018

I'll take a look. Perhaps a race condition: hidden nodes won't take events, so depending on how we do that (before/after connecting the event) the visibility change may be messing up with the DnD event.

I tried adding pointer-events: none; and even z-index: 0 to make the mover/handle visible on the drag clone even if not interactable. But this didn't work.

jasmussen and others added some commits Sep 4, 2018

Use a div as the drag handle
Firefox can't make buttons draggable.
Expose an isDraggable property to control draggability
So drag is something that can be turned on/off depending
on different UI needs.
Show the mover in the clone image
The drag handle is part of the mover so we want to keep it visible
while dragging.
Use BlockDraggable instead of Draggable
This makes BlockDraggable DOM agnostic and uses it as a base to
compose drag data. The vision is that we may have an ImageDraggable
component in the future that would abstract what data the dragging
operation needs to drag images, and so on.

The goal would be to use it by only providing one prop, the
blockClientId.

Making this a single commit in case we want to revert.

@nosolosw nosolosw merged commit 62c527f into master Sep 14, 2018

2 checks passed

codecov/project 49.03% (-0.02%) compared to f9d3f6e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nosolosw nosolosw deleted the add/drag-handle branch Sep 14, 2018

@designsimply

This comment has been minimized.

Show comment
Hide comment
@designsimply

designsimply Sep 14, 2018

Contributor

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit. (screenshot)

Contributor

designsimply commented Sep 14, 2018

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit. (screenshot)

jasmussen added a commit that referenced this pull request Sep 17, 2018

Fix draghandle color
Fixes design nitpick from #9569 (comment).

@jasmussen jasmussen referenced this pull request Sep 17, 2018

Merged

Fix draghandle color #9948

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 17, 2018

Contributor

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Yes, that's intentional. It's more of a mode for writing and focusing, than it is for rearranging. But thanks for checking int.

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit.

EAGLE EYE SHERI! Awesome.

PR here: #9948

Contributor

jasmussen commented Sep 17, 2018

I noticed neither the draggable handle nor the movers are available in spotlight mode, and I wanted to check whether that is the intended behavior?

Yes, that's intentional. It's more of a mode for writing and focusing, than it is for rearranging. But thanks for checking int.

Every so tiny design nitpick: I noticed the icon for the drag handle (6 dots) is not the same shade of gray compared to the mover icons and it seemed to me they should be the same color. However, I also thought the color difference might intentional to make the draggable handle stand out a bit.

EAGLE EYE SHERI! Awesome.

PR here: #9948

.is-dark-theme & {
background-color: $light-opacity-light-200;
.editor-block-list__layout .editor-block-list__block.is-selected { // Needs specificity to override inherited styles.

This comment has been minimized.

@youknowriad

youknowriad Sep 17, 2018

Contributor

It looks like the changes here introduced a regression in Spotlight mode.

It should be fixed here #9951

@youknowriad

youknowriad Sep 17, 2018

Contributor

It looks like the changes here introduced a regression in Spotlight mode.

It should be fixed here #9951

jasmussen added a commit that referenced this pull request Sep 17, 2018

Fix draghandle color (#9948)
Fixes design nitpick from #9569 (comment).

mcsf pushed a commit that referenced this pull request Sep 21, 2018

Fix draghandle color (#9948)
Fixes design nitpick from #9569 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment