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.

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

Fix triple click issue in paragraph with widget #15011

Merged
97 changes: 81 additions & 16 deletions packages/ckeditor5-widget/src/widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Plugin } from '@ckeditor/ckeditor5-core';

import {
MouseObserver,
TreeWalker,
type DomEventData,
type DowncastSelectionEvent,
type DowncastWriter,
Expand All @@ -19,7 +20,9 @@ import {
type ViewDocumentArrowKeyEvent,
type ViewDocumentFragment,
type ViewDocumentMouseDownEvent,
type ViewElement
type ViewElement,
type Schema,
type Position
} from '@ckeditor/ckeditor5-engine';

import { Delete, type ViewDocumentDeleteEvent } from '@ckeditor/ckeditor5-typing';
Expand Down Expand Up @@ -202,27 +205,20 @@ export default class Widget extends Plugin {
const viewDocument = view.document;
let element: ViewElement | null = domEventData.target;

// Do nothing for single or double click inside nested editable.
if ( isInsideNestedEditable( element ) ) {
// But at least triple click inside nested editable causes broken selection in Safari.
// For such event, we select the entire nested editable element.
// See: https://github.com/ckeditor/ckeditor5/issues/1463.
if ( ( env.isSafari || env.isGecko ) && domEventData.domEvent.detail >= 3 ) {
const mapper = editor.editing.mapper;
const viewElement = element.is( 'attributeElement' ) ?
element.findAncestor( element => !element.is( 'attributeElement' ) )! : element;
const modelElement = mapper.toModelElement( viewElement )!;

// If triple click should select entire paragraph.
if ( domEventData.domEvent.detail >= 3 ) {
if ( this._selectBlockContent( element ) ) {
domEventData.preventDefault();

this.editor.model.change( writer => {
writer.setSelection( modelElement, 'in' );
} );
}

return;
}

// Do nothing for single or double click inside nested editable.
if ( isInsideNestedEditable( element ) ) {
return;
}

// If target is not a widget element - check if one of the ancestors is.
if ( !isWidget( element ) ) {
element = element.findAncestor( isWidget );
Expand All @@ -249,6 +245,38 @@ export default class Widget extends Plugin {
this._setSelectionOverElement( modelElement! );
}

/**
* Selects entire block content, e.g. on triple click it selects entire paragraph.
*/
private _selectBlockContent( element: ViewElement ): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code could be simpler, we could use some existing helper methods. Let's talk about it f2f.

const editor = this.editor;
const model = editor.model;
const mapper = editor.editing.mapper;
const schema = model.schema;

const viewElement = mapper.findMappedViewAncestor( this.editor.editing.view.createPositionAt( element, 0 ) );
const modelElement = findTextBlockAncestor( mapper.toModelElement( viewElement )!, model.schema );

if ( !modelElement ) {
return false;
}

model.change( writer => {
const nextTextBlock = !schema.isLimit( modelElement ) ?
findNextTextBlock( writer.createPositionAfter( modelElement ), schema ) :
null;

const start = writer.createPositionAt( modelElement, 0 );
const end = nextTextBlock ?
writer.createPositionAt( nextTextBlock, 0 ) :
writer.createPositionAt( modelElement, 'end' );

writer.setSelection( writer.createRange( start, end ) );
} );

return true;
}

/**
* Handles {@link module:engine/view/document~Document#event:keydown keydown} events and changes
* the model selection when:
Expand Down Expand Up @@ -479,3 +507,40 @@ function isChild( element: ViewElement, parent: ViewElement | null ) {

return Array.from( element.getAncestors() ).includes( parent );
}

/**
* Returns nearest text block ancestor.
*/
function findTextBlockAncestor( modelElement: Element, schema: Schema ): Element | null {
for ( const element of modelElement.getAncestors( { includeSelf: true, parentFirst: true } ) ) {
if ( schema.checkChild( element as Element, '$text' ) ) {
return element as Element;
}

// Do not go beyond nested editable.
if ( schema.isLimit( element ) && !schema.isObject( element ) ) {
break;
}
}

return null;
}

/**
* Returns next text block where could put selection.
*/
function findNextTextBlock( position: Position, schema: Schema ): Element | null {
const treeWalker = new TreeWalker( { startPosition: position } );

for ( const { item } of treeWalker ) {
if ( schema.isLimit( item ) || !item.is( 'element' ) ) {
return null;
}

if ( schema.checkChild( item, '$text' ) ) {
return item;
}
}

return null;
}
56 changes: 14 additions & 42 deletions packages/ckeditor5-widget/tests/widget-integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Typing from '@ckeditor/ckeditor5-typing/src/typing';
import LinkEditing from '@ckeditor/ckeditor5-link/src/linkediting';
import Image from '@ckeditor/ckeditor5-image/src/image';
import Widget from '../src/widget';
import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';

Expand All @@ -33,7 +34,7 @@ describe( 'Widget - integration', () => {
editorElement = document.createElement( 'div' );
document.body.appendChild( editorElement );

return ClassicEditor.create( editorElement, { plugins: [ Paragraph, Widget, Typing, LinkEditing ] } )
return ClassicEditor.create( editorElement, { plugins: [ Paragraph, Widget, Typing, LinkEditing, Image ] } )
.then( newEditor => {
editor = newEditor;
model = editor.model;
Expand Down Expand Up @@ -205,8 +206,8 @@ describe( 'Widget - integration', () => {
'<div class="ck-widget" contenteditable="false">' +
'<figcaption contenteditable="true">' +
'<p>foo</p>' +
'<p>{foo <a href="abc">bar</a>]</p>' +
'<p>bar</p>' +
'<p>{foo <a href="abc">bar</a></p>' +
'<p>}bar</p>' +
'</figcaption>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</div>'
Expand All @@ -215,8 +216,8 @@ describe( 'Widget - integration', () => {
'<widget>' +
'<nested>' +
'<paragraph>foo</paragraph>' +
'<paragraph>[foo <$text linkHref="abc">bar</$text>]</paragraph>' +
'<paragraph>bar</paragraph>' +
'<paragraph>[foo <$text linkHref="abc">bar</$text></paragraph>' +
'<paragraph>]bar</paragraph>' +
'</nested>' +
'</widget>'
);
Expand Down Expand Up @@ -275,55 +276,26 @@ describe( 'Widget - integration', () => {
expect( getModelData( model ) ).to.equal( '<widget><nested>[foo bar]</nested></widget>' );
} );

it( 'should select the inline widget if triple clicked', () => {
setModelData( model, '<paragraph>Foo<inline-widget>foo bar</inline-widget>Bar</paragraph>' );
it( 'should select image block if triple clicked', () => {
setModelData( model, '[]<imageBlock></imageBlock>' );

const viewParagraph = viewDocument.getRoot().getChild( 0 );
const viewInlineWidget = viewParagraph.getChild( 1 );
const image = viewDocument.getRoot().getChild( 0 );
const preventDefault = sinon.spy();
const domEventDataMock = new DomEventData( view, {
target: view.domConverter.mapViewToDom( viewInlineWidget ),
target: view.domConverter.mapViewToDom( image ),
preventDefault,
detail: 3
} );

viewDocument.fire( 'mousedown', domEventDataMock );

expect( viewDocument.selection.isFake ).to.be.true;
expect( getViewData( view ) ).to.equal(
'<p>Foo[<span class="ck-widget ck-widget_selected" contenteditable="false">foo bar</span>]Bar</p>'
);

expect( getModelData( model ) ).to.equal( '<paragraph>Foo[<inline-widget>foo bar</inline-widget>]Bar</paragraph>' );
} );

it( 'should do nothing for non-Safari and non-Gecko browser', () => {
testUtils.sinon.stub( env, 'isSafari' ).get( () => false );
testUtils.sinon.stub( env, 'isGecko' ).get( () => false );

setModelData( model, '<paragraph>[]</paragraph><widget><nested>foo bar</nested></widget>' );

const viewDiv = viewDocument.getRoot().getChild( 1 );
const viewFigcaption = viewDiv.getChild( 0 );
const preventDefault = sinon.spy();
const domEventDataMock = new DomEventData( view, {
target: view.domConverter.mapViewToDom( viewFigcaption ),
preventDefault,
detail: 4
} );

viewDocument.fire( 'mousedown', domEventDataMock );

sinon.assert.notCalled( preventDefault );

expect( getViewData( view ) ).to.equal(
'<p>[]</p>' +
'<div class="ck-widget" contenteditable="false">' +
'<figcaption contenteditable="true">foo bar</figcaption>' +
'[<figure class="ck-widget ck-widget_selected image" contenteditable="false">' +
'<img></img>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</div>'
'</figure>]'
);

expect( getModelData( model ) ).to.equal( '<paragraph>[]</paragraph><widget><nested>foo bar</nested></widget>' );
expect( getModelData( model ) ).to.equal( '[<imageBlock></imageBlock>]' );
} );
} );
Loading