From c3f7edc104df3ad8e325fc1dbfa75a50776cc767 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 13 Jul 2017 15:11:36 +0200 Subject: [PATCH 1/2] Swap underlying editor correctly when calling setModel on editor element Previously, when `setModel` was called, we forgot to update the pointer to the component in the newly supplied editor. This was causing the element to not update in response to model updates but only as a result of focus or visibility changes. We suspect this regressed during the rewrite of the editor rendering layer. With this commit we will now correctly swap the element's underlying editor by updating the component pointer on the newly supplied editor. Also, if the element was already attached to another editor, we will null out the component reference on it, because one instance of `TextEditorElement` can only represent one instance of `TextEditor`. --- spec/text-editor-element-spec.js | 27 +++++++++++++++++++++++++++ src/text-editor-component.js | 4 ++++ 2 files changed, 31 insertions(+) diff --git a/spec/text-editor-element-spec.js b/spec/text-editor-element-spec.js index c92c6f144..a9692bc21 100644 --- a/spec/text-editor-element-spec.js +++ b/spec/text-editor-element-spec.js @@ -201,6 +201,33 @@ describe('TextEditorElement', () => { }) }) + describe('::setModel', () => { + describe('when the element does not have an editor yet', () => { + it('uses the supplied one', () => { + const element = buildTextEditorElement({attach: false}) + const editor = new TextEditor() + element.setModel(editor) + jasmine.attachToDOM(element) + expect(editor.element).toBe(element) + expect(element.getModel()).toBe(editor) + }) + }) + + describe('when the element already has an editor', () => { + it('unbinds it and then swaps it with the supplied one', async () => { + const element = buildTextEditorElement({attach: true}) + const previousEditor = element.getModel() + expect(previousEditor.element).toBe(element) + + const newEditor = new TextEditor() + element.setModel(newEditor) + expect(previousEditor.element).not.toBe(element) + expect(newEditor.element).toBe(element) + expect(element.getModel()).toBe(newEditor) + }) + }) + }) + describe('::onDidAttach and ::onDidDetach', () => it('invokes callbacks when the element is attached and detached', () => { const element = buildTextEditorElement({attach: false}) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 037e45462..a93cbabba 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -174,6 +174,10 @@ class TextEditorComponent { } update (props) { + if (props.model !== this.props.model) { + this.props.model.component = null + props.model.component = this + } this.props = props this.scheduleUpdate() } From 1d42590a5bf8eb9e9fe69fab5eda746df2457905 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 13 Jul 2017 10:12:37 -0600 Subject: [PATCH 2/2] Force legacy scrollbars in text-editor-element-spec --- spec/text-editor-element-spec.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/text-editor-element-spec.js b/spec/text-editor-element-spec.js index a9692bc21..684d160a8 100644 --- a/spec/text-editor-element-spec.js +++ b/spec/text-editor-element-spec.js @@ -9,6 +9,10 @@ describe('TextEditorElement', () => { beforeEach(() => { jasmineContent = document.body.querySelector('#jasmine-content') + // Force scrollbars to be visible regardless of local system configuration + const scrollbarStyle = document.createElement('style') + scrollbarStyle.textContent = '::-webkit-scrollbar { -webkit-appearance: none }' + jasmine.attachToDOM(scrollbarStyle) }) function buildTextEditorElement (options = {}) {