From d3b198e4148847ac06e9887dee816d5956f857ce Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 4 May 2017 13:13:24 +0200 Subject: [PATCH 1/2] Fix: Rect utility should work for collapsed DOM Ranges. Closes #153. --- src/dom/rect.js | 36 ++++++++++++++++++++++++++++++++---- tests/dom/rect.js | 30 +++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/dom/rect.js b/src/dom/rect.js index 40591a5..b9f22d4 100644 --- a/src/dom/rect.js +++ b/src/dom/rect.js @@ -54,11 +54,39 @@ export default class Rect { enumerable: false } ); - if ( isElement( source ) || isRange( source ) ) { - source = source.getBoundingClientRect(); - } + // Acquires all the rect properties from the passed source. + // + // @private + // @param {ClientRect|module:utils/dom/rect~Rect|Object} source} + const setProperties = source => { + rectProperties.forEach( p => this[ p ] = source[ p ] ); + }; - rectProperties.forEach( p => this[ p ] = source[ p ] ); + if ( isElement( source ) ) { + setProperties( source.getBoundingClientRect() ); + } else if ( isRange( source ) ) { + // Use getClientRects() when the range is collapsed + // https://github.com/ckeditor/ckeditor5-utils/issues/153 + if ( source.collapsed ) { + const rects = source.getClientRects(); + + if ( rects.length ) { + setProperties( rects[ 0 ] ); + } + // If there's no client rects for the Range, use parent container's bounding + // rect instead and adjust rect's width to simulate the actual geometry of such + // range. + // https://github.com/ckeditor/ckeditor5-utils/issues/153 + else { + setProperties( source.startContainer.getBoundingClientRect() ); + this.width = 0; + } + } else { + setProperties( source.getBoundingClientRect() ); + } + } else { + setProperties( source ); + } /** * The "top" value of the rect. diff --git a/tests/dom/rect.js b/tests/dom/rect.js index b1df5cf..2e486b2 100644 --- a/tests/dom/rect.js +++ b/tests/dom/rect.js @@ -41,14 +41,41 @@ describe( 'Rect', () => { assertRect( new Rect( element ), geometry ); } ); - it( 'should accept Range', () => { + it( 'should accept Range (non–collapsed)', () => { const range = document.createRange(); + range.selectNode( document.body ); testUtils.sinon.stub( range, 'getBoundingClientRect' ).returns( geometry ); assertRect( new Rect( range ), geometry ); } ); + // https://github.com/ckeditor/ckeditor5-utils/issues/153 + it( 'should accept Range (collapsed)', () => { + const range = document.createRange(); + + range.collapse(); + testUtils.sinon.stub( range, 'getClientRects' ).returns( [ geometry ] ); + + assertRect( new Rect( range ), geometry ); + } ); + + // https://github.com/ckeditor/ckeditor5-utils/issues/153 + it( 'should accept Range (collapsed, no Range rects available)', () => { + const range = document.createRange(); + const element = document.createElement( 'div' ); + + range.setStart( element, 0 ); + range.collapse(); + testUtils.sinon.stub( range, 'getClientRects' ).returns( [] ); + testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( geometry ); + + const expectedGeometry = Object.assign( {}, geometry ); + expectedGeometry.width = 0; + + assertRect( new Rect( range ), expectedGeometry ); + } ); + it( 'should accept Rect', () => { const sourceRect = new Rect( geometry ); const rect = new Rect( sourceRect ); @@ -481,6 +508,7 @@ describe( 'Rect', () => { it( 'should return the visible rect (Range), partially cropped', () => { range.setStart( ancestorA, 0 ); + range.setEnd( ancestorA, 1 ); testUtils.sinon.stub( range, 'getBoundingClientRect' ).returns( { top: 0, From 273513d55d6f873422af0fa79b3250c9f6a63679 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 4 May 2017 14:32:33 +0200 Subject: [PATCH 2/2] Minor code refactoring. --- src/dom/rect.js | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/dom/rect.js b/src/dom/rect.js index b9f22d4..dc53cc4 100644 --- a/src/dom/rect.js +++ b/src/dom/rect.js @@ -11,8 +11,6 @@ import global from './global'; import isRange from './isrange'; import isElement from '../lib/lodash/isElement'; -const rectProperties = [ 'top', 'right', 'bottom', 'left', 'width', 'height' ]; - /** * A helper class representing a `ClientRect` object, e.g. value returned by * the native `object.getBoundingClientRect()` method. Provides a set of methods @@ -54,38 +52,30 @@ export default class Rect { enumerable: false } ); - // Acquires all the rect properties from the passed source. - // - // @private - // @param {ClientRect|module:utils/dom/rect~Rect|Object} source} - const setProperties = source => { - rectProperties.forEach( p => this[ p ] = source[ p ] ); - }; - if ( isElement( source ) ) { - setProperties( source.getBoundingClientRect() ); + copyRectProperties( this, source.getBoundingClientRect() ); } else if ( isRange( source ) ) { - // Use getClientRects() when the range is collapsed + // Use getClientRects() when the range is collapsed. // https://github.com/ckeditor/ckeditor5-utils/issues/153 if ( source.collapsed ) { const rects = source.getClientRects(); if ( rects.length ) { - setProperties( rects[ 0 ] ); + copyRectProperties( this, rects[ 0 ] ); } // If there's no client rects for the Range, use parent container's bounding // rect instead and adjust rect's width to simulate the actual geometry of such // range. // https://github.com/ckeditor/ckeditor5-utils/issues/153 else { - setProperties( source.startContainer.getBoundingClientRect() ); + copyRectProperties( this, source.startContainer.getBoundingClientRect() ); this.width = 0; } } else { - setProperties( source.getBoundingClientRect() ); + copyRectProperties( this, source.getBoundingClientRect() ); } } else { - setProperties( source ); + copyRectProperties( this, source ); } /** @@ -279,3 +269,16 @@ export default class Rect { } ); } } + +const rectProperties = [ 'top', 'right', 'bottom', 'left', 'width', 'height' ]; + +// Acquires all the rect properties from the passed source. +// +// @private +// @param {module:utils/dom/rect~Rect} rect +// @param {ClientRect|module:utils/dom/rect~Rect|Object} source +function copyRectProperties( rect, source ) { + for ( const p of rectProperties ) { + rect[ p ] = source[ p ]; + } +}