From 5d6ef2057d04836b58b874c83376fd57ad910151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Fri, 17 Apr 2020 09:41:06 +0200 Subject: [PATCH 1/2] Other: Remove `env.isEdge`. Closes ckeditor/ckeditor5#6202. Remove some special cases for Edge, as since it's Chromium-based now, it behaves closer to others. --- .../src/imageupload/imageuploadprogress.js | 4 +-- packages/ckeditor5-image/tests/image.js | 4 --- .../ckeditor5-image/tests/image/converters.js | 4 --- .../tests/image/imageediting.js | 4 --- .../tests/imagecaption/imagecaptionediting.js | 4 --- .../tests/imagestyle/imagestyleediting.js | 6 ---- .../ui/textalternativeformview.js | 3 -- .../ckeditor5-image/tests/imagetoolbar.js | 4 --- .../tests/imageupload/imageuploadediting.js | 26 ++-------------- .../tests/imageupload/imageuploadprogress.js | 31 ------------------- packages/ckeditor5-image/tests/integration.js | 4 --- 11 files changed, 4 insertions(+), 90 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js b/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js index 1d6976ab372..5a1ef306b56 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js +++ b/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js @@ -12,7 +12,6 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository'; import uploadingPlaceholder from '../../theme/icons/image_placeholder.svg'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import { getViewImgFromWidget } from '../image/utils'; import '../../theme/imageuploadprogress.css'; @@ -104,8 +103,7 @@ export default class ImageUploadProgress extends Plugin { return; } - // Because in Edge there is no way to show fancy animation of completeIcon we need to skip it. - if ( status == 'complete' && fileRepository.loaders.get( uploadId ) && !env.isEdge ) { + if ( status == 'complete' && fileRepository.loaders.get( uploadId ) ) { _showCompleteIcon( viewFigure, viewWriter, editor.editing.view ); } diff --git a/packages/ckeditor5-image/tests/image.js b/packages/ckeditor5-image/tests/image.js index 3fda91795c2..7f9780b04af 100644 --- a/packages/ckeditor5-image/tests/image.js +++ b/packages/ckeditor5-image/tests/image.js @@ -11,7 +11,6 @@ import ImageTextAlternative from '../src/imagetextalternative'; import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'Image', () => { @@ -20,9 +19,6 @@ describe( 'Image', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - editorElement = global.document.createElement( 'div' ); global.document.body.appendChild( editorElement ); diff --git a/packages/ckeditor5-image/tests/image/converters.js b/packages/ckeditor5-image/tests/image/converters.js index 96f7a92d1a7..e24deb8fb45 100644 --- a/packages/ckeditor5-image/tests/image/converters.js +++ b/packages/ckeditor5-image/tests/image/converters.js @@ -13,7 +13,6 @@ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtest import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'Image converters', () => { @@ -22,9 +21,6 @@ describe( 'Image converters', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - return VirtualTestEditor.create() .then( newEditor => { editor = newEditor; diff --git a/packages/ckeditor5-image/tests/image/imageediting.js b/packages/ckeditor5-image/tests/image/imageediting.js index f518845c5a6..a9eabda1248 100644 --- a/packages/ckeditor5-image/tests/image/imageediting.js +++ b/packages/ckeditor5-image/tests/image/imageediting.js @@ -14,7 +14,6 @@ import { getData as getModelData, setData as setModelData } from '@ckeditor/cked import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import { isImageWidget } from '../../src/image/utils'; import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'ImageEditing', () => { @@ -23,9 +22,6 @@ describe( 'ImageEditing', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - return VirtualTestEditor .create( { plugins: [ ImageEditing ] diff --git a/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js b/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js index 65cbc32b36a..a54997d3f98 100644 --- a/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js +++ b/packages/ckeditor5-image/tests/imagecaption/imagecaptionediting.js @@ -13,7 +13,6 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'ImageCaptionEditing', () => { @@ -22,9 +21,6 @@ describe( 'ImageCaptionEditing', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - return VirtualTestEditor .create( { plugins: [ ImageCaptionEditing, ImageEditing, UndoEditing, Paragraph ] diff --git a/packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js b/packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js index 7442e6003ec..cf7b4ef5b35 100644 --- a/packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js +++ b/packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js @@ -12,18 +12,12 @@ import { getData as getModelData, setData as setModelData } from '@ckeditor/cked import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import env from '@ckeditor/ckeditor5-utils/src/env'; describe( 'ImageStyleEditing', () => { let editor, model, document, viewDocument; testUtils.createSinonSandbox( 'ImageStyleEditing' ); - beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - } ); - afterEach( () => { editor.destroy(); } ); diff --git a/packages/ckeditor5-image/tests/imagetextalternative/ui/textalternativeformview.js b/packages/ckeditor5-image/tests/imagetextalternative/ui/textalternativeformview.js index d2fcda2ce4c..6d1993a5595 100644 --- a/packages/ckeditor5-image/tests/imagetextalternative/ui/textalternativeformview.js +++ b/packages/ckeditor5-image/tests/imagetextalternative/ui/textalternativeformview.js @@ -13,7 +13,6 @@ import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker'; import FocusCycler from '@ckeditor/ckeditor5-ui/src/focuscycler'; import ViewCollection from '@ckeditor/ckeditor5-ui/src/viewcollection'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import env from '@ckeditor/ckeditor5-utils/src/env'; describe( 'TextAlternativeFormView', () => { let view; @@ -21,8 +20,6 @@ describe( 'TextAlternativeFormView', () => { testUtils.createSinonSandbox(); beforeEach( () => { - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - view = new TextAlternativeFormView( { t: () => {} } ); } ); diff --git a/packages/ckeditor5-image/tests/imagetoolbar.js b/packages/ckeditor5-image/tests/imagetoolbar.js index 38ac4e82ff9..54209000b4c 100644 --- a/packages/ckeditor5-image/tests/imagetoolbar.js +++ b/packages/ckeditor5-image/tests/imagetoolbar.js @@ -14,7 +14,6 @@ import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import View from '@ckeditor/ckeditor5-ui/src/view'; import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'ImageToolbar', () => { @@ -23,9 +22,6 @@ describe( 'ImageToolbar', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - editorElement = global.document.createElement( 'div' ); global.document.body.appendChild( editorElement ); diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index 097e10f51fa..8a38c83fbee 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -23,13 +23,11 @@ import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData, stringify as stringifyView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; describe( 'ImageUploadEditing', () => { // eslint-disable-next-line max-len const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; - const isEdgeEnv = env.isEdge; let adapterMocks = []; let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader; @@ -49,15 +47,6 @@ describe( 'ImageUploadEditing', () => { } beforeEach( () => { - if ( isEdgeEnv ) { - sinon.stub( window, 'File' ).callsFake( () => { - return { name: 'file.jpg' }; - } ); - } - - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - sinon.stub( env, 'isEdge' ).get( () => false ); - sinon.stub( window, 'FileReader' ).callsFake( () => { nativeReaderMock = new NativeFileReaderMock(); @@ -816,14 +805,7 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should not upload and remove image when `File` constructor is not supported', done => { - if ( isEdgeEnv ) { - // Since on Edge `File` is already stubbed, restore it to it native form so that exception will be thrown. - sinon.restore(); - // Since all stubs were restored, re-stub `scrollToTheSelection`. - sinon.stub( editor.editing.view, 'scrollToTheSelection' ).callsFake( () => {} ); - } else { - sinon.stub( window, 'File' ).throws( 'Function expected.' ); - } + sinon.stub( window, 'File' ).throws( 'Function expected.' ); const notification = editor.plugins.get( Notification ); @@ -857,8 +839,7 @@ describe( 'ImageUploadEditing', () => { ); } ); - // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. - ( isEdgeEnv ? it.skip : it )( 'should get file extension from base64 string', done => { + it( 'should get file extension from base64 string', done => { setModelData( model, '[]foo' ); const clipboardHtml = ``; @@ -883,8 +864,7 @@ describe( 'ImageUploadEditing', () => { } ); } ); - // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. - ( isEdgeEnv ? it.skip : it )( 'should use fallback file extension', done => { + it( 'should use fallback file extension', done => { setModelData( model, '[]foo' ); const clipboardHtml = ``; diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadprogress.js b/packages/ckeditor5-image/tests/imageupload/imageuploadprogress.js index 9db02f34a12..9f1b0d90966 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadprogress.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadprogress.js @@ -20,7 +20,6 @@ import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-util import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import svgPlaceholder from '../../theme/icons/image_placeholder.svg'; -import env from '@ckeditor/ckeditor5-utils/src/env'; describe( 'ImageUploadProgress', () => { const imagePlaceholder = encodeURIComponent( svgPlaceholder ); @@ -44,9 +43,6 @@ describe( 'ImageUploadProgress', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - testUtils.sinon.stub( window, 'FileReader' ).callsFake( () => { nativeReaderMock = new NativeFileReaderMock(); @@ -333,31 +329,4 @@ describe( 'ImageUploadProgress', () => { ']' ); } ); - - it( 'should not create completeIcon element when browser is Microsoft Edge', done => { - testUtils.sinon.stub( env, 'isEdge' ).get( () => true ); - - setModelData( model, '[]foo' ); - editor.execute( 'imageUpload', { file: createNativeFileMock() } ); - - model.document.once( 'change', () => { - model.document.once( 'change', () => { - try { - expect( getViewData( view ) ).to.equal( - '[
' + - '' + - '
]

foo

' - ); - - done(); - } catch ( err ) { - done( err ); - } - }, { priority: 'lowest' } ); - - loader.file.then( () => adapterMock.mockSuccess( { default: 'image.png' } ) ); - } ); - - loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); - } ); } ); diff --git a/packages/ckeditor5-image/tests/integration.js b/packages/ckeditor5-image/tests/integration.js index 587364247ca..6c5825d4139 100644 --- a/packages/ckeditor5-image/tests/integration.js +++ b/packages/ckeditor5-image/tests/integration.js @@ -11,7 +11,6 @@ import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import Image from '../src/image'; import ImageToolbar from '../src/imagetoolbar'; import View from '@ckeditor/ckeditor5-ui/src/view'; -import env from '@ckeditor/ckeditor5-utils/src/env'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; describe( 'ImageToolbar integration', () => { @@ -21,9 +20,6 @@ describe( 'ImageToolbar integration', () => { testUtils.createSinonSandbox(); beforeEach( () => { - // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. - testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); - editorElement = global.document.createElement( 'div' ); global.document.body.appendChild( editorElement ); From 48f219266a76811ee07530e2d455f3ebeb4cf209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 20 Apr 2020 13:52:39 +0200 Subject: [PATCH 2/2] Other: Remove the Edge-specific fallback for image upload. ckeditor/ckeditor5#6202. Agreed at https://github.com/ckeditor/ckeditor5-utils/pull/333#issuecomment-615275702 --- .../ckeditor5-image/src/imageupload/utils.js | 22 ++----------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/utils.js b/packages/ckeditor5-image/src/imageupload/utils.js index e39ac3efbb2..22cc43861a8 100644 --- a/packages/ckeditor5-image/src/imageupload/utils.js +++ b/packages/ckeditor5-image/src/imageupload/utils.js @@ -44,9 +44,9 @@ export function fetchLocalImage( image ) { const mimeType = getImageMimeType( blob, imageSrc ); const ext = mimeType.replace( 'image/', '' ); const filename = `image.${ ext }`; - const file = createFileFromBlob( blob, filename, mimeType ); + const file = new File( [ blob ], filename, { type: mimeType } ); - file ? resolve( file ) : reject(); + resolve( file ); } ) .catch( reject ); } ); @@ -82,21 +82,3 @@ function getImageMimeType( blob, src ) { return 'image/jpeg'; } } - -// Creates a `File` instance from the given `Blob` instance using the specified file name. -// -// @param {Blob} blob The `Blob` instance from which the file will be created. -// @param {String} filename The file name used during the file creation. -// @param {String} mimeType The file MIME type. -// @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. -function createFileFromBlob( blob, filename, mimeType ) { - try { - return new File( [ blob ], filename, { type: mimeType } ); - } catch ( err ) { - // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. - // However, the `File` function is present (so cannot be checked with `!window.File` or `typeof File === 'function'`), but - // calling it with `new File( ... )` throws an error. This try-catch prevents that. Also when the function will - // be implemented correctly in Edge the code will start working without any changes (see #247). - return null; - } -}