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.

Sign up for GitHub

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

Jump to bottom

EventHelper has globally visible breaking side effects #5480

Closed
simonihmig opened this issue Oct 26, 2022 · 2 comments
Closed

EventHelper has globally visible breaking side effects #5480

simonihmig opened this issue Oct 26, 2022 · 2 comments
Assignees
@ExtAnimal
Labels
bug Something isn't working resolved Fixed but not yet released (available in the nightly builds)
Milestone
5.2.1

Comments

@simonihmig
Copy link

The EventHelper calls fixEvent on any event for which a handler has been added. This applies a good number of "fixes" on that event object, which can cause unexpected side effects in other parts of the application.

In our particular case this is the breaking part (Core/helper/EventHelper.js line 397-401 for @bryntum/schedulerpro@5.1.5):

        // Sync OSX's meta key with the ctrl key. This will only happen on Mac platform.
        // It's read-only, so define a local property to return true for ctrlKey.
        if (event.metaKey && !event.ctrlKey) {
            Object.defineProperty(event, 'ctrlKey', returnTrueProp);
        }

Which makes all key events that have the meta key pressed look as if the user also pressed the ctrl key, for any parts of the application. In a different library for keyboard handling, where we have a meta+k keyboard shortcut defined, this causes the event matching to fail, as the library would (correctly IMO!) do a complete check on all modifier keys. So (assuming a Mac here) when a user presses ⌘K it would only match the meta+k shortcut, if the ctrl modifier key is not pressed. But those "fixes" make it look as if both ⌘ and ctrl have been pressed.

I don't know if any of the other "fixes" have the potential to break application code, but frankly I believe that the approach of mutating the native browser event, including properties that are (for a reason!?) read-only, is a quite bad idea! If bryntum needs that kind of event fixes or normalization, it could totally handle that internally (e.g. by passing an internal custom event representation around), without global side effects.

Also these event "fixes" live for the whole lifespan of the app (assuming a SPA here), even when the scheduler widget has been torn down, as the EventHelper calls EH.on() (Line 948) in module space, and that listener never gets removed. Having the weird but not breaking side effect of adding the .b-using-keyboard class to <body>, even when not a single bryntum widget is active anymore...
@matsbryntse matsbryntse added the bug Something isn't working label Oct 26, 2022
@ExtAnimal
Copy link

ExtAnimal commented Oct 26, 2022 •
edited
Loading

Widget destruction removes all its event handlers. That's a basic part of cleanup. If there are any Widget element event listeners left around that's a bug.

We have a GobalEvents singleton which tracks whether the user has interacted using the keyboard last. This is for accessibility purposes.

We show visual focus position only if user has used a keyboard not the mouse (most of the time) Field focus has an active rendition. Most other types of focus only appear when the user last interacted via keyboard.

@ExtAnimal
Copy link

We can remove all injected properties after we have called out own handlers.

@ExtAnimal ExtAnimal self-assigned this Oct 26, 2022
@ExtAnimal ExtAnimal added in progress ready for review Issue is fixed, the pull request is being reviewed and removed in progress labels Oct 26, 2022
@ExtAnimal ExtAnimal added this to the 5.2.1 milestone Oct 26, 2022
@ExtAnimal ExtAnimal added resolved Fixed but not yet released (available in the nightly builds) and removed ready for review Issue is fixed, the pull request is being reviewed labels Oct 27, 2022
@SergeyMaltsev SergeyMaltsev changed the title EventHelper has globally visible breaking side effects EventHelper has globally visible breaking side effects Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ExtAnimal ExtAnimal

Labels
bug Something isn't working resolved Fixed but not yet released (available in the nightly builds)
Projects
None yet
Milestone
5.2.1
Development

No branches or pull requests
3 participants
@ExtAnimal @matsbryntse @simonihmig