lime-elements icon indicating copy to clipboard operation
lime-elements copied to clipboard

RFC: stop global clickjacking in popover and menu

Open anderssonjohan opened this issue 3 years ago • 13 comments

When using our popover and menu components, the click from the user is killed when the component is open. So for example, if a filter popover is open in the table view, the user can no longer single-click to open a record because the popover steals the first click to close it.

The same goes for the menu component where we have documented the reasons for having this clickjacking.

  // When the menu surface is open, we want to stop the `click` event from propagating
  // when clicking outside the surface itself. This is to prevent any dialog that might
  // be open from closing, etc. However, when dragging a scrollbar no `click` event is emitted,
  // only mousedown and mouseup. So we listen for `mousedown` and attach a one-time listener
  // for `click`, so we can capture and "kill" it.

IMHO, it would be better if we only stopped the propagation of the click when clicking outside a dialog when another portal is open.

To demonstrate the differences of using event.stopPropagation(), the published docs of this PR contains an example, popover-click-issue.tsx, where the clickjacking can be turned off for popover and menu. Tick "Stop clickjacking" to get the wanted behavior.

So, what would be the best way to detect if we have an dialog open and should stop propagating the click event? Could we check (in the click handlers in popover and menu) if the last opened portal is a dialog and only then use event.stopPropagation()? Or even better, could we place this in the dialog component and let it check if another portal have opened after the dialog was opened? So if there is a "newer" portal than the one for the current dialog when receiving the click, stop the event from also closing the dialog.

Thoughts?

anderssonjohan avatar Mar 26 '21 17:03 anderssonjohan