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.

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

fix: correct triggeredByAccelerator Event property behavior #18865

Merged
merged 14 commits into from Jun 28, 2019

Conversation

@erickzhao
Copy link
Member

erickzhao commented Jun 18, 2019

Description of Change

Fixes #18808

Previously, the triggeredByAccelerator flag would be entirely coupled with whether or not the modifier keys were being used or not.

This PR swaps out the ui::EventFlagsFromModifiers([event modifierFlags])) call in the macOS code to ui::EventFlagsFromNSEventWithModifiers(event, [event modifierFlags])). The latter outputs flags that take into account mouse click events on top of modifier flags (see Chromium documentation).

The business logic to detect triggeredByAccelerator is then changed to exclude any mouse click flags.

Furthermore, a broken test was changed to properly detect that the accelerator registers properly and that the triggeredByAccelerator flag is properly detected.

Checklist

cc @deermichel @codebytere

Release Notes

Notes: Fixed triggeredByAccelerator Event property behavior for MenuItems

@erickzhao erickzhao requested review from codebytere and deermichel Jun 18, 2019
@erickzhao erickzhao requested a review from ckerr Jun 18, 2019
Copy link
Member

ckerr left a comment

Nice improvement. The fix makes sense

atom/browser/api/event_emitter.cc Outdated Show resolved Hide resolved
atom/browser/api/event_emitter.cc Outdated Show resolved Hide resolved
atom/browser/ui/cocoa/atom_menu_controller.mm Outdated Show resolved Hide resolved
spec/api-menu-spec.js Show resolved Hide resolved
spec/api-menu-spec.js Show resolved Hide resolved
@ckerr ckerr dismissed their stale review Jun 18, 2019

--ci mode was to blame for the odd test behavior, not this PR.

@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 19, 2019
@erickzhao erickzhao force-pushed the intern/decouple-trigger-from-modifiers branch from c19ef5d to 4085d53 Jun 20, 2019
@erickzhao erickzhao force-pushed the intern/decouple-trigger-from-modifiers branch from 3268f43 to 4f9f1d7 Jun 21, 2019
@codebytere codebytere mentioned this pull request Jun 24, 2019
3 of 3 tasks complete
@erickzhao

This comment has been minimized.

Copy link
Member Author

erickzhao commented Jun 24, 2019

It seems like CI is failing the newly added tests on linux-x64, ARM, and Windows. A few notes on that after exploring the problem a bit:

  • It seems like the accelerator isn't being triggered upon the RobotJS button press. The same test is failing everywhere. We're expecting the hasBeenClicked flag to be switched to true, but that line of code is never triggered.
  • The problem doesn't seem to be explicitly tied to the changes made in this PR. A different branch that fixed the test a bit differently also failed CI in a similar way (without any changes to the accelerator code).
  • It doesn't seem to be explicitly tied to RobotJS. This PR also adds a before() hook to the accelerator tests that skips them if Electron's globalShortcut API doesn't work with RobotJS.

Will try to debug this on a Linux/Windows machine soon.

cc @deermichel @codebytere

@erickzhao

This comment has been minimized.

Copy link
Member Author

erickzhao commented Jun 25, 2019

Hey y'all,

The root cause of the failing test was very much related to the problem that @ckerr was referring to here #18865 (comment). After debugging the test code with @jkleinsc, it seems like the --ci flag hiding the app window during tests (see src/spec/static/main.js) is causing weird interactions with RobotJS.

Observed behaviour: RobotJS logs its keystrokes into the console rather than into the test app (observe tnot ok 10 and Tok 11 in the screenshot below corresponding to the T and Shift+T key taps in the spec).
image

Things that have been attempted to fix this:

  • Test without this PR's changes using the binary from dist.zip in the master branch in the CI build. (Still failed).
  • Create a new BrowserWindow on the Mocha before() hook and clean it up with the after() hook.
    • Set the window's options as show: true or show: false.
    • Calling w.focus() on the window w before running the tests.
    • Copy over the other instantiation options from src/spec/static/main.js.
    • N.B. Creating a new browser window gets rid of the RobotJS key taps logging to console, but the tests still fail.

Elsewhere in the code, there's only one other instance of RobotJS, which also skips CI on Mac and Windows.

Therefore, I'm advocating to just skip the CI for the accelerator tests for now. Would that be okay?

cc @deermichel @codebytere @ckerr @jkleinsc

@codebytere codebytere merged commit e03a400 into master Jun 28, 2019
13 checks passed
13 checks passed
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190625.3 succeeded
Details
electron-arm64-testing Build #20190625.3 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jun 28, 2019

Release Notes Persisted

Fixed triggeredByAccelerator Event property behavior for MenuItems

@codebytere codebytere deleted the intern/decouple-trigger-from-modifiers branch Jun 28, 2019
@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Jun 28, 2019

@erickzhao this will probably need some manual backports to 6/5/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.