From be8e44e17e6102817bac5c2f2519ab44610525eb Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Tue, 27 Sep 2022 10:58:19 +0200 Subject: [PATCH 1/5] Add types for styleElements object --- src/theme.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/theme.ts b/src/theme.ts index 54bc42f807..06267b4a9e 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -237,8 +237,8 @@ export async function setTheme(theme?: string): Promise { // look for the stylesheet elements. // styleElements is a map from style name to HTMLLinkElement. - const styleElements = Object.create(null); - const themes = Array.from(document.querySelectorAll('[data-mx-theme]')); + const styleElements: Record = Object.create(null); + const themes = Array.from(document.querySelectorAll('[data-mx-theme]')); themes.forEach(theme => { styleElements[theme.attributes['data-mx-theme'].value.toLowerCase()] = theme; }); From f574247452f6e99fe33fc977eddff0e2e8ec1637 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Tue, 27 Sep 2022 11:02:57 +0200 Subject: [PATCH 2/5] Fix the white/black theme switch in Chrome Chrome doesn't fire twice the load event on a stylesheet when the disabled attribute is toggled (enabled => disabled => enabled) --- src/theme.ts | 56 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/src/theme.ts b/src/theme.ts index 06267b4a9e..f932cfe872 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -279,26 +279,48 @@ export async function setTheme(theme?: string): Promise { resolve(); }; - // turns out that Firefox preloads the CSS for link elements with - // the disabled attribute, but Chrome doesn't. + const isStyleSheetLoaded = () => Boolean( + [...document.styleSheets] + .find(styleSheet => styleSheet?.href === styleElements[stylesheetName].href), + ); - let cssLoaded = false; - - styleElements[stylesheetName].onload = () => { - switchTheme(); - }; - - for (let i = 0; i < document.styleSheets.length; i++) { - const ss = document.styleSheets[i]; - if (ss && ss.href === styleElements[stylesheetName].href) { - cssLoaded = true; - break; + function waitForStyleSheetLoading() { + // turns out that Firefox preloads the CSS for link elements with + // the disabled attribute, but Chrome doesn't. + if (isStyleSheetLoaded()) { + switchTheme(); + return; } + + let counter = 0; + + // In case of theme toggling (white => black => white) + // Chrome doesn't fire the `load` event when the white theme is selected the second times + const intervalId = setInterval(() => { + if (isStyleSheetLoaded()) { + clearInterval(intervalId); + styleElements[stylesheetName].onload = undefined; + styleElements[stylesheetName].onerror = undefined; + switchTheme(); + } + + // Avoid to be stuck in an endless loop if there is an issue in the stylesheet loading + counter++; + if (counter === 5) { + clearInterval(intervalId); + } + }, 100); + + styleElements[stylesheetName].onload = () => { + clearInterval(intervalId); + switchTheme(); + }; + + styleElements[stylesheetName].onerror = () => { + clearInterval(intervalId); + }; } - if (cssLoaded) { - styleElements[stylesheetName].onload = undefined; - switchTheme(); - } + waitForStyleSheetLoading(); }); } From 55450bc0a6b6f0a84ac550f8584c995fd6c6fcd4 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 28 Sep 2022 10:44:32 +0200 Subject: [PATCH 3/5] Use map instead of object for styleElements --- src/theme.ts | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/theme.ts b/src/theme.ts index f932cfe872..a657807704 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -237,13 +237,13 @@ export async function setTheme(theme?: string): Promise { // look for the stylesheet elements. // styleElements is a map from style name to HTMLLinkElement. - const styleElements: Record = Object.create(null); + const styleElements = new Map(); const themes = Array.from(document.querySelectorAll('[data-mx-theme]')); themes.forEach(theme => { - styleElements[theme.attributes['data-mx-theme'].value.toLowerCase()] = theme; + styleElements.set(theme.attributes['data-mx-theme'].value.toLowerCase(), theme); }); - if (!(stylesheetName in styleElements)) { + if (!styleElements.has(stylesheetName)) { throw new Error("Unknown theme " + stylesheetName); } @@ -258,7 +258,8 @@ export async function setTheme(theme?: string): Promise { // having them interact badly... but this causes a flash of unstyled app // which is even uglier. So we don't. - styleElements[stylesheetName].disabled = false; + const styleSheet = styleElements.get(stylesheetName); + styleSheet.disabled = false; return new Promise((resolve) => { const switchTheme = function() { @@ -266,9 +267,9 @@ export async function setTheme(theme?: string): Promise { // theme set request as per https://github.com/vector-im/element-web/issues/5601. // We could alternatively lock or similar to stop the race, but // this is probably good enough for now. - styleElements[stylesheetName].disabled = false; - Object.values(styleElements).forEach((a: HTMLStyleElement) => { - if (a == styleElements[stylesheetName]) return; + styleSheet.disabled = false; + styleElements.forEach(a => { + if (a == styleSheet) return; a.disabled = true; }); const bodyStyles = global.getComputedStyle(document.body); @@ -281,7 +282,7 @@ export async function setTheme(theme?: string): Promise { const isStyleSheetLoaded = () => Boolean( [...document.styleSheets] - .find(styleSheet => styleSheet?.href === styleElements[stylesheetName].href), + .find(_styleSheet => _styleSheet?.href === styleSheet.href), ); function waitForStyleSheetLoading() { @@ -299,8 +300,8 @@ export async function setTheme(theme?: string): Promise { const intervalId = setInterval(() => { if (isStyleSheetLoaded()) { clearInterval(intervalId); - styleElements[stylesheetName].onload = undefined; - styleElements[stylesheetName].onerror = undefined; + styleSheet.onload = undefined; + styleSheet.onerror = undefined; switchTheme(); } @@ -311,12 +312,12 @@ export async function setTheme(theme?: string): Promise { } }, 100); - styleElements[stylesheetName].onload = () => { + styleSheet.onload = () => { clearInterval(intervalId); switchTheme(); }; - styleElements[stylesheetName].onerror = () => { + styleSheet.onerror = () => { clearInterval(intervalId); }; } From ad9cbe93994888e8f8ba88ce3614dc041a698930 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 28 Sep 2022 12:42:40 +0200 Subject: [PATCH 4/5] Add unit tests --- src/theme.ts | 12 +++-- test/theme-test.ts | 111 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 test/theme-test.ts diff --git a/src/theme.ts b/src/theme.ts index a657807704..9d2f836fb4 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -261,7 +261,7 @@ export async function setTheme(theme?: string): Promise { const styleSheet = styleElements.get(stylesheetName); styleSheet.disabled = false; - return new Promise((resolve) => { + return new Promise(((resolve, reject) => { const switchTheme = function() { // we re-enable our theme here just in case we raced with another // theme set request as per https://github.com/vector-im/element-web/issues/5601. @@ -307,21 +307,23 @@ export async function setTheme(theme?: string): Promise { // Avoid to be stuck in an endless loop if there is an issue in the stylesheet loading counter++; - if (counter === 5) { + if (counter === 10) { clearInterval(intervalId); + reject(); } - }, 100); + }, 200); styleSheet.onload = () => { clearInterval(intervalId); switchTheme(); }; - styleSheet.onerror = () => { + styleSheet.onerror = (e) => { clearInterval(intervalId); + reject(e); }; } waitForStyleSheetLoading(); - }); + })); } diff --git a/test/theme-test.ts b/test/theme-test.ts new file mode 100644 index 0000000000..47e42f5a70 --- /dev/null +++ b/test/theme-test.ts @@ -0,0 +1,111 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { setTheme } from "../src/theme"; + +describe('theme', () => { + describe('setTheme', () => { + let lightTheme; + let darkTheme; + + beforeEach(() => { + const styles = [ + { + attributes: { + 'data-mx-theme': { + value: 'light', + }, + }, + disabled: true, + href: 'urlLight', + onload: () => void 0, + }, + { + attributes: { + 'data-mx-theme': { + value: 'dark', + }, + }, + disabled: true, + href: 'urlDark', + onload: () => void 0, + }, + ]; + lightTheme = styles[0]; + darkTheme = styles[1]; + + jest.spyOn(document.body, 'style', 'get').mockReturnValue([] as any); + jest.spyOn(document, 'querySelectorAll').mockReturnValue(styles as any); + }); + + afterEach(() => { + jest.restoreAllMocks(); + jest.useRealTimers(); + }); + + it('should switch theme on onload call', async () => { + // When + await new Promise(resolve => { + setTheme('light').then(resolve); + lightTheme.onload(); + }); + + // Then + expect(lightTheme.disabled).toBe(false); + expect(darkTheme.disabled).toBe(true); + }); + + it('should reject promise on onerror call', () => { + return expect(new Promise(resolve => { + setTheme('light').catch(e => resolve(e)); + lightTheme.onerror('call onerror'); + })).resolves.toBe('call onerror'); + }); + + it('should switch theme if CSS are preloaded', async () => { + // When + jest.spyOn(document, 'styleSheets', 'get').mockReturnValue([lightTheme] as any); + + await setTheme('light'); + + // Then + expect(lightTheme.disabled).toBe(false); + expect(darkTheme.disabled).toBe(true); + }); + + it('should switch theme if CSS is loaded during pooling', async () => { + // When + jest.useFakeTimers(); + await new Promise(resolve => { + setTheme('light').then(resolve); + jest.spyOn(document, 'styleSheets', 'get').mockReturnValue([lightTheme] as any); + jest.advanceTimersByTime(200); + }); + + // Then + expect(lightTheme.disabled).toBe(false); + expect(darkTheme.disabled).toBe(true); + }); + + it('should reject promise if pooling maximum value is reached', () => { + jest.useFakeTimers(); + return new Promise(resolve => { + setTheme('light').catch(resolve); + jest.advanceTimersByTime(200 * 10); + }); + }); + }); +}); From b2c2ef2bd66230fdc39103293320983e73b32e90 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 28 Sep 2022 15:25:16 +0200 Subject: [PATCH 5/5] Test querySelectorAll call --- test/theme-test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/theme-test.ts b/test/theme-test.ts index 47e42f5a70..8e0e6c94e1 100644 --- a/test/theme-test.ts +++ b/test/theme-test.ts @@ -21,6 +21,8 @@ describe('theme', () => { let lightTheme; let darkTheme; + let spyQuerySelectorAll: jest.MockInstance, [selectors: string]>; + beforeEach(() => { const styles = [ { @@ -48,7 +50,7 @@ describe('theme', () => { darkTheme = styles[1]; jest.spyOn(document.body, 'style', 'get').mockReturnValue([] as any); - jest.spyOn(document, 'querySelectorAll').mockReturnValue(styles as any); + spyQuerySelectorAll = jest.spyOn(document, 'querySelectorAll').mockReturnValue(styles as any); }); afterEach(() => { @@ -64,6 +66,8 @@ describe('theme', () => { }); // Then + expect(spyQuerySelectorAll).toHaveBeenCalledWith('[data-mx-theme]'); + expect(spyQuerySelectorAll).toBeCalledTimes(1); expect(lightTheme.disabled).toBe(false); expect(darkTheme.disabled).toBe(true); });