Overview

After applying the fix from #3459249, which addressed the problem of opening a context menu on right-click, a new issue has emerged related to the handling of mouse wheel events.

In the previous fix, we introduced the following code snippet to handle zooming in and out on a canvas element when a modifier key (like `Ctrl`) is pressed, and to allow for canvas scrolling when the modifier key is not pressed.

With this change, the default mouse wheel scrolling behavior is always prevented unless a modifier key is pressed. This has caused a problem when a sidebar on the left is open:

- **No Scroll on Sidebar**: When the sidebar is open and the user tries to scroll using the mouse wheel, the sidebar does not scroll. This is because the event is being captured by the `handleWheel` function and prevented from propagating to the sidebar.

Proposed resolution

User interface changes

Issue fork experience_builder-3472634

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

boulaffasae created an issue. See original summary.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Priority: Normal » Critical

Confirmed that the component scroll isn't working in latest code as of a couple hours ago.

https://youtu.be/j46NzjwEfp8

boulaffasae opened merge request !273

boulaffasae’s picture

boulaffasae commented
Status: Active » Needs review

The above should resolve the issue.

1. Removing this code should allow default scrolling behavior


        } else {
          e.preventDefault();
          if (canvasPaneRef.current) {
            canvasPaneRef.current.scrollTop += e.deltaY;
            canvasPaneRef.current.scrollLeft += e.deltaX;
          }
        }

2. There was an issue with zoom in/out because the event was triggered two times - so I had to remove the 2nd call


        if (e.ctrlKey) {
          e.preventDefault();
          dispatch(canvasViewPortZoomDelta(e.deltaY));
        }
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

I've tested this and scrolling the component list does work now.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

I can't say if the code is okay though so we need a frontend person to review.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I assume this is breaking something about interacting with the canvas because there's a test failure related to that. It's okay if you want to use this as a workaround in the meanwhile but before merging this, that would need to be investigated.

balintbrews’s picture

balintbrews
he/him/his
Primary language Hungarian
Location Netherlands 🇪🇺
commented
Status: Needs review » Needs work

@boulaffasae, thank you for opening the issue and the MR. I left you a comment there. Can you please explain what you wrote about removing that second piece?

There was an issue with zoom in/out because the event was triggered two times - so I had to remove the 2nd call

Where did you see that the event was getting triggered twice? I didn't manage to reproduce it by adding those lines back.

boulaffasae’s picture

boulaffasae commented

Hi @balintbrews,

Looking at the code, modifierKeyPressedRef use modifierKeyPressed which is defined as 'ctrl' key

useHotkeys('ctrl', () => setModifierKeyPressed(true), {
    keydown: true,
    keyup: false,
  });

which mean this will be trigged when you hold ctrl, same goes for the code I removed.

It get triggered on e.ctrlKey.

The 1st one dispatch canvasViewPortZoomIn/Out, the 2nd one dispatch canvasViewPortZoomDelta

I recorded a video:
https://www.loom.com/share/ca60394e70844bb88cfe954ee4d172c3?sid=5893a489-832a-4d4b-ba22-f6e3acfe8831

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs work » Needs review
balintbrews’s picture

balintbrews
he/him/his
Primary language Hungarian
Location Netherlands 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

@boulaffasae and I worked on this issue together via Slack, and concluded that #4.1 is the actual solution, and that the code was most likely re-introduced in #3459249: Allow opening the contextual menu by right clicking a component by a merge mistake. (It was removed in #3471102: Unable to scroll component list.) We also had to add back #4.2, but it was indeed the case that the same event was being handled in two different ways simultaneously, so we ended up removing some code for that which came from #3459249: Allow opening the contextual menu by right clicking a component, which we deemed unnecessary.

Setting the issue to RTBC, I'd like another pair of eyes on this before we merge.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Awesome! Thanks for the fast review... hopefully this can land soon.

balintbrews’s picture

balintbrews
he/him/his
Primary language Hungarian
Location Netherlands 🇪🇺
commented

In the meantime I opened #3472687: Write end-to-end test for scrolling component insert panel.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » jessebaker

jessebaker made their first commit to this issue’s fork.

jessebaker’s picture

jessebaker commented
Assigned: jessebaker » Unassigned
Status: Reviewed & tested by the community » Fixed

Tested, and code reviewed. Merging!

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Yay!!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.