From 113d8448c139be56e97cbf41d9cb83900d50f95b Mon Sep 17 00:00:00 2001 From: Adam Weeden Date: Tue, 15 Jun 2021 09:02:57 -0400 Subject: [PATCH] Fix CSS injection (#1227) This fixes https://github.com/nativefier/nativefier/pull/1222#issuecomment-860913698 , where: 1. When it works (e.g. initial page load), CSS is slower to inject (you can see pages without injected CSS) 2. CSS isn't injected when navigating to a page On both Windows & Linux, a `git revert 9a6c6f870df75ca2a580128d8c429cf5071d6bad && npm run build` fixes the issue. -- I'm still not 100% sure what went wrong, but I suspect that the new version of Electron may not be firing onHeadersReceived for the actual navigation events, and only its child requests. To counteract it, I'm now injecting at the navigation event as well. I was able to reproduce the issue and this does seem to fix it. Please let me know if it does for you as well.. Also I noticed some funkiness in your logs where we're trying to inject on font files. So I realized the method is probably not nearly as important as the content-type, so I've switched blacklist methods to blacklist content-types. --- .github/manual-test | 4 +- app/package.json | 2 +- app/src/helpers/windowHelpers.test.ts | 63 +++++++++++++++++++++------ app/src/helpers/windowHelpers.ts | 41 +++++++++++++---- 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/.github/manual-test b/.github/manual-test index 1bf5ab7..db2dca4 100755 --- a/.github/manual-test +++ b/.github/manual-test @@ -9,9 +9,11 @@ if ! command -v node > /dev/null; then echo "Missing node"; missingDeps=true; fi if [ "$missingDeps" = true ]; then exit 1; fi function launch_app() { - printf '\n*** Running appn' + printf '\n*** Running app\n' if [ "$(uname -s)" = "Darwin" ]; then open -a "$1/$2-darwin-x64/$2.app" + elif [ "$(uname -o)" = "Msys" ]; then + "$1/$2-win32-x64/$2.exe" --verbose else "$1/$2-linux-x64/$2" fi diff --git a/app/package.json b/app/package.json index fb4a08e..cbb18a2 100644 --- a/app/package.json +++ b/app/package.json @@ -20,6 +20,6 @@ "source-map-support": "^0.5.19" }, "devDependencies": { - "electron": "^12.0.7" + "electron": "^12.0.11" } } diff --git a/app/src/helpers/windowHelpers.test.ts b/app/src/helpers/windowHelpers.test.ts index ee12665..0ec7327 100644 --- a/app/src/helpers/windowHelpers.test.ts +++ b/app/src/helpers/windowHelpers.test.ts @@ -97,6 +97,8 @@ describe('createNewTab', () => { }); describe('injectCSS', () => { + jest.setTimeout(10000); + const mockGetCSSToInject: jest.SpyInstance = getCSSToInject as jest.Mock; const mockLogError: jest.SpyInstance = error as jest.Mock; const mockWebContentsInsertCSS: jest.SpyInstance = jest.spyOn( @@ -105,12 +107,13 @@ describe('injectCSS', () => { ); const css = 'body { color: white; }'; - const responseHeaders = { 'x-header': 'value' }; + let responseHeaders; beforeEach(() => { mockGetCSSToInject.mockReset().mockReturnValue(''); mockLogError.mockReset(); mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); + responseHeaders = { 'x-header': 'value', 'content-type': ['test/other'] }; }); afterAll(() => { @@ -181,22 +184,34 @@ describe('injectCSS', () => { ); }); - test.each(['DELETE', 'OPTIONS'])( - 'will not inject for method %s', - (method: string, done: jest.DoneCallback) => { + test.each([ + 'application/json', + 'font/woff2', + 'image/png', + ])( + 'will not inject for content-type %s', + (contentType: string, done: jest.DoneCallback) => { mockGetCSSToInject.mockReturnValue(css); const window = new BrowserWindow(); + responseHeaders['content-type'] = [contentType]; + injectCSS(window); expect(mockGetCSSToInject).toHaveBeenCalled(); - window.webContents.emit('did-navigate'); + expect(window.webContents.emit('did-navigate')).toBe(true); + mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock window.webContents.session.webRequest.send( 'onHeadersReceived', - { responseHeaders, webContents: window.webContents, method }, + { + responseHeaders, + webContents: window.webContents, + url: `test-${contentType}`, + }, (result: HeadersReceivedResponse) => { + // insertCSS will still run once for the did-navigate expect(mockWebContentsInsertCSS).not.toHaveBeenCalled(); expect(result.cancel).toBe(false); expect(result.responseHeaders).toBe(responseHeaders); @@ -206,23 +221,30 @@ describe('injectCSS', () => { }, ); - test.each(['GET', 'PATCH', 'POST', 'PUT'])( - 'will inject for method %s', - (method: string, done: jest.DoneCallback) => { + test.each(['text/html'])( + 'will inject for content-type %s', + (contentType: string, done: jest.DoneCallback) => { mockGetCSSToInject.mockReturnValue(css); const window = new BrowserWindow(); + responseHeaders['content-type'] = [contentType]; + injectCSS(window); expect(mockGetCSSToInject).toHaveBeenCalled(); window.webContents.emit('did-navigate'); + mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock window.webContents.session.webRequest.send( 'onHeadersReceived', - { responseHeaders, webContents: window.webContents, method }, + { + responseHeaders, + webContents: window.webContents, + url: `test-${contentType}`, + }, (result: HeadersReceivedResponse) => { - expect(mockWebContentsInsertCSS).toHaveBeenCalled(); + expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1); expect(result.cancel).toBe(false); expect(result.responseHeaders).toBe(responseHeaders); done(); @@ -247,11 +269,18 @@ describe('injectCSS', () => { expect(mockGetCSSToInject).toHaveBeenCalled(); window.webContents.emit('did-navigate'); + mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock window.webContents.session.webRequest.send( 'onHeadersReceived', - { responseHeaders, webContents: window.webContents, resourceType }, + { + responseHeaders, + webContents: window.webContents, + resourceType, + url: `test-${resourceType}`, + }, (result: HeadersReceivedResponse) => { + // insertCSS will still run once for the did-navigate expect(mockWebContentsInsertCSS).not.toHaveBeenCalled(); expect(result.cancel).toBe(false); expect(result.responseHeaders).toBe(responseHeaders); @@ -272,12 +301,18 @@ describe('injectCSS', () => { expect(mockGetCSSToInject).toHaveBeenCalled(); window.webContents.emit('did-navigate'); + mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock window.webContents.session.webRequest.send( 'onHeadersReceived', - { responseHeaders, webContents: window.webContents, resourceType }, + { + responseHeaders, + webContents: window.webContents, + resourceType, + url: `test-${resourceType}`, + }, (result: HeadersReceivedResponse) => { - expect(mockWebContentsInsertCSS).toHaveBeenCalled(); + expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1); expect(result.cancel).toBe(false); expect(result.responseHeaders).toBe(responseHeaders); done(); diff --git a/app/src/helpers/windowHelpers.ts b/app/src/helpers/windowHelpers.ts index cc42e24..b0eeefd 100644 --- a/app/src/helpers/windowHelpers.ts +++ b/app/src/helpers/windowHelpers.ts @@ -204,6 +204,13 @@ export function injectCSS(browserWindow: BrowserWindow): void { 'browserWindow.webContents.did-navigate', browserWindow.webContents.getURL(), ); + + browserWindow.webContents + .insertCSS(cssToInject) + .catch((err: unknown) => + log.error('browserWindow.webContents.insertCSS', err), + ); + // We must inject css early enough; so onHeadersReceived is a good place. // Will run multiple times, see `did-finish-load` event on the window // that unsets this handler. @@ -213,7 +220,18 @@ export function injectCSS(browserWindow: BrowserWindow): void { details: OnHeadersReceivedListenerDetails, callback: (headersReceivedResponse: HeadersReceivedResponse) => void, ) => { - injectCSSIntoResponse(details, cssToInject) + const contentType = + 'content-type' in details.responseHeaders + ? details.responseHeaders['content-type'][0] + : undefined; + + log.debug('onHeadersReceived', { + contentType, + resourceType: details.resourceType, + url: details.url, + }); + + injectCSSIntoResponse(details, contentType, cssToInject) .then((responseHeaders) => { callback({ cancel: false, @@ -234,28 +252,33 @@ export function injectCSS(browserWindow: BrowserWindow): void { async function injectCSSIntoResponse( details: OnHeadersReceivedListenerDetails, + contentType: string, cssToInject: string, ): Promise> { - // We go with a denylist rather than a whitelist (e.g. only GET/html) + // We go with a denylist rather than a whitelist (e.g. only text/html) // to avoid "whoops I didn't think this should have been CSS-injected" cases - const nonInjectableMethods = ['DELETE', 'OPTIONS']; + const nonInjectableContentTypes = [ + /application\/.*/, + /font\/.*/, + /image\/.*/, + ]; const nonInjectableResourceTypes = ['image', 'script', 'stylesheet', 'xhr']; if ( - nonInjectableMethods.includes(details.method) || + nonInjectableContentTypes.filter((x) => x.exec(contentType)?.length > 0) + ?.length > 0 || nonInjectableResourceTypes.includes(details.resourceType) || !details.webContents ) { log.debug( - `Skipping CSS injection for:\n${details.url}\nwith method ${details.method} and resourceType ${details.resourceType} and content-type ${details.responseHeaders['content-type']}`, + `Skipping CSS injection for:\n${details.url}\nwith resourceType ${details.resourceType} and content-type ${contentType}`, ); return details.responseHeaders; } - log.debug('browserWindow.webContents.session.webRequest.onHeadersReceived', { - details, - contentType: details.responseHeaders['content-type'], - }); + log.debug( + `Injecting CSS for:\n${details.url}\nwith resourceType ${details.resourceType} and content-type ${contentType}`, + ); await details.webContents.insertCSS(cssToInject); return details.responseHeaders;