-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This dependency is no longer needed now that we use Node's built-in APIs
In Chrome 66+ it seems that getComputedStyle().fontWeight returns the computed numeric value of the style instead of the original descriptive name. We now look for value '700' which corresponds to the value of 'bold'.
This comment has been minimized.
This comment has been minimized.
I also think we should consider looking into how text is being rendered in this new version before shipping it. Below you can find a zoomed-in screenshot of rendering the word Atom on Electron 3 (this pull request)VS Code on Electron 3 (latest published version)Atom on Electron 2 (Atom 1.38.0-beta0)You can reproduce the above tests by using Fira Code Retina, 15px. I suspect some of the styling we do in |
Thanks, @as-cii. I've added this to the task list in the PR body. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@rafeca and I think this pull request is ready to be merged. The sooner we do this, the more time we will have to catch bugs before it goes to Beta. Please, note that we are purposely ignoring #18915 for now because we haven't been able to reproduce it, and it's unclear how it would be related to the Electron upgrade. Feedback on this is super welcome! Especially after this pull request gets merged, it would be useful to get as many 👀 on Nightly as possible to catch potential regressions that we didn't notice. /cc: @atom/maintainers |
Have you had a chance to run through the regression test plan? Once we've tested out all the items on that checklist, I'm 👍 on merging this pull request. It's a pretty long checklist, so I'd be perfectly happy to divide and conquer with y'all. |
Note: The |
Check for regressions experienced in previous upgrades
Text Input/KeybindingsSee how to setup keyboard layouts.
UI
Other
|
I've done the sanity checks on Linux and haven't found any regression related to this upgrade. I have some notes though about some items:
I've tried with a several million lines file and the editor becomes almost unresponsive (good thing is that Atom shows a warning saying that the file is too big and may cause issues with the editor). When I tried to scroll to ~800k line Atom ended up crashing. This same behaviour happened on current Atom beta, so it's not an electron issue.
I'm not able to test this one because I don't have a wheel mouse.
I'm not able to test this one because I don't have a wheel mouse. |
Probable regression: #19372 |
Description of the Change
#18603 and #18815 upgraded Atom from Electron 2 to Electron 3, but we unfortunately experienced frequent crashes following those changes. As a short-term solution, #18908 reverted those changes.
This pull request restores the changes from #18603 and #18815. (It does so by reverting the changes from #18908.) Before we merge this pull request, we'll need to resolve the crashes that led to us having to revert the previous Electron 3 upgrade.
Issues to resolve
I think these issues are all caused by the upgrade to Electron 3. If we discover that they're not directly related to the Electron 3 upgrade, we can remove them from this list and address them separately.
Fixes #6666.
Fixes #18535.
Fixes atom/tabs#544