mirror of
https://github.com/Llewellynvdm/nativefier.git
synced 2025-01-03 14:17:29 +00:00
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 9a6c6f870d && 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.
This commit is contained in:
Adam Weeden
GitHub
parent
9d0a2f5b44
commit
113d8448c1
4
.github/manual-test
vendored
4
.github/manual-test
vendored
@ -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
|
||||
|
||||
@ -20,6 +20,6 @@
|
||||
"source-map-support": "^0.5.19"
|
||||
},
|
||||
"devDependencies": {
|
||||
"electron": "^12.0.7"
|
||||
"electron": "^12.0.11"
|
||||
}
|
||||
}
|
||||
|
||||
@ -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<string | jest.DoneCallback>(['DELETE', 'OPTIONS'])(
|
||||
'will not inject for method %s',
|
||||
(method: string, done: jest.DoneCallback) => {
|
||||
test.each<string | jest.DoneCallback>([
|
||||
'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<string | jest.DoneCallback>(['GET', 'PATCH', 'POST', 'PUT'])(
|
||||
'will inject for method %s',
|
||||
(method: string, done: jest.DoneCallback) => {
|
||||
test.each<string | jest.DoneCallback>(['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();
|
||||
|
||||
@ -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<Record<string, string[]>> {
|
||||
// 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;
|
||||
|
||||
Loading…
Reference in New Issue
Block a user