RFC: stop global clickjacking in popover and menu

[anderssonjohan]

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?

[github-actions]

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-1197/

[adrianschmidt]

I don't think we stop the click just to avoid closing the dialog that the menu might appear in, it's also to avoid interacting with any links or other components when clicking or tapping to close the menu. I'm not sure how Windows and Linux works, but on my Mac, if I click to open a menu, say the File menu for example, the first click outside the menu or menu bar is only closing the menu dropdown, not interacting with any components. While writing this comment, if I open the menu, I can close the menu by clicking anywhere on this web page, without blurring the input field, triggering the Comment or Close with comment buttons, or following any links. For me, that's the expected behaviour. I think that's even more important on a phone, because the combination of the smaller screen area and the larger target areas can make finding any area outside a dropdown where it's "safe" to tap very difficult. (I'm not completely sure, but I think phones typically even "go looking" for nearby interactions to trigger when you tap somewhere, rather than just deciding which pixel is in the middle of your touch area and trigger an event exactly on that pixel.)

However, in the specific case of having a kind of "menubar" with several different menus and their respective triggers, similar to the example linked to in the issue description, I agree that it feels more natural that clicks on another of the menu triggers should both close the currently open dropdown, and open the new dropdown, with only the one click.

I think that rather than removing the "clickjacking" completely, we might wish to add some kind of support for putting menu and popover triggers in some kind of "group", where a click on a different trigger in the same group is not prevented, while clicks on elements outside the group still is…

[anderssonjohan]

@adrianschmidt agreed! I think adding a conditional check for calling stopPropagation would be a good idea. The source should check the target for some state (a naive example could be checking for data-isalwaysinteractable="true") that tells it to not stop the propagation if it's true.

Right now some components are more intrusive than others making the interaction a bit confusing.
I tried to grab some of the inconsistencies from this context here:

• Going from limel-menu to limel-chipset type=input interacts (changes focus) using a single click
• Going from the limel-menu to anything but the input requires two clicks
• Going from the lime-dropdown (old angular component?) to the same input requires to clicks to do the same

Screen.Recording.2021-03-29.at.14.01.04.mov

I don't get what's happening there, because the input should not be able to grab the click event from limel-menu when it has stopped the propagation of the event? In this case, it's only this particular input that behaves like this.

I would think going from one menu to the other on the same page should only require a single click. Currently it does not:

Screen.Recording.2021-03-29.at.14.19.48.mov

Just out of curiosity I checked some menus and parts of Chrome to see how they do it. It's also full of inconsistencies depending on what part you click. Even in the dev tools, there are differences between the little options menu and the expander for its tabbed views. Clicking on a browser tab switches to it with a single click, but not for every open menu. The dev tools option menu stays open in that tab while switching away from the tab.

Screen.Recording.2021-03-29.at.14.23.11.mov

Ugh, this is maybe just one of those inconsistencies we have to live with.

[Kiarokh]

This one is a hard nut to crack.
I agree with both of you @adrianschmidt and @anderssonjohan .
This is highly dependent on the use case.

Adrian certainly has a point. That's why for instance we have an issue like this https://github.com/Lundalogik/crm-feature/issues/1873 where we wanna block click. But it would be certainly very nice to open another popover if user clicks on the filter icon on the column next to the current one.

On the same view (table view) we wanna open the preview card, which is some sort of modal. But still allow users to keep clicking on rows in the table and open new items.

However, as Johans says, there are many situations where expected behaviour after clicking on another "similar" popover or menu is opening it directly. Like menu-bars (which on desktop computers usually even stay active and react to the mouseover instead of clicks) 👇

menubar.mp4

We have this annoying behaviour today in the dock (side-nav-bar) which forces you to close one already-open menu with your first click, to open another one with your second click on the same item: https://github.com/Lundalogik/crm-feature/issues/1500

Solution?

I have no spontaneous and magical solutions.

Given how we have setup our ecosystem, it feels many of these things should be handled for each case individually.

I'm not sure how feasible it is to make manu-bars which group similar components (like menus or popovers), but it might be worth giving it some thoughts. Can we let consumers group similar triggers even if they are in different parts of the UI?

Maybe an easy way out is letting clicks work when the intention is opening another popover or another menu. Meaning, if a menu or a popover is open, any clicks outside of it will close it, unless the click is on trigger of another menu and or popover. Thoughts?

We should also always keep in mind issues like this: https://github.com/Lundalogik/crm-feature/issues/1122

[anderssonjohan]

@Kiarokh thanks for the input!

Maybe an easy way out is letting clicks work when the intention is opening another popover or another menu. Meaning, if a menu or a popover is open, any clicks outside of it will close it, unless the click is on trigger of another menu and or popover. Thoughts?

This wouldn't work in my case where I have both a popover and a menu in the same context. Also, the click (that should close the current portal and open the next) has different types of triggers for the next portal. It may be a custom button or something else that acts as the trigger. In my example of the table filter, the target is an input field.

I don't have a good solution proposal at the moment but I think the following is true:

• The source must keep being responsible for calling stopPropagation (reason: we can't add a source check to every element, ie. a video element would receive the click and act on it)
• There are no clear ways of determining if the event should propagate based on element type (reason: clicking different limel-menus in the main navbar should require a single click to close + open while clicking the main navbar and then the object card context menu should require two click to close + open)
• Some kind of grouping mechanism could be a solution

Some pseudo-code for letting through clicks made outside a dialog/menu/popover to limel-popover, limel-menu, .menu-trigger used in a group:

return [
  <div class="menubar">
    <limel-portal-clickthrough to={['limel-popover', 'limel-menu', '.menu-trigger']}>
		<limel-menu/>
		<limel-menu>
			<limel-button class="menu-trigger" slot="trigger"/>
		</limel-menu>
		<limel-popover/>
	  	<limel-button onClick={this.openDialogA}/>
	</limel-portal-clickthrough>
  </div>,
  <div class="content">
  	<limel-button onClick={this.openDialogB}/>
  </div>
];

So when I click outside the dialog opened from a trigger in .content it would require two clicks to open the menus, popover and dialog inside the <limel-portal-clickthrough/> but everything inside the same <limel-portal-clickthrough/> (whitelisted by it's to prop with selectors) would select when NOT to call stopPropagation. Makes sense or just bullcrap? :)

PS. Also noticed that both Gmail and Outlook (OWA) does not have any use of stopPropagation meaning you can have the "go to date" picker open and click in a toolbar (like delete, archive etc) without first closing the open picker.. I haven't noticed this until now and I think it's not the way it should be. Thanks @adrianschmidt for providing the excellent example with the GitHub comment UI. DS.

[Kiarokh]

Thanks yourself @anderssonjohan !

This wouldn't work in my case where I have both a popover and a menu in the same context.

Not sure if we understood each other.
I meant :

If the currently opened portal is for a menu or a popover, clicking outside but on ANY other trigger of any menu or popover (doesn't matter which one) should close the currently open portal and open the intended one directly after.

Does it make sense?

[anderssonjohan]

Thanks yourself @anderssonjohan !

This wouldn't work in my case where I have both a popover and a menu in the same context.

Not sure if we understood each other.
I meant :

If the currently opened portal is for a menu or a popover, clicking outside but on ANY other trigger of any menu or popover (doesn't matter which one) should close the currently open portal and open the intended one directly after.

Does it make sense?

Ok, yes it does!
The only piece missing there is the grouping of related triggers.
If it's ok to forward the click to any other menu/popover in the DOM, that works.
If we only want to forward the click to these elements in the same context then we need some kind of grouping.
Example: If I've opened a dropdown in a filter popover on the page and then click the user menu in the main navbar, should it open that menu in the same click that closes the dropdown in the popover or not? (also, should the click both close the dropdown in the popover AND the popover?) 🤔 💭

[Kiarokh]

Great point! I think what you're saying only will happen for popovers, and perhaps also modals which can be dismissed by clicking outside.

So could we add the following:

"Popovers and modals create their own isolated contexts/screens. Which can include nested menus or popovers inside themselves. In this case, if a new popover or menu is opened whose trigger is inside an already opened popover or modal (portal) clicking outside of it will only close it, but not close the modal or popover which holds its trigger."

I think this is another narration of what you call "grouping" of related triggers.

No idea how we can solve it technically, but maybe this re-formulation gives some ideas :)

[anderssonjohan]

@Kiarokh Here's a visual example on that use case:

[adrianschmidt]

I'm happy to leave the hashing out to you two, because I don't feel like I have time to follow along in these rather wordy discussions 😊

But, I'd appreciate it a lot if you would ping me about it (on Slack preferably, since I'll try to ignore emails from this issue) before you do any coding on this. I get a little worried when I read stuff like "forward the event", but if I know I'll get to have a look once you have settled on what functionality you want, I don't have to feel stressed about it 😄

[Kiarokh]

@anderssonjohan tnx for the pic. Awesome.

What should left click on Company filter do?

Answer A

What should left click on user menu do?

Result should be the same as answer A for previous question. An interaction design detail: that menu shouldn't visualize the click (display that pressing down animation, or the elevation on hover). Which is the wish here: https://github.com/Lundalogik/crm-feature/issues/1873, even though this issue is immature in its description, given what we discussed here.

[eketorp]

But what if a user clicks the pink area? Johan's mock is missing the table (which is completely clickable). Almost everything in CRM is clickable and it's hard to find "safe space" to minimize pop-overs.

[Kiarokh]

Thats' right @eketorp
Which is why we –unclearly– concluded that:

We will be letting clicks work when the intention is opening another popover or another menu. Meaning, if a menu or a popover is open, any clicks outside of it will close it, unless the click is on trigger of another menu and or popover. In other words, if the currently opened portal is for a menu or a popover, clicking outside but on ANY other trigger of any menu or popover (doesn't matter which one) should close the currently open portal and open the intended one directly after.

However, popovers and modals create their own isolated contexts/screens. Which can include nested menus, dropdowns, or popovers inside themselves. In this case, if a new popover or menu is opened whose trigger is inside an already opened popover or modal (a portal) clicking outside of it (the second portal layer) will only close that layer, but will not close the modal or popover which holds its trigger.

---

I know it's very wordy and hard to follow. But hope you are following.