Jelajahi Sumber

Internal (engine): Added support for excluding imageLoaded event from ImageLoadObserver for ignored element. Also added some review corrections.

Marek Lewandowski 5 tahun lalu
induk
melakukan
b5d1596e50

+ 2 - 0
packages/ckeditor5-engine/package.json

@@ -27,11 +27,13 @@
   "devDependencies": {
     "@ckeditor/ckeditor5-basic-styles": "^23.0.0",
     "@ckeditor/ckeditor5-block-quote": "^23.0.0",
+    "@ckeditor/ckeditor5-clipboard": "^23.0.0",
     "@ckeditor/ckeditor5-core": "^23.0.0",
     "@ckeditor/ckeditor5-editor-classic": "^23.0.0",
     "@ckeditor/ckeditor5-enter": "^23.0.0",
     "@ckeditor/ckeditor5-essentials": "^23.0.0",
     "@ckeditor/ckeditor5-heading": "^23.0.0",
+    "@ckeditor/ckeditor5-image": "^23.0.0",
     "@ckeditor/ckeditor5-link": "^23.0.0",
     "@ckeditor/ckeditor5-list": "^23.0.0",
     "@ckeditor/ckeditor5-paragraph": "^23.0.0",

+ 18 - 13
packages/ckeditor5-engine/tests/manual/tickets/4600/1.html

@@ -1,41 +1,46 @@
 <style>
 	.simple-widget-container {
-		margin: 1em 0;
+		margin: 10px 0;
 		font-family: sans-serif;
 	}
 
 	.simple-widget-element {
 		display: flex;
-		height: 14em;
 	}
 
 	.simple-widget-element>fieldset {
-		display: flex;
-		align-items: center;
-		margin: 3em;
+		margin: 18px;
 		width: 50%;
-		border: 1px solid #ddd;
+		border: 1px solid #ccc;
 		border-radius: 4px;
 	}
 
+	.simple-widget-element>fieldset>* {
+		margin: 8px 0;
+	}
+
 	.simple-widget-element>fieldset>legend {
-		font-size: 0.85em;
-		padding: .2em 2em;
-		border: 1px solid #ddd;
+		font-size: 13px;
+		padding: 4px 16px;
+		border: 1px solid #ccc;
 		border-radius: 4px;
 		background: #fff;
 	}
 
 	.simple-widget-element>fieldset>input {
-		flex: 1;
-		margin: 1em 1em 1em 0;
+		width: 100%;
+		box-sizing: border-box;
 	}
 
 	.simple-widget-element>fieldset>button {
-		padding: .2em 3em;
+		padding: 4px 18px;
+	}
+
+	.simple-widget-element>fieldset>img {
+		display: block;
 	}
 </style>
 
 <div id="editor">
-	<section class="simple-widget-container" />
+	<section class="simple-widget-container"></section>
 </div>

+ 21 - 30
packages/ckeditor5-engine/tests/manual/tickets/4600/1.js

@@ -3,9 +3,10 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* globals console, document, window, Event */
+/* globals console, document, window */
 
 import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials';
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
@@ -19,6 +20,10 @@ import InputObserver from '../../../../src/view/observer/inputobserver';
 import KeyObserver from '../../../../src/view/observer/keyobserver';
 import MouseObserver from '../../../../src/view/observer/mouseobserver';
 import MouseEventsObserver from '@ckeditor/ckeditor5-table/src/tablemouse/mouseeventsobserver';
+import DeleteObserver from '@ckeditor/ckeditor5-typing/src/deleteobserver';
+import ClipboardObserver from '@ckeditor/ckeditor5-clipboard/src/clipboardobserver';
+import EnterObserver from '@ckeditor/ckeditor5-enter/src/enterobserver';
+import ImageLoadObserver from '@ckeditor/ckeditor5-image/src/image/imageloadobserver';
 
 class SimpleWidgetEditing extends Plugin {
 	static get requires() {
@@ -73,11 +78,13 @@ class SimpleWidgetEditing extends Plugin {
 						<legend>Ignored container with <strong>data-cke-ignore-events="true"</strong></legend>
 						<input>
 						<button>Click!</button>
+						<img src="https://placekitten.com/30/30" height="30">
 					</fieldset>
 					<fieldset>
 						<legend>Regular container</legend>
 						<input>
 						<button>Click!</button>
+						<img src="https://placekitten.com/30/30" height="30">
 					</fieldset>
 				`;
 			} );
@@ -98,58 +105,42 @@ class SimpleWidgetEditing extends Plugin {
 			[ InputObserver, [ 'beforeinput' ] ],
 			[ KeyObserver, [ 'keydown', 'keyup' ] ],
 			[ MouseEventsObserver, [ 'mousemove', 'mouseup', 'mouseleave' ] ],
-			[ MouseObserver, [ 'mousedown' ] ]
+			[ MouseObserver, [ 'mousedown' ] ],
+			[ ClipboardObserver, [ 'paste', 'copy', 'cut', 'drop', 'dragover' ] ], // It's inheriting domEventObserver
+			[ DeleteObserver, [ 'delete' ] ], // Is ignored for some reason, even though there's no explicit support.
+			[ EnterObserver, [ 'enter' ] ], // Is ignored for some reason, even though there's no explicit support.
+			[ ImageLoadObserver, [ 'imageLoaded' ] ]
 		] );
 
 		observers.forEach( ( events, observer ) => {
 			view.addObserver( observer );
 
 			events.forEach( eventName => {
-				this.listenTo( view.document, eventName, () => {
-					console.log( `Received ${ eventName } event.` );
+				this.listenTo( view.document, eventName, ( event, eventData ) => {
+					if ( eventName.startsWith( 'mouse' ) ) {
+						console.log( `Received ${ eventName } event.` );
+					} else {
+						console.log( `Received ${ eventName } event. Target: `, eventData.domTarget || eventData.target );
+					}
 				} );
 			} );
 		} );
 	}
 }
 
-class SimpleWidgetUI extends Plugin {}
-
 class SimpleWidget extends Plugin {
 	static get requires() {
-		return [ SimpleWidgetEditing, SimpleWidgetUI ];
+		return [ SimpleWidgetEditing ];
 	}
 }
 
 ClassicEditor
 	.create( document.querySelector( '#editor' ), {
-		plugins: [ Essentials, SimpleWidget ]
+		plugins: [ Essentials, Paragraph, SimpleWidget ]
 	} )
 	.then( editor => {
 		window.editor = editor;
-		addEventDispatcherForButtons( editor, 'click' );
 	} )
 	.catch( error => {
 		console.error( error.stack );
 	} );
-
-function addEventDispatcherForButtons( editor, eventName ) {
-	const view = editor.editing.view;
-	const container = Array
-		.from( view.document.getRoot().getChildren() )
-		.find( element => element.hasClass( 'simple-widget-container' ) );
-
-	view.domConverter
-		.viewToDom( container )
-		.querySelectorAll( 'button' )
-		.forEach( button => {
-			button.addEventListener( 'click', event => {
-				if ( !event.isTrusted ) {
-					return;
-				}
-
-				console.log( `Dispatched ${ eventName } event.` );
-				button.dispatchEvent( new Event( eventName, { bubbles: true } ) );
-			} );
-		} );
-}

+ 44 - 13
packages/ckeditor5-engine/tests/manual/tickets/4600/1.md

@@ -1,19 +1,50 @@
-### Ignoring events fired by certain elements [#4600](https://github.com/ckeditor/ckeditor5/issues/4600)
+## Ignoring events propagation
 
-Events are logged in console.
+It's a test for ignoring events fired in elements with a `data-cke-ignore-events` attribute ([#4600](https://github.com/ckeditor/ckeditor5/issues/4600)).
 
-### Case 1: Events are ignored and they are not handled by default listeners.
-1. Move mouse cursor over a left container named `Ignored container with data-cke-ignore-events="true"`.
-2. When it is already there, start moving it around within container boundaries, start typing in text field and start clicking on text field and button.
-3. Click on the button.
+Events are logged in the browser's console.
 
-**Expected results**: Only then, when the mouse cursor is over the left container, new logs will stop appearing in the console. Clicking inside it, typing in text field and moving mouse cursor inside the container boundaries should not be logged in console. Clicking on a button dispatches the `click` event (the `Dispatched click event` message should be logged), but `Received click event` shouldn't be present in console.
+### Case 1: Events ignoring
 
-One note for `focus` nad `blur` events: they will be logged in console, but only when container lost focus or gets it back, respectively.
+1. Focus the widget by clicking it.
+1. Move mouse cursor over the left container (`Ignored container with data-cke-ignore-events="true"`).
+1. Keep moving the cursor over the container.
 
-### Case 2: Events are not ignored and they are handled by default listeners.
-1. Move mouse cursor over a right container named `Regular container`.
-2. When it is already there, start moving it around within container boundaries, start typing in text field and start clicking on text field and button.
-3. Click on the button.
+	**Expected:** no further "Received mousemove event." logs are added.
 
-**Expected results**: Events should be logged in console. It shouldn't be possible to focus the text field and type anything there. Clicking on a button dispatches the `click` event (the `Dispatched click event` message should be logged) and doubled `Received click event` should be present in console: one from "real" user mouse click and second one from script-generated `click` event.
+1. Click on a "Click!" button in the container.
+
+	**Expected:** No "Received click event." gets logged.
+
+1. Click on the text input in the container.
+
+	**Expected:** Text input gets focused.
+
+1. Type few characters into the focused input.
+
+	**Expected:** Text is inserted. No keyboard events are logged.
+
+Note: you might get unexpected `focus` nad `blur` events in the process, it's a [known issue](https://github.com/ckeditor/ckeditor5/issues/8309).
+
+### Case 2: Container without event ignoring
+
+1. Focus the widget by clicking it.
+1. Move mouse cursor over the right container (`Regular container`).
+1. Keep moving the cursor over the container.
+
+	**Expected:** "Received mousemove event." logs are added.
+
+1. Click on a "Click!" button in the container.
+
+	**Expected:** "Received click event." is logged.
+
+1. Click on the text input in the container.
+
+	**Expected:** Click events are logged. Editor decides what to do with the selection.
+
+### Case 3: `imageLoaded` event
+
+1. Reload the page (test).
+1. Check the console.
+
+	**Expected:** There's only **one** `Received imageLoaded event.` log.

+ 3 - 3
packages/ckeditor5-engine/tests/view/observer/domeventobserver.js

@@ -122,7 +122,7 @@ describe( 'DomEventObserver', () => {
 		expect( evtSpy.called ).to.be.false;
 	} );
 
-	it( 'should not fire event if target is ignored', () => {
+	it( 'should not fire event if the target is ignored', () => {
 		const domElement = document.createElement( 'p' );
 		const domEvent = new MouseEvent( 'click' );
 		const evtSpy = sinon.spy();
@@ -138,10 +138,10 @@ describe( 'DomEventObserver', () => {
 
 		domElement.dispatchEvent( domEvent );
 
+		ignoreEventFromTargetStub.restore();
+
 		expect( ignoreEventFromTargetStub.called ).to.be.true;
 		expect( evtSpy.called ).to.be.false;
-
-		ignoreEventFromTargetStub.restore();
 	} );
 
 	it( 'should fire event if observer is disabled and re-enabled', () => {

+ 4 - 0
packages/ckeditor5-image/src/image/imageloadobserver.js

@@ -27,6 +27,10 @@ export default class ImageLoadObserver extends Observer {
 		this.listenTo( domRoot, 'load', ( event, domEvent ) => {
 			const domElement = domEvent.target;
 
+			if ( this.checkShouldIgnoreEventFromTarget( domElement ) ) {
+				return;
+			}
+
 			if ( domElement.tagName == 'IMG' ) {
 				this._fireEvents( domEvent );
 			}

+ 24 - 0
packages/ckeditor5-image/tests/image/imageloadobserver.js

@@ -109,6 +109,30 @@ describe( 'ImageLoadObserver', () => {
 		sinon.assert.notCalled( spy );
 	} );
 
+	it( 'should not fire `loadImage` event if an image has `data-cke-ignore-events` attribute', () => {
+		const spy = sinon.spy();
+
+		viewDocument.on( 'imageLoaded', spy );
+
+		setData( view, '<img src="/assets/sample.png" data-cke-ignore-events="true" />' );
+
+		domRoot.querySelector( 'img' ).dispatchEvent( new Event( 'load' ) );
+
+		sinon.assert.notCalled( spy );
+	} );
+
+	it( 'should not fire `loadImage` event if an image has an ancestor with `data-cke-ignore-events` attribute', () => {
+		const spy = sinon.spy();
+
+		viewDocument.on( 'imageLoaded', spy );
+
+		setData( view, '<div data-cke-ignore-events="true"><p><img src="/assets/sample.png" /></p></div>' );
+
+		domRoot.querySelector( 'img' ).dispatchEvent( new Event( 'load' ) );
+
+		sinon.assert.notCalled( spy );
+	} );
+
 	it( 'should do nothing with an image when changes are in the other parent', () => {
 		setData(
 			view,