Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Fake selection container was not appended to the new fake selection editable when fake selection changes #1679

Merged
merged 4 commits into from
Feb 19, 2019
Merged

Fake selection container was not appended to the new fake selection editable when fake selection changes #1679

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
539a0aa
Fix: Fake selection container was not appended to the new fake select…
scofalik Feb 18, 2019
cae80cb
Tests: Re-written test so it is less prone to browser quirks.
scofalik Feb 18, 2019
8bc959e
Merge branch 'master' into t/ckeditor5/1523
jodator Feb 18, 2019
4dcbfa3
Simplified the logic.
Reinmar Feb 19, 2019
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
5 changes: 4 additions & 1 deletion src/view/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,10 @@ export default class Renderer {
container.appendChild( domDocument.createTextNode( '\u00A0' ) );
}

// Add fake container if not already added.
if ( container.parentElement && container.parentElement != domRoot ) {
Copy link
Contributor Author

@scofalik scofalik Feb 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This if (and below if) could simply be

if ( container.parentElement ) {
    container.remove();
}

domRoot.appendChild( container );

So simpler, but I am not sure if we want to remove and append dom element (container) if there's no need for it. Can it break something? If not, we can go with simpler solution.

Copy link
Member

Choose a reason for hiding this comment

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

I'd want to avoid moving it around. That may do some strange things to focus. I even considered leaving just one line: domRoot.appendChild( container ); (and no conditions), but I'm afraid what it may do. It depends on how it's implemented internally in the browsers.

However, if you feel braver than I, then you can test it :D

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 may feel brave but I also feel lazy.

Copy link
Contributor

@jodator jodator Feb 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICS:

domRoot.appendChild( container );
  1. It works (checked the manual tests for tables & widgets, including inline widgets in tables)
  2. Tests pass
  3. Memory tests for editor-classic passes
  4. No increase in detached DOM nodes (I was worrying that it may happen).

Tested on Chrome only though.

ps.: to be clear:
selection_201

Copy link
Member

Choose a reason for hiding this comment

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

Tested on Chrome only though.

Which means nothing ;) It has to be tested thoroughly in all browsers. That's why I'd choose the safe path myself.

container.remove( container );
}

if ( !container.parentElement ) {
domRoot.appendChild( container );
}
Expand Down
55 changes: 36 additions & 19 deletions tests/view/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import View from '../../src/view/view';
import ViewElement from '../../src/view/element';
import ViewEditableElement from '../../src/view/editableelement';
import ViewContainerElement from '../../src/view/containerelement';
import ViewAttributeElement from '../../src/view/attributeelement';
import ViewText from '../../src/view/text';
Expand Down Expand Up @@ -112,10 +113,12 @@ describe( 'Renderer', () => {
} );

describe( 'render', () => {
let viewRoot, domRoot, selectionEditable;
let viewRoot, domRoot;

beforeEach( () => {
viewRoot = new ViewElement( 'div' );
viewRoot = new ViewEditableElement( 'div' );
viewRoot.getFillerOffset = () => null;

domRoot = document.createElement( 'div' );
document.body.appendChild( domRoot );

Expand All @@ -127,16 +130,7 @@ describe( 'Renderer', () => {

selection._setTo( null );

selectionEditable = viewRoot;

renderer.isFocused = true;

// Fake selection editable - it is needed to render selection properly.
Object.defineProperty( selection, 'editableElement', {
get() {
return selectionEditable;
}
} );
} );

afterEach( () => {
Expand Down Expand Up @@ -422,8 +416,6 @@ describe( 'Renderer', () => {
} );

it( 'should not care about filler if there is no DOM', () => {
selectionEditable = null;

const { view: viewP, selection: newSelection } = parse(
'<container:p>foo<attribute:b>[]</attribute:b>bar</container:p>' );

Expand Down Expand Up @@ -1202,11 +1194,10 @@ describe( 'Renderer', () => {
domSelection.removeAllRanges();
domSelection.collapse( domDiv, 0 );

selectionEditable = null;

const viewDiv = new ViewElement( 'div' );
const { view: viewP, selection: newSelection } = parse( '<container:p>fo{o}</container:p>' );

viewRoot._appendChild( viewP );
viewDiv._appendChild( viewP );
selection._setTo( newSelection );

renderer.render();
Expand Down Expand Up @@ -1353,13 +1344,13 @@ describe( 'Renderer', () => {
} );

it( 'should handle focusing element', () => {
selection._setTo( viewRoot, 0 );

const domFocusSpy = testUtils.sinon.spy( domRoot, 'focus' );
const editable = selection.editableElement;

renderer.render();

expect( editable ).to.equal( viewRoot );
expect( domFocusSpy.calledOnce ).to.be.true;
expect( domFocusSpy.called ).to.be.true;
} );

it( 'should not focus editable if isFocues is set to false', () => {
Expand Down Expand Up @@ -1928,6 +1919,32 @@ describe( 'Renderer', () => {
expect( container.style.left ).to.equal( '-9999px' );
} );

it( 'should move fake selection container between editables', () => {
const viewEditable = new ViewEditableElement( 'div' );
viewEditable._appendChild( parse( '<container:p>abc xyz</container:p>' ) );

const domEditable = document.createElement( 'div' );

document.body.appendChild( domEditable );

domConverter.bindElements( domEditable, viewEditable );

renderer.markToSync( 'children', viewEditable );
selection._setTo( selection.getRanges(), { fake: true, label: 'fake selection' } );
renderer.render();

let container = document.getSelection().anchorNode;

expect( domRoot.contains( container ) ).to.be.true;

selection._setTo( viewEditable, 'in', { fake: true, label: 'fake selection' } );
renderer.render();

container = document.getSelection().anchorNode;

expect( domEditable.contains( container ) ).to.be.true;
} );

it( 'should bind fake selection container to view selection', () => {
selection._setTo( selection.getRanges(), { fake: true, label: 'fake selection' } );
renderer.render();
Expand Down