Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevented error thrown when ckeditor5-utils/src/env.ts is used in an environment without window global object #16370

Merged
merged 3 commits into from
May 16, 2024
Merged

Prevented error thrown when ckeditor5-utils/src/env.ts is used in an environment without window global object #16370

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2562311
Fix (utils): Prevented error thrown when `ckeditor5-utils/src/env.ts`…
scofalik May 16, 2024
7f27cc0
Fixed lint issue.
scofalik May 16, 2024
64cd07c
Rename `globalVar` to `global`.
scofalik May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions packages/ckeditor5-utils/src/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@
* @module utils/env
*/

import globalVar from './dom/global';

/**
* Safely returns `userAgent` from browser's navigator API in a lower case.
* If navigator API is not available it will return an empty string.
*/
export function getUserAgent( ): string {
export function getUserAgent(): string {
// In some environments navigator API might not be available.
try {
return navigator.userAgent.toLowerCase();
Expand Down Expand Up @@ -109,7 +111,9 @@ const env: EnvType = {

isBlink: isBlink( userAgent ),

isMediaForcedColors: isMediaForcedColors(),
get isMediaForcedColors() {
return isMediaForcedColors();
},
Comment on lines +114 to +116
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first wanted to get rid of these getters and make it so that the property is resolved immediately (just like all other is* properties above). However, the existing tests were failing and it appeared that we test whether the util reacts to the change of related settings.

Indeed, this setting can change (tested that), so I decided to keep these functions as getters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh... so maybe this should be a function? To indicate that it's a dynamic value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an API change, but from what I understand this is a freshly introduced prop and that'd be fine (not a breaking change, IMO).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other properties of the env object are static (should not change with time) except the two recently introduced. So it'd make sense to think twice before we commit in any direction.


get isMotionReduced() {
return isMotionReduced();
Expand Down Expand Up @@ -218,14 +222,18 @@ export function isRegExpUnicodePropertySupported(): boolean {

/**
* Checks if the user agent has enabled a forced colors mode (e.g. Windows High Contrast mode).
*
* Returns `false` in environments where `window` global object is not available.
*/
export function isMediaForcedColors(): boolean {
return window.matchMedia( '(forced-colors: active)' ).matches;
return globalVar.window.matchMedia ? globalVar.window.matchMedia( '(forced-colors: active)' ).matches : false;
}

/**
* Checks if user enabled "prefers reduced motion" setting in browser.
*
* Returns `false` in environments where `window` global object is not available.
*/
export function isMotionReduced(): boolean {
return window.matchMedia( '(prefers-reduced-motion)' ).matches;
return globalVar.window.matchMedia ? globalVar.window.matchMedia( '(prefers-reduced-motion)' ).matches : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it was a getter, this function didn't crash in described scenario because the getter was never called. However, the getter would still crash if called in an environment without window. To make the solution a bit more future-proof, I decided to add a check for globalVar.window.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should mock all methods and fields in globalVar.window that we utilize in our codebase. Centralizing them would eliminate the need to repeat this logic in multiple places. However, perhaps this task could be deferred until later.

}
67 changes: 65 additions & 2 deletions packages/ckeditor5-utils/tests/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
*/

import env, {
isMac, isWindows, isGecko, isSafari, isiOS, isAndroid, isRegExpUnicodePropertySupported, isBlink, getUserAgent, isMediaForcedColors
isMac, isWindows, isGecko, isSafari, isiOS, isAndroid, isRegExpUnicodePropertySupported, isBlink, getUserAgent,
isMediaForcedColors, isMotionReduced
} from '../src/env.js';

import global from '../src/dom/global.js';
Expand Down Expand Up @@ -64,9 +65,33 @@ describe( 'Env', () => {
} );

describe( 'isMediaForcedColors', () => {
let matchMediaStub;

beforeEach( () => {
matchMediaStub = sinon.stub( global.window, 'matchMedia' );
} );

it( 'is a boolean', () => {
expect( env.isMediaForcedColors ).to.be.a( 'boolean' );
mockMediaForcedColors();

expect( env.isMediaForcedColors ).to.be.true;
} );

it( 'should watch changes in forced colors setting', () => {
mockMediaForcedColors();

expect( env.isMediaForcedColors ).to.be.true;

mockMediaForcedColors( false );

expect( env.isMediaForcedColors ).to.be.false;
} );

function mockMediaForcedColors( enabled = true ) {
return matchMediaStub
.withArgs( '(forced-colors: active)' )
.returns( { matches: enabled } );
}
} );

describe( 'isMotionReduced', () => {
Expand Down Expand Up @@ -326,6 +351,44 @@ describe( 'Env', () => {

expect( isMediaForcedColors() ).to.be.false;
} );

it( 'returns false if window object is not available', () => {
// `global.window` is an empty object if `window` was not available in global space.
const _window = global.window;
global.window = {};

expect( isMediaForcedColors() ).to.be.false;

global.window = _window;
} );
} );

describe( 'isMotionReduced()', () => {
Mati365 marked this conversation as resolved.
Show resolved Hide resolved
it( 'returns true if the document media query matches prefers-reduced-motion', () => {
testUtils.sinon.stub( global.window, 'matchMedia' )
.withArgs( '(prefers-reduced-motion)' )
.returns( { matches: true } );

expect( isMotionReduced() ).to.be.true;
} );

it( 'returns false if the document media query does not match prefers-reduced-motion', () => {
testUtils.sinon.stub( global.window, 'matchMedia' )
.withArgs( '(prefers-reduced-motion)' )
.returns( { matches: false } );

expect( isMotionReduced() ).to.be.false;
} );

it( 'returns false if window object is not available', () => {
// `global.window` is an empty object if `window` was not available in global space.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a better idea how to mock, please suggest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I can think of is the use of before/after() so it's properly cleaned up on a fail, but it's a simple test so I don't see this as a must-have.

const _window = global.window;
global.window = {};

expect( isMotionReduced() ).to.be.false;

global.window = _window;
} );
} );

describe( 'isRegExpUnicodePropertySupported()', () => {
Expand Down