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

Make ContextMenu async #16192

Merged
merged 4 commits into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@50Wliu
Member

50Wliu commented Nov 15, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Makes the context menu async, which is important for perceived performance and UX. No longer will everything freeze when opening a context menu. Here's a gif of snowfall!
async-context-menu

Alternate Designs

None.

Why Should This Be In Core?

Context menu handling occurs in the main process.

Benefits

Context menus no longer block the renderer process until they are closed.

Possible Drawbacks

Per electron/electron#9441 it is not possible to detect when the context menu is closed. Atom currently does not track context menu state so I think this compromise is fine.

Applicable Issues

Supersedes and closes #13398
Fixes #2991
Refs #13617, though it looks like right-click isn't properly focusing the tab so more investigation will be needed.

@50Wliu 50Wliu added the needs-review label Nov 15, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 15, 2017

Member

Follow-up issues:

Member

50Wliu commented Nov 15, 2017

Follow-up issues:

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Nov 15, 2017

Member

/cc @BinaryMuse to review

Member

lee-dohm commented Nov 15, 2017

/cc @BinaryMuse to review

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Nov 17, 2017

Member

The concern I had about the right-click hijacking hack in the GitHub package appears to be fine with this change. 👍 to merge once it's green!

Member

BinaryMuse commented Nov 17, 2017

The concern I had about the right-click hijacking hack in the GitHub package appears to be fine with this change. 👍 to merge once it's green!

@50Wliu 50Wliu merged commit cdb5103 into master Nov 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-async-context-menu branch Nov 17, 2017

@Ingramz Ingramz referenced this pull request Dec 5, 2017

Closed

Right clicking on selected text deselects the text #16324

1 of 1 task complete

@50Wliu 50Wliu referenced this pull request Aug 7, 2018

Closed

Right Clicking Creates a Different Type of Text Selection #17799

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment