ClickAwayListener triggers on clicking browser scrollbar

[Umerbhat]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

When Menu is opened, if the user tries to scroll using a browser scrollbar. The Menu shouldn't close.

Current Behavior 😯

When Menu is opened, if the user tries to scroll using a browser scrollbar. The Menu closes.

Steps to Reproduce 🕹

Link: https://material-ui.com/demos/menus/#menulist-composition

1. Navigate to mentioned link, Click on TOGGLE MENU GROW button
2. The menu opens up under the button
3. When the menu is open, try to scroll by clicking browser scrollbar
4. As soon u leave click from the scrollbar, Menu closes itself

Context 🔦

When the menu has too many items if the user wants to select the last item, he needs to scroll. But this issue blocks a user from selecting the last options of the menu.

[eps1lon]

We would need to detect in the ClickAwayListener if the click happened on the scrollbars. I don't know if this is possible.

Could be so kind and create a playground with a Menu with just many items so that we can verify whether this is an issue with this particular demo or the base Menu.

[Umerbhat]

We would need to detect in the ClickAwayListener if the click happened on the scrollbars. I don't know if this is possible.

Could be so kind and create a playground with a Menu with just many items so that we can verify whether this is an issue with this particular demo or the base Menu.

I have checked code in ClickAwayListener. No condition is written to handle scrollbar click

[oliviertassinari]

@Umerbhat I have found a similar thread on kentor/react-click-outside#9. It's something Bootstrap handles well. What do you think of this change?

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index 604c02a75..f4347e60c 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -40,6 +40,11 @@ function ClickAwayListener(props) {

   const handleClickAway = React.useCallback(
     event => {
+      // Ignore click events on the scrollbar.
+      if (event.offsetX > event.target.clientWidth || event.offsetY > event.target.clientHeight) {
+        return;
+      }
+
       // Ignore events that have been `event.preventDefault()` marked.
       if (event.defaultPrevented) {
         return;

Used https://stackoverflow.com/questions/10045423/determine-whether-user-clicking-scrollbar-or-content-onclick-for-native-scroll

[Umerbhat]

@Umerbhat I have found a similar thread on kentor/react-click-outside#9. It's something Bootstrap handles well. What do you think of this change?

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index 604c02a75..f4347e60c 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -40,6 +40,11 @@ function ClickAwayListener(props) {

   const handleClickAway = React.useCallback(
     event => {
+      // Ignore click events on the scrollbar.
+      if (event.offsetX > event.target.clientWidth || event.offsetY > event.target.clientHeight) {
+        return;
+      }
+
       // Ignore events that have been `event.preventDefault()` marked.
       if (event.defaultPrevented) {
         return;

Used https://stackoverflow.com/questions/10045423/determine-whether-user-clicking-scrollbar-or-content-onclick-for-native-scroll

Yes, I had checked it before that bootstrap handles it well. It is a great solution to find whether we clicked on the scrollbar. However, it works only for inner scrollbars. For outer scrollbars like in macOS, it fails

[oliviertassinari]

@Umerbhat Correct, this would work:

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index 604c02a75..b1273b25a 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -64,6 +64,15 @@ function ClickAwayListener(props) {

       const doc = ownerDocument(node);

+      // Ignore click events on the scrollbar.
+      if (
+        event.offsetX > event.target.clientWidth ||
+        event.offsetY > event.target.clientHeight ||
+        event.target === doc.querySelector('html')
+      ) {
+        return;
+      }
+
       if (
         doc.documentElement &&
         doc.documentElement.contains(event.target) &&

But something is off, I can't figure out how bootstrap, reactstrap and react-bootstrap have a correct behavior without any explicit code 😮.

[oliviertassinari]

Found it 💡 ! It has to do with using onClick (OK) vs onMouseDown / onMouseUp (KO).

[Umerbhat]

Found it 💡 ! It has to do with using onClick (OK) vs onMouseDown / onMouseUp (KO).

Got it! it differs on Event. Well, I had a solution. It works for chrome macOS

const rect = event.target.getBoundingClientRect()
      // Do not act if user clicks any scrollbar
      if (rect.width > event.target.clientWidth) {
        return;
      }

  // Do not act if user clicks on body inner scrollbar - like in chrome mac OS
  if(rect.left + rect.right === window.innerWidth){
    return;
  }

[oliviertassinari]

@Umerbhat The fewer layout math we can do, the better.

[oliviertassinari]

@Umerbhat Do you want to move forward with the event.target === doc.querySelector('html') check in a pull request?
It's going to help in the upcoming dropdown effort of @ryancogswell.

[Umerbhat]

get === doc.querySelector('html')

yes, it is fine to do for document's scrollbar. In my case dropdown is inside scrollable divisions, we need to handle it. I think these two will work fine together.

event.target === doc.querySelector('html')
const rect = event.target.getBoundingClientRect() // Do not act if user clicks any scrollbar if (rect.width > event.target.clientWidth) { return; }

Thus, handling click on all visible scrollbars.

[Umerbhat]

@Umerbhat The fewer layout math we can do, the better.

and the best

[oliviertassinari]

Ok, so maybe the best option is to start by changing the default event listener?

[eps1lon]

One thing I noticed is that no onClick is fired if I click on a scrollbar. Only mousedown + mouseup. So the easiest solution is to just use mouseEvent="onClick" it seems.

[oliviertassinari]

Yeah, exactly, change it to use onClick +1.

[eps1lon]

Though I have a hard time finding this in the spec. We need to verify if this works on all browsers.

[Umerbhat]

One thing I noticed is that no onClick is fired if I click on a scrollbar. Only mousedown + mouseup. So the easiest solution is to just use mouseEvent="onClick" it seems.

I am glad to know this, the only difference in bootstrap. Great Work!

[oliviertassinari]

@Umerbhat Do you want to submit a pull request here to fix the default value?

[Umerbhat]

@Umerbhat Do you want to submit a pull request here to fix the default value?

@oliviertassinari Sure. Thanks for the honour. Default props onMouseUp to onClick