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

#7542: Widget type around UI should not be selectable by triple-click. #7599

Merged
merged 5 commits into from
Jul 10, 2020
Merged

#7542: Widget type around UI should not be selectable by triple-click. #7599

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac274b7
Widget type around UI should not be selectable by triple-click.
niegowski Jul 9, 2020
752b8dc
Updated triple-click handler inside nested editable to include Gecko …
niegowski Jul 10, 2020
77addc9
Merge branch 'master' into i/7542
oleq Jul 10, 2020
390f127
Added test for triple-click in nested editable with multiple paragraphs.
niegowski Jul 10, 2020
96aa2e7
Updated test for gecko browsers.
niegowski Jul 10, 2020
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
1 change: 1 addition & 0 deletions packages/ckeditor5-widget/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"@ckeditor/ckeditor5-heading": "^20.0.0",
"@ckeditor/ckeditor5-horizontal-line": "^20.0.0",
"@ckeditor/ckeditor5-image": "^20.0.0",
"@ckeditor/ckeditor5-link": "^20.0.0",
"@ckeditor/ckeditor5-media-embed": "^20.0.0",
"@ckeditor/ckeditor5-paragraph": "^20.0.0",
"@ckeditor/ckeditor5-table": "^20.0.0",
Expand Down
9 changes: 6 additions & 3 deletions packages/ckeditor5-widget/src/widget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,15 @@ export default class Widget extends Plugin {
// 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 && domEventData.domEvent.detail >= 3 ) {
if ( ( env.isSafari || env.isGecko ) && domEventData.domEvent.detail >= 3 ) {
const mapper = editor.editing.mapper;
const modelElement = mapper.toModelElement( element );
const viewElement = element.is( 'attributeElement' ) ?
element.findAncestor( element => !element.is( 'attributeElement' ) ) : element;
Copy link
Member

Choose a reason for hiding this comment

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

What if there's a widget > nested editable > block-quote > link structure? Feels like viewElement will be the blockquote and the entire nested editable will not be selected. I think isInsideNestedEditable() could become getClosestNestedEditable() and you could re-use its output here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in that situation entire content of that content editable would be selected instead of single paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that table cells are also nested editables and triple-click action is expected to select current block only, not entire content editable.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, my bad.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have another test then that checks only the closest non-attribute is selected and not the entire nested editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

const modelElement = mapper.toModelElement( viewElement );

domEventData.preventDefault();

this.editor.model.change( writer => {
domEventData.preventDefault();
writer.setSelection( modelElement, 'in' );
} );
}
Expand Down
28 changes: 27 additions & 1 deletion packages/ckeditor5-widget/tests/widget-integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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 Widget from '../src/widget';
import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';

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

return ClassicEditor.create( editorElement, { plugins: [ Paragraph, Widget, Typing ] } )
return ClassicEditor.create( editorElement, { plugins: [ Paragraph, Widget, Typing, LinkEditing ] } )
.then( newEditor => {
editor = newEditor;
model = editor.model;
Expand Down Expand Up @@ -148,6 +149,31 @@ describe( 'Widget - integration', () => {
expect( getModelData( model ) ).to.equal( '<widget><nested>[foo bar]</nested></widget>' );
} );

it( 'should select the entire nested editable if triple clicked on link', () => {
setModelData( model, '[]<widget><nested>foo <$text linkHref="abc">bar</$text></nested></widget>' );

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

viewDocument.fire( 'mousedown', domEventDataMock );

sinon.assert.called( preventDefault );

expect( getViewData( view ) ).to.equal(
'<div class="ck-widget" contenteditable="false">' +
'<figcaption contenteditable="true">{foo <a href="abc">bar</a>]</figcaption>' +
'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
'</div>'
);
expect( getModelData( model ) ).to.equal( '<widget><nested>[foo <$text linkHref="abc">bar</$text>]</nested></widget>' );
} );

it( 'should select proper nested editable if triple clicked', () => {
setModelData( model, '[]<widget><nested>foo</nested><nested>bar</nested></widget>' );

Expand Down