From 9262e1f1efc899470fcc44b4c99f082df0d206aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 6 Nov 2023 16:28:14 +0100 Subject: [PATCH] (core) Fixing bug with collapsed custom widget. Summary: Fix for a bug. Custom widget when collapsed and expanded was disconnecting from Grist, as WidgetFrame was disposed to early. Test Plan: Added new Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D4109 --- app/client/components/CustomView.ts | 8 ++++- app/client/components/WidgetFrame.ts | 20 ++++++------ test/nbrowser/ViewLayoutCollapse.ts | 46 +++++++++++++++++++++++++--- test/nbrowser/gristUtils.ts | 11 ++++++- 4 files changed, 69 insertions(+), 16 deletions(-) diff --git a/app/client/components/CustomView.ts b/app/client/components/CustomView.ts index f1569b13..e9a256bb 100644 --- a/app/client/components/CustomView.ts +++ b/app/client/components/CustomView.ts @@ -242,7 +242,7 @@ export class CustomView extends Disposable { const {baseUrl, access, showAfterReady, widgetId, pluginId} = options; const documentSettings = this.gristDoc.docData.docSettings(); const readonly = this.gristDoc.isReadonly.get(); - return grains.create(WidgetFrame, { + const frame = WidgetFrame.create(null, { url: baseUrl || this.getEmptyWidgetPage(), widgetId, pluginId, @@ -304,6 +304,12 @@ export class CustomView extends Disposable { gristDoc: this.gristDoc, }); + // Can't use dom.create() because it seems buggy in this context. This dom will be detached + // and attached several times, and dom.create() doesn't seem to handle that well as it returns an + // array of nodes (comment, node, comment) and it somehow breaks the dispose order. Collapsed widgets + // relay on a correct order of dispose, and are detaching nodes just before they are disposed, so if + // the order is wrong, the node is disposed without being detached first. + return grains.update(frame.buildDom(), dom.autoDispose(frame)); } } diff --git a/app/client/components/WidgetFrame.ts b/app/client/components/WidgetFrame.ts index 6a710af8..71558f46 100644 --- a/app/client/components/WidgetFrame.ts +++ b/app/client/components/WidgetFrame.ts @@ -185,17 +185,17 @@ export class WidgetFrame extends DisposableWithEvents { public buildDom() { const onElem = this._options.onElem ?? ((el: HTMLIFrameElement) => el); - return onElem( - (this._iframe = dom( - 'iframe', - dom.style('visibility', use => use(this._visible) ? 'visible' : 'hidden'), - dom.cls('clipboard_focus'), - dom.cls('custom_view'), - dom.attr('src', use => this._getUrl(use(this._widget))), - hooks.iframeAttributes, - testId('ready', this._readyCalled), - )) + this._iframe = dom( + 'iframe', + dom.style('visibility', use => use(this._visible) ? 'visible' : 'hidden'), + dom.cls('clipboard_focus'), + dom.cls('custom_view'), + dom.attr('src', use => this._getUrl(use(this._widget))), + hooks.iframeAttributes, + testId('ready', this._readyCalled), + self => void onElem(self), ); + return this._iframe; } private _getUrl(widget: ICustomWidget|null): string { diff --git a/test/nbrowser/ViewLayoutCollapse.ts b/test/nbrowser/ViewLayoutCollapse.ts index ff1ea299..b1643e66 100644 --- a/test/nbrowser/ViewLayoutCollapse.ts +++ b/test/nbrowser/ViewLayoutCollapse.ts @@ -4,11 +4,12 @@ import {getCollapsedSection, openCollapsedSectionMenu} from 'test/nbrowser/ViewL import {assert, driver, Key, WebElement, WebElementPromise} from 'mocha-webdriver'; import {arrayRepeat} from 'app/plugin/gutil'; import {addStatic, serveSomething} from 'test/server/customUtil'; +import {AccessLevel} from 'app/common/CustomWidget'; const GAP = 16; // Distance between buttons representing collapsed widgets. describe("ViewLayoutCollapse", function() { - this.timeout(40000); + this.timeout('50s'); const cleanup = setupTestSuite(); gu.bigScreen(); let session: gu.Session; @@ -19,6 +20,45 @@ describe("ViewLayoutCollapse", function() { await gu.openPage("Overview"); }); + it('fix: custom widget should restart when added back after collapsing', async function() { + const revert = await gu.begin(); + + // Add custom section. + await gu.addNewPage('Table', 'Companies'); + await gu.addNewSection('Custom', 'Companies', { selectBy: 'COMPANIES'}); + + // Serve custom widget. + const widgetServer = await serveSomething(app => { + addStatic(app); + }); + cleanup.addAfterAll(widgetServer.shutdown); + await gu.openWidgetPanel(); + await gu.setWidgetUrl(widgetServer.url + '/probe/index.html'); + await gu.widgetAccess(AccessLevel.full); + + // Collapse it. + await collapseByMenu('COMPANIES Custom'); + + // Now restore its position. + await addToMainByMenu('COMPANIES Custom'); + + // Collapsed widget used to lost connection with Grist as it was disposed to early. + // Make sure that this widget can call the API. + await gu.doInIframe(async () => { + await gu.waitToPass(async () => { + assert.equal(await driver.find('#output').getText(), + `["Companies","Investments","Companies_summary_category_code","Investments_summary_funded_year",` + + `"Investments_summary_Company_category_code_funded_year","Investments_summary_Company_category_code"]` + ); + }); + }); + + + // Make sure we don't have an error. + await gu.checkForErrors(); + await revert(); + }); + it('fix: custom widget should not throw errors when collapsed', async function() { const revert = await gu.begin(); @@ -33,9 +73,7 @@ describe("ViewLayoutCollapse", function() { cleanup.addAfterAll(widgetServer.shutdown); await gu.openWidgetPanel(); await gu.setWidgetUrl(widgetServer.url + '/probe/index.html'); - await driver.find('.test-config-widget-access .test-select-open').click(); - await driver.findContent('.test-select-menu li', 'Full document access').click(); - await gu.waitForServer(); + await gu.widgetAccess(AccessLevel.full); // Collapse it. await collapseByMenu('COMPANIES Custom'); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 1f22cc48..a6453a22 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -894,8 +894,17 @@ export async function importUrlDialog(url: string): Promise { * Executed passed function in the context of given iframe, and then switching back to original context * */ -export async function doInIframe(iframe: WebElement, func: () => Promise) { +export async function doInIframe(func: () => Promise): Promise +export async function doInIframe(iframe: WebElement, func: () => Promise): Promise +export async function doInIframe(frameOrFunc: WebElement|(() => Promise), func?: () => Promise): Promise { try { + let iframe: WebElement; + if (!func) { + func = frameOrFunc as () => Promise; + iframe = await driver.findWait('iframe', 5000); + } else { + iframe = frameOrFunc as WebElement; + } await driver.switchTo().frame(iframe); return await func(); } finally {