Flush DOM before displaying context menu #13266

Merged
merged 2 commits into from Dec 6, 2016

Projects

None yet

3 participants

@vjeux
Contributor
vjeux commented Nov 18, 2016 edited

Electron has an issue where displaying the context menu pauses the rendering loop. electron/electron#1854

One unfortunate side effect is that when you right click on an element, the dom mutations to highlight the element have not been flushed yet which makes for a very bad user experience #2991

In order to solve this issue, we would like to flush the mutations and only then display the menu. Unfortunately, electron doesn't have APIs for that.

Things I have tried:

  1. Electron has browserWindow.webContents.invalidate() and .on('paint' -> ...) but it only works with offscreen rendering which atom doesn't use.
  2. .capturePage(-> ...) takes a screenshot of the page and I hoped it would do a flush first but it inconsistently displays the highlight so it won't work.
  3. Using setTimeout((-> ...), 16) works many times but not always
  4. requestAnimationFrame(-> ...) should be what we want but unfortunately it doesn't always show the highlights :(

What I found worked was to nest three requestAnimationFrame, it works all the time even when my machine is busy. This is not a 100% solution but I think that it's okay to assume that if the highlight hasn't been updated in three frames then you should probably try and optimize your code. 3 frames is 50ms and that delay isn't noticeable.

Note: the proper way to solve this that was mentioned in the issue is to open the menu from the main process but this is already what atom does and it's still an issue.

Before:

After:

Released under CC0

@as-cii
Member
as-cii commented Nov 25, 2016

Thanks for the pull request, @vjeux! 💯

Note: the proper way to solve this that was mentioned in the issue is to open the menu from the main process but this is already what atom does and it's still an issue.

Can you confirm this is what Atom is doing? In this line of code it seems like we are calling emit on the BrowserWindow instance: this works because BrowserWindow is an EventEmitter but it looks like that call to emit might actually be synchronous. Can we try sending an asynchronous message to the main process instead and see if it fixes it? What am I missing?

@vjeux
Contributor
vjeux commented Nov 25, 2016

Do you know what is the function to send an asynchronous message? (First time I'm discovering that part of the codebase so I have no idea what I am doing :p)

I tried to copy and paste the solution given in the issue to make it multi-process but it didn't seem to work, but it's very likely that I did it wrong.

@as-cii
Member
as-cii commented Nov 28, 2016

Do you know what is the function to send an asynchronous message? (First time I'm discovering that part of the codebase so I have no idea what I am doing :p)

I think you should be able to use the call method in https://github.com/atom/atom/tree/master/src/ipc-helpers.js to invoke an instance method defined in https://github.com/atom/atom/tree/master/src/main-process/atom-window.coffee. You can see examples of this in https://github.com/atom/atom/tree/master/src/application-delegate.coffee where, for instance, we use ipcHelpers.call('window-method', 'minimize') to asynchronously call AtomWindow.prototype.minimize. 👍

@vjeux vjeux Flush DOM before displaying context menu
Released under CC0
cd781b9
@vjeux
Contributor
vjeux commented Nov 29, 2016 edited

O M G, this works!

So good and way less hacky <3 Thanks!

@as-cii

One minor comment and we can 🚢 this. Thanks again, @vjeux!

src/main-process/atom-window.coffee
+ @openContextMenu(menuTemplate)
+
+ openContextMenu: (menuTemplate) ->
+ ContextMenu = require './context-menu'
@as-cii
as-cii Dec 5, 2016 Member

What do you think about declaring a ContextMenu variable at the top of the file, and change this line so that we require ./context-menu only once? Like:

# at the top...
ContextMenu = null

# this line…
ContextMenu ?= require('./context-menu')

Not a huge deal, but should save some cycles when opening context menus.

@as-cii
as-cii approved these changes Dec 6, 2016 View changes
@as-cii as-cii 🎨
eb0b48f
@as-cii as-cii merged commit 847ca8f into master Dec 6, 2016

4 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
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
continuous-integration/travis-ci/push The Travis CI build passed
Details
@as-cii as-cii deleted the fb-vjeux-flush-context-menu branch Dec 6, 2016
@as-cii
Member
as-cii commented Dec 6, 2016

I went ahead and performed the changes, thanks again @vjeux!

@as-cii
Member
as-cii commented Dec 6, 2016

After testing this out on master, it doesn't seem like it works on macOS. Can you double check it works as expected on your machine?

@vjeux
Contributor
vjeux commented Dec 6, 2016

/facepalm

I checked in a the triple nested request animation frame on Nuclide (which is working) and forgot about it. So the Nuclide fix was applied and I thought this pull request was working, when indeed it isn't. Sorry about that :(

https://github.com/facebook/nuclide/blob/master/pkg/nuclide-file-tree/lib/main.js#L75-L95

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