feat: update to Electron 12 #2220

barmac · 2021-04-19T15:16:58Z

This introduces context isolation to the app. Because of that,
the remote APIs have changed. Backend does not expose off
method anymore. Use the return value of on and once instead.

Example:

const subscription = backend.on('client:window-focused', listener);

subscription.cancel();

Closes #1926

barmac · 2021-04-19T15:34:03Z

Let's test it with the build artifacts:

app/lib/preload.js

nikku · 2021-04-19T20:25:19Z

client/src/remote/index.js

@@ -22,14 +22,10 @@ const {
  flags,
  platform,
  plugins: appPlugins,
-  ipcRenderer
+  api


What is api here? Can we give it a more sensible name? Alternative: Construct the Backend directly in appPreload. Not sure what's the benefit to construct it here?

Benefits:

We don't expose the ipcRenderer methods directly.

We can filter events for which send can be used.

So yeah, I could rename it to ipcRenderer again. However, how does the client benefit from knowing about ipcRenderer?

Does the client benefit from knowing something that is called API?

A solution could be to return the instantiated backend from the pre-load function. This way we do not need to pass around these names at all. Let us try to avoid generics like "api" and "descriptor" in our code. We got a lot of these in there already 😉

Let's try that out.

Implemented via 0a84217

nikku

Some comments inlined.

app/lib/preload.js

barmac · 2021-04-20T08:08:55Z

Chromium update shipped with outline changes.

barmac · 2021-04-20T08:11:22Z

Camunda Modeler 4.7 for comparison

andreasgeier · 2021-04-20T09:42:20Z

We should fix the wrong focus outline style.

nikku · 2021-04-20T12:26:57Z

app/lib/preload.js

+  // expose api only once
+  // related to https://github.com/camunda/camunda-modeler/issues/2143
+  if (executed) {
+    return;


Could be helpful to blow up here, so we do not silently discard miss-use.

Suggested change

return;

throw new Error('🚔');

throw new Error('this is recorded! We\'ll track you down!!!!11^^10815').

barmac · 2021-04-20T13:33:07Z

It seems that we cannot have an exactly identical outline, but the outcome looks OK in my opinion.

Screen.Recording.2021-04-20.at.15.30.12.mov

barmac · 2021-04-20T13:34:15Z

Regarding the deployment button, I'll remove the outline as it is not present in 4.7.

barmac · 2021-04-20T13:46:28Z

Some insights about why they changed the outline: https://blog.chromium.org/2020/03/updates-to-form-controls-and-focus.html#Blog1:~:text=A%20More%20Visible%20Focus%20Ring

barmac · 2021-04-20T14:26:09Z

CI fails due to unrelated error 🤪

andreasgeier

There are now unwanted focus outlines on several places in the UI:

Diagrams

Status bar overlay

Decision table

barmac · 2021-04-21T09:42:21Z

We discussed the issue and decided on the following:

change the default color of the outline to a matching one (will be picked up by non-styled plugins)
use "faked" outline for our custom content where we used to base on the user-agent values.

barmac · 2021-04-21T11:46:35Z

CI fails due to unrelated error 🤪

It was because of missing sinon.restore() after I stubbed process.platform.

app/lib/preload.js

nikku

Works for me. Also smoke tested it on Linux 👍

nikku · 2021-04-21T12:24:12Z

Needs approval by @andreasgeier.

This introduces context isolation to the app. Because of that, the remote APIs have changed. Backend does not expose `off` method anymore. Use the return value of `on` and `once` instead. Example: ```javascript const subscription = backend.on('client:window-focused', listener); subscription.cancel(); ``` Closes #1926

* Use custom color for default outline. * Replace outline with box-shadow for custom elements.

barmac · 2021-04-21T12:54:18Z

Found one more event thanks to codecov ;) It should be OK now.

andreasgeier

I did a quick exploratory test through the user interface and found no more outline problems.

bpmn-io-tasks bot added the in progress Currently worked on label Apr 19, 2021

barmac force-pushed the 1926-update-electron branch from 9268434 to 80f7241 Compare April 19, 2021 15:26

barmac requested review from philippfromme, nikku and pinussilvestrus April 19, 2021 15:34

barmac marked this pull request as ready for review April 19, 2021 15:34

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 19, 2021

barmac force-pushed the 1926-update-electron branch from 80f7241 to 94619ee Compare April 19, 2021 15:38

nikku reviewed Apr 19, 2021

View reviewed changes

app/lib/preload.js Outdated Show resolved Hide resolved

nikku reviewed Apr 19, 2021

View reviewed changes

nikku requested changes Apr 19, 2021

View reviewed changes

pinussilvestrus reviewed Apr 20, 2021

View reviewed changes

app/lib/preload.js Show resolved Hide resolved

barmac requested a review from andreasgeier April 20, 2021 08:09

nikku reviewed Apr 20, 2021

View reviewed changes

barmac force-pushed the 1926-update-electron branch from 3246dcc to 22db281 Compare April 20, 2021 14:17

andreasgeier suggested changes Apr 21, 2021

View reviewed changes

barmac force-pushed the 1926-update-electron branch from c7c6f5e to 84674b4 Compare April 21, 2021 10:24

barmac force-pushed the 1926-update-electron branch from 84674b4 to 0a84217 Compare April 21, 2021 11:47

barmac requested a review from nikku April 21, 2021 11:51

barmac requested review from pinussilvestrus and andreasgeier April 21, 2021 11:51

nikku reviewed Apr 21, 2021

View reviewed changes

app/lib/preload.js Show resolved Hide resolved

nikku approved these changes Apr 21, 2021

View reviewed changes

barmac added 4 commits April 21, 2021 14:48

style: update focus indicator

fbcaebb

* Use custom color for default outline. * Replace outline with box-shadow for custom elements.

chore: move backend component entirely to preload script

32b8d0a

fix(preload): add missing event

2a1705f

barmac force-pushed the 1926-update-electron branch from 0a84217 to 2a1705f Compare April 21, 2021 12:53

andreasgeier approved these changes Apr 22, 2021

View reviewed changes

nikku merged commit 91491e2 into develop Apr 22, 2021

nikku deleted the 1926-update-electron branch April 22, 2021 08:06

bpmn-io-tasks bot removed the needs review Review pending label Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update to Electron 12 #2220

feat: update to Electron 12 #2220

barmac commented Apr 19, 2021 •

edited

Loading

barmac commented Apr 19, 2021

nikku Apr 19, 2021

barmac Apr 20, 2021

barmac Apr 20, 2021

nikku Apr 20, 2021

barmac Apr 20, 2021

barmac Apr 21, 2021

nikku left a comment

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

andreasgeier commented Apr 20, 2021

nikku Apr 20, 2021

barmac Apr 20, 2021

barmac Apr 20, 2021

nikku Apr 20, 2021 •

edited

Loading

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

andreasgeier left a comment

barmac commented Apr 21, 2021

barmac commented Apr 21, 2021

nikku left a comment

nikku commented Apr 21, 2021

barmac commented Apr 21, 2021

andreasgeier left a comment

feat: update to Electron 12 #2220

feat: update to Electron 12 #2220

Conversation

barmac commented Apr 19, 2021 • edited Loading

barmac commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

andreasgeier commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

barmac commented Apr 20, 2021

andreasgeier left a comment

Choose a reason for hiding this comment

barmac commented Apr 21, 2021

barmac commented Apr 21, 2021

nikku left a comment

Choose a reason for hiding this comment

nikku commented Apr 21, 2021

barmac commented Apr 21, 2021

andreasgeier left a comment

Choose a reason for hiding this comment

barmac commented Apr 19, 2021 •

edited

Loading

nikku Apr 20, 2021 •

edited

Loading