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

Commit 93639d0

Browse files
ReinmarReinmar
authored
Merge pull request #990 from ckeditor/t/986
Fix: `LiveSelection` will not read attributes from object element's children. Closes #986.
2 parents 56347d1 + ac82135 commit 93639d0

File tree

2 files changed

+147
-77
lines changed

2 files changed

+147
-77
lines changed

‎src/model/liveselection.js‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,7 @@ export default class LiveSelection extends Selection {
556556
*/
557557
_getSurroundingAttributes() {
558558
const position = this.getFirstPosition();
559+
const schema = this._document.schema;
559560

560561
let attrs = null;
561562

@@ -564,11 +565,16 @@ export default class LiveSelection extends Selection {
564565
const range = this.getFirstRange();
565566

566567
// ...look for a first character node in that range and take attributes from it.
567-
for ( const item of range ) {
568+
for ( const value of range ) {
569+
// If the item is an object, we don't want to get attributes from its children.
570+
if ( value.item.is( 'element' ) && schema.objects.has( value.item.name ) ) {
571+
break;
572+
}
573+
568574
// This is not an optimal solution because of https://github.com/ckeditor/ckeditor5-engine/issues/454.
569575
// It can be done better by using `break;` instead of checking `attrs === null`.
570-
if ( item.type == 'text' && attrs === null ) {
571-
attrs = item.item.getAttributes();
576+
if ( value.type == 'text' && attrs === null ) {
577+
attrs = value.item.getAttributes();
572578
}
573579
}
574580
} else {

‎tests/model/liveselection.js‎

Lines changed: 138 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -672,7 +672,7 @@ describe( 'LiveSelection', () => {
672672
} );
673673
} );
674674

675-
describe( 'attributes interface', () => {
675+
describe( 'attributes', () => {
676676
let fullP, emptyP, rangeInFullP, rangeInEmptyP;
677677

678678
beforeEach( () => {
@@ -688,7 +688,7 @@ describe( 'LiveSelection', () => {
688688
rangeInEmptyP = new Range( new Position( root, [ 1, 0 ] ), new Position( root, [ 1, 0 ] ) );
689689
} );
690690

691-
describe( 'setAttribute', () => {
691+
describe( 'setAttribute()', () => {
692692
it( 'should store attribute if the selection is in empty node', () => {
693693
selection.setRanges( [ rangeInEmptyP ] );
694694
selection.setAttribute( 'foo', 'bar' );
@@ -699,7 +699,7 @@ describe( 'LiveSelection', () => {
699699
} );
700700
} );
701701

702-
describe( 'setAttributesTo', () => {
702+
describe( 'setAttributesTo()', () => {
703703
it( 'should fire change:attribute event with correct parameters', done => {
704704
selection.setAttributesTo( { foo: 'bar', abc: 'def' } );
705705

@@ -737,7 +737,7 @@ describe( 'LiveSelection', () => {
737737
} );
738738
} );
739739

740-
describe( 'removeAttribute', () => {
740+
describe( 'removeAttribute()', () => {
741741
it( 'should remove attribute set on the text fragment', () => {
742742
selection.setRanges( [ rangeInFullP ] );
743743
selection.setAttribute( 'foo', 'bar' );
@@ -759,7 +759,7 @@ describe( 'LiveSelection', () => {
759759
} );
760760
} );
761761

762-
describe( 'clearAttributes', () => {
762+
describe( 'clearAttributes()', () => {
763763
it( 'should remove all stored attributes if the selection is in empty node', () => {
764764
selection.setRanges( [ rangeInEmptyP ] );
765765
selection.setAttribute( 'foo', 'bar' );
@@ -774,102 +774,166 @@ describe( 'LiveSelection', () => {
774774
expect( emptyP.hasAttribute( LiveSelection._getStoreAttributeKey( 'abc' ) ) ).to.be.false;
775775
} );
776776
} );
777-
} );
778777

779-
describe( 'update attributes on direct range change', () => {
780-
beforeEach( () => {
781-
root.insertChildren( 0, [
782-
new Element( 'p', { p: true } ),
783-
new Text( 'a', { a: true } ),
784-
new Element( 'p', { p: true } ),
785-
new Text( 'b', { b: true } ),
786-
new Text( 'c', { c: true } ),
787-
new Element( 'p', [], [
788-
new Text( 'd', { d: true } )
789-
] ),
790-
new Element( 'p', { p: true } ),
791-
new Text( 'e', { e: true } )
792-
] );
778+
describe( '_getStoredAttributes()', () => {
779+
it( 'should return no values if there are no ranges in selection', () => {
780+
const values = Array.from( selection._getStoredAttributes() );
781+
782+
expect( values ).to.deep.equal( [] );
783+
} );
793784
} );
794785

795-
it( 'if selection is a range, should find first character in it and copy it\'s attributes', () => {
796-
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 5 ] ) ) ] );
786+
describe( 'are updated on a direct range change', () => {
787+
beforeEach( () => {
788+
root.insertChildren( 0, [
789+
new Element( 'p', { p: true } ),
790+
new Text( 'a', { a: true } ),
791+
new Element( 'p', { p: true } ),
792+
new Text( 'b', { b: true } ),
793+
new Text( 'c', { c: true } ),
794+
new Element( 'p', [], [
795+
new Text( 'd', { d: true } )
796+
] ),
797+
new Element( 'p', { p: true } ),
798+
new Text( 'e', { e: true } )
799+
] );
800+
} );
797801

798-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'b', true ] ] );
802+
it( 'if selection is a range, should find first character in it and copy it\'s attributes', () => {
803+
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 5 ] ) ) ] );
799804

800-
// Step into elements when looking for first character:
801-
selection.setRanges( [ new Range( new Position( root, [ 5 ] ), new Position( root, [ 7 ] ) ) ] );
805+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'b', true ] ] );
802806

803-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
804-
} );
807+
// Step into elements when looking for first character:
808+
selection.setRanges( [ new Range( new Position( root, [ 5 ] ), new Position( root, [ 7 ] ) ) ] );
805809

806-
it( 'if selection is collapsed it should seek a character to copy that character\'s attributes', () => {
807-
// Take styles from character before selection.
808-
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 2 ] ) ) ] );
809-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
810+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
811+
} );
810812

811-
// If there are none,
812-
// Take styles from character after selection.
813-
selection.setRanges( [ new Range( new Position( root, [ 3 ] ), new Position( root, [ 3 ] ) ) ] );
814-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'b', true ] ] );
813+
it( 'if selection is collapsed it should seek a character to copy that character\'s attributes', () => {
814+
// Take styles from character before selection.
815+
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 2 ] ) ) ] );
816+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
817+
818+
// If there are none,
819+
// Take styles from character after selection.
820+
selection.setRanges( [ new Range( new Position( root, [ 3 ] ), new Position( root, [ 3 ] ) ) ] );
821+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'b', true ] ] );
822+
823+
// If there are none,
824+
// Look from the selection position to the beginning of node looking for character to take attributes from.
825+
selection.setRanges( [ new Range( new Position( root, [ 6 ] ), new Position( root, [ 6 ] ) ) ] );
826+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'c', true ] ] );
827+
828+
// If there are none,
829+
// Look from the selection position to the end of node looking for character to take attributes from.
830+
selection.setRanges( [ new Range( new Position( root, [ 0 ] ), new Position( root, [ 0 ] ) ) ] );
831+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
832+
833+
// If there are no characters to copy attributes from, use stored attributes.
834+
selection.setRanges( [ new Range( new Position( root, [ 0, 0 ] ), new Position( root, [ 0, 0 ] ) ) ] );
835+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [] );
836+
} );
815837

816-
// If there are none,
817-
// Look from the selection position to the beginning of node looking for character to take attributes from.
818-
selection.setRanges( [ new Range( new Position( root, [ 6 ] ), new Position( root, [ 6 ] ) ) ] );
819-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'c', true ] ] );
838+
it( 'should overwrite any previously set attributes', () => {
839+
selection.collapse( new Position( root, [ 5, 0 ] ) );
820840

821-
// If there are none,
822-
// Look from the selection position to the end of node looking for character to take attributes from.
823-
selection.setRanges( [ new Range( new Position( root, [ 0 ] ), new Position( root, [ 0 ] ) ) ] );
824-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
841+
selection.setAttribute( 'x', true );
842+
selection.setAttribute( 'y', true );
825843

826-
// If there are no characters to copy attributes from, use stored attributes.
827-
selection.setRanges( [ new Range( new Position( root, [ 0, 0 ] ), new Position( root, [ 0, 0 ] ) ) ] );
828-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [] );
829-
} );
844+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ], [ 'x', true ], [ 'y', true ] ] );
830845

831-
it( 'should overwrite any previously set attributes', () => {
832-
selection.collapse( new Position( root, [ 5, 0 ] ) );
846+
selection.collapse( new Position( root, [ 1 ] ) );
833847

834-
selection.setAttribute( 'x', true );
835-
selection.setAttribute( 'y', true );
848+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
849+
} );
836850

837-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ], [ 'x', true ], [ 'y', true ] ] );
851+
it( 'should fire change:attribute event', () => {
852+
const spy = sinon.spy();
853+
selection.on( 'change:attribute', spy );
838854

839-
selection.collapse( new Position( root, [ 1 ] ) );
855+
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 5 ] ) ) ] );
840856

841-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'a', true ] ] );
842-
} );
857+
expect( spy.calledOnce ).to.be.true;
858+
} );
843859

844-
it( 'should fire change:attribute event', () => {
845-
const spy = sinon.spy();
846-
selection.on( 'change:attribute', spy );
860+
it( 'should not fire change:attribute event if attributes did not change', () => {
861+
selection.collapse( new Position( root, [ 5, 0 ] ) );
847862

848-
selection.setRanges( [ new Range( new Position( root, [ 2 ] ), new Position( root, [ 5 ] ) ) ] );
863+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
849864

850-
expect( spy.calledOnce ).to.be.true;
865+
const spy = sinon.spy();
866+
selection.on( 'change:attribute', spy );
867+
868+
selection.collapse( new Position( root, [ 5, 1 ] ) );
869+
870+
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
871+
expect( spy.called ).to.be.false;
872+
} );
851873
} );
852874

853-
it( 'should not fire change:attribute event if attributes did not change', () => {
854-
selection.collapse( new Position( root, [ 5, 0 ] ) );
875+
// #986
876+
describe( 'are not inherited from the inside of object elements', () => {
877+
beforeEach( () => {
878+
doc.schema.registerItem( 'image' );
879+
doc.schema.allow( { name: 'image', inside: '$root' } );
880+
doc.schema.allow( { name: 'image', inside: '$block' } );
881+
doc.schema.allow( { name: '$inline', inside: 'image' } );
882+
doc.schema.objects.add( 'image' );
855883

856-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
884+
doc.schema.registerItem( 'caption' );
885+
doc.schema.allow( { name: '$inline', inside: 'caption' } );
886+
doc.schema.allow( { name: 'caption', inside: 'image' } );
887+
doc.schema.allow( { name: '$text', attributes: 'bold', inside: 'caption' } );
888+
} );
857889

858-
const spy = sinon.spy();
859-
selection.on( 'change:attribute', spy );
890+
it( 'ignores attributes inside an object if selection contains that object', () => {
891+
setData( doc, '<p>[<image><$text bold="true">Caption for the image.</$text></image>]</p>' );
860892

861-
selection.collapse( new Position( root, [ 5, 1 ] ) );
893+
const liveSelection = LiveSelection.createFromSelection( selection );
862894

863-
expect( Array.from( selection.getAttributes() ) ).to.deep.equal( [ [ 'd', true ] ] );
864-
expect( spy.called ).to.be.false;
865-
} );
866-
} );
895+
expect( liveSelection.hasAttribute( 'bold' ) ).to.equal( false );
896+
} );
897+
898+
it( 'ignores attributes inside an object if selection contains that object (deeper structure)', () => {
899+
setData( doc, '<p>[<image><caption><$text bold="true">Caption for the image.</$text></caption></image>]</p>' );
900+
901+
const liveSelection = LiveSelection.createFromSelection( selection );
902+
903+
expect( liveSelection.hasAttribute( 'bold' ) ).to.equal( false );
904+
} );
905+
906+
it( 'ignores attributes inside an object if selection contains that object (block level)', () => {
907+
setData( doc, '<p>foo</p>[<image><$text bold="true">Caption for the image.</$text></image>]<p>foo</p>' );
908+
909+
const liveSelection = LiveSelection.createFromSelection( selection );
910+
911+
expect( liveSelection.hasAttribute( 'bold' ) ).to.equal( false );
912+
} );
913+
914+
it( 'reads attributes from text even if the selection contains an object', () => {
915+
setData( doc, '<p>x<$text bold="true">[bar</$text><image></image>foo]</p>' );
916+
917+
const liveSelection = LiveSelection.createFromSelection( selection );
918+
919+
expect( liveSelection.getAttribute( 'bold' ) ).to.equal( true );
920+
} );
921+
922+
it( 'reads attributes when the entire selection inside an object', () => {
923+
setData( doc, '<p><image><caption><$text bold="true">[bar]</$text></caption></image></p>' );
867924

868-
describe( '_getStoredAttributes', () => {
869-
it( 'should return no values if there are no ranges in selection', () => {
870-
const values = Array.from( selection._getStoredAttributes() );
925+
const liveSelection = LiveSelection.createFromSelection( selection );
871926

872-
expect( values ).to.deep.equal( [] );
927+
expect( liveSelection.getAttribute( 'bold' ) ).to.equal( true );
928+
} );
929+
930+
it( 'stops reading attributes if selection starts with an object', () => {
931+
setData( doc, '<p>[<image></image><$text bold="true">bar]</$text></p>' );
932+
933+
const liveSelection = LiveSelection.createFromSelection( selection );
934+
935+
expect( liveSelection.hasAttribute( 'bold' ) ).to.equal( false );
936+
} );
873937
} );
874938
} );
875939
} );

0 commit comments

Comments
 (0)