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

Give focus to drag handle if not used for dragging? #1645

Open
TotooriaHyperion opened this issue Nov 29, 2019 · 7 comments
Open

Give focus to drag handle if not used for dragging? #1645

TotooriaHyperion opened this issue Nov 29, 2019 · 7 comments

Comments

@TotooriaHyperion
Copy link

@TotooriaHyperion TotooriaHyperion commented Nov 29, 2019

Expected behavior

click button inside rbd should cause outer elements to blur.

Actual behavior

click button inside rbd makes any element's blur event disabled.

Steps to reproduce

simple as said in behavior

Suggested solution?

rbd only stops blur action inside it.
(sounds impossible, if it's not possible what's the recommanded way to do it ? My solution is to manually blur document.activeElement if exists, at the beginning of the click handler.)

What version of React are you using?

react 16.11.0

What version of react-beautiful-dnd are you running?

11.0.3

What browser are you using?

chrome 78.0.3904.70

Demo

@alexreardon

This comment has been minimized.

Copy link
Contributor

@alexreardon alexreardon commented Dec 4, 2019

Hi there!

Thanks for raising this issue. Can you please create a standalone example on codesandbox.io using our boilerplate: https://codesandbox.io/s/k260nyxq9v

Without a standalone example, we will not be able to action this one

Cheers!

@alexreardon

This comment has been minimized.

Copy link
Contributor

@alexreardon alexreardon commented Dec 4, 2019

I think this issue was fixed in 12.x. Please upgrade and take another look

@TotooriaHyperion

This comment has been minimized.

Copy link
Author

@TotooriaHyperion TotooriaHyperion commented Dec 5, 2019

I think this issue was fixed in 12.x. Please upgrade and take another look

https://codesandbox.io/s/vertical-list-nnlhb

reproduce, seems still the same on v12.
focus the input element outside rbd, then click the item inside rbd, the input element is still focused.

@alexreardon

This comment has been minimized.

Copy link
Contributor

@alexreardon alexreardon commented Dec 5, 2019

I can explain why this is happening, but i'm not sure if we can do much about it at this stage

  • in our mouse sensor we need to call event.preventDefault() on mousedown events that we think could be starting a drag. See how we use dom events. This is to prevent the drag handle from getting focus (which is what would happen because it has a tabIndex)

This is unfortunate as it means that clicking on a drag handle will not cause it to gain focus or for the other item to lose focus.

I think you hit on the right solution: manually cause the element to get focus in your on click (if you want to).

function onClick(event) {
 // we used the event for dragging
 if(event.defaultPrevented) {
    return;
 }

 event.currentTarget.focus();
}

Perhaps we should add this be default?

@alexreardon alexreardon changed the title click button inside rbd don't blur element outside. Give focus to drag handle if not used for dragging? Dec 5, 2019
@alexreardon

This comment has been minimized.

Copy link
Contributor

@alexreardon alexreardon commented Dec 6, 2019

Perhaps we could always our own click handler after we prevent default on a mousedown to ensure focus is correctly given to the element if a click does occur 🤔

@TotooriaHyperion

This comment has been minimized.

Copy link
Author

@TotooriaHyperion TotooriaHyperion commented Dec 6, 2019

I think should also allow inner element to get focused if it's clicked, if not focusable, focus the drag handle.
Also the focus should fire before the user's own onClick event because the native focus/blur fire before click(mousedown -> blur/focus -> mouseup -> click), so I use onClickCapture.

    function onClickCapture(event) {
      if (e.defaultPrevented) {
        return;
      }
      if (e.currentTarget !== e.target && e.target.tabIndex >= 0) {
        e.target.focus();
      } else {
        e.currentTarget.focus();
      }
    }

https://codesandbox.io/s/clever-zhukovsky-rxh22

feels much more intuitive.

image

@alexreardon

This comment has been minimized.

Copy link
Contributor

@alexreardon alexreardon commented Dec 6, 2019

It would be worth thinking through what the behaviour should be for sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.