Skip to content

Commit

Permalink
Merge pull request #332 from ckeditor/i/6573
Browse files Browse the repository at this point in the history 
Other: The `getPositionedAncestor()` helper should use `offsetParent` instead of `getComputedStyle()` for performance reasons. Closes #6573.

MINOR BREAKING CHANGE: The `getPositionedAncestor()` helper will no longer return the passed element when it is positioned.
  • Loading branch information
@Reinmar
Reinmar authored Apr 17, 2020
2 parents 9c89bb9 + 50bb75c commit f8a9363
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 36 deletions.
12 changes: 6 additions & 6 deletions packages/ckeditor5-utils/src/dom/getpositionedancestor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ import global from './global';
* @returns {HTMLElement|null}
*/
export default function getPositionedAncestor( element ) {
while ( element && element.tagName.toLowerCase() != 'html' ) {
if ( global.window.getComputedStyle( element ).position != 'static' ) {
return element;
}
if ( !element || !element.parentNode ) {
return null;
}

element = element.parentElement;
if ( element.offsetParent === global.document.body ) {
return null;
}

return null;
return element.offsetParent;
}
2 changes: 1 addition & 1 deletion packages/ckeditor5-utils/src/dom/position.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export function getOptimalPosition( { element, target, positions, limiter, fitIn
limiter = limiter();
}

const positionedElementAncestor = getPositionedAncestor( element.parentElement );
const positionedElementAncestor = getPositionedAncestor( element );
const elementRect = new Rect( element );
const targetRect = new Rect( target );

Expand Down
6 changes: 3 additions & 3 deletions packages/ckeditor5-utils/tests/dom/getpositionedancestor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ describe( 'getPositionedAncestor', () => {
expect( getPositionedAncestor() ).to.be.null;
} );

it( 'should return null when there is no parent', () => {
it( 'should return null when there is no positioned ancestor', () => {
expect( getPositionedAncestor( element ) ).to.be.null;
} );

it( 'should consider passed element', () => {
it( 'should not consider the passed element', () => {
element.style.position = 'relative';

expect( getPositionedAncestor( element ) ).to.equal( element );
expect( getPositionedAncestor( element ) ).to.be.null;
} );

it( 'should find the positioned ancestor (direct parent)', () => {
Expand Down
95 changes: 69 additions & 26 deletions packages/ckeditor5-utils/tests/dom/position.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ describe( 'getOptimalPosition()', () => {

beforeEach( () => {
testUtils.sinon.stub( window, 'getComputedStyle' );
window.getComputedStyle.callThrough();

stubWindow( {
innerWidth: 10000,
Expand All @@ -120,6 +121,15 @@ describe( 'getOptimalPosition()', () => {
} );
} );

afterEach( () => {
element.remove();
target.remove();

if ( limiter ) {
limiter.remove();
}
} );

it( 'should work when the target is a Function', () => {
setElementTargetPlayground();

Expand Down Expand Up @@ -177,6 +187,10 @@ describe( 'getOptimalPosition()', () => {
describe( 'positioned element parent', () => {
let parent;

afterEach( () => {
parent.remove();
} );

it( 'should return coordinates', () => {
stubWindow( {
innerWidth: 10000,
Expand All @@ -196,7 +210,7 @@ describe( 'getOptimalPosition()', () => {
position: 'absolute'
} );

element.parentElement = parent;
parent.appendChild( element );

assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
top: -900,
Expand All @@ -219,20 +233,34 @@ describe( 'getOptimalPosition()', () => {
bottom: 10,
left: 0,
width: 10,
height: 10,
scrollTop: 100,
scrollLeft: 200
height: 10
}, {
position: 'absolute',
borderLeftWidth: '20px',
borderTopWidth: '40px'
borderTopWidth: '40px',
overflow: 'scroll',
width: '10px',
height: '10px',
background: 'red'
} );

Object.assign( element.style, {
width: '20px',
height: '20px',
marginTop: '100px',
marginLeft: '200px',
background: 'green'
} );

element.parentElement = parent;
parent.appendChild( element );
document.body.appendChild( parent );

parent.scrollLeft = 200;
parent.scrollTop = 100;

assertPosition( { element, target, positions: [ attachLeftBottom ] }, {
top: 160,
left: 260,
top: 200,
left: 280,
name: 'left-bottom'
} );
} );
Expand Down Expand Up @@ -268,6 +296,10 @@ describe( 'getOptimalPosition()', () => {
describe( 'with a limiter', () => {
beforeEach( setElementTargetLimiterPlayground );

afterEach( () => {
limiter.remove();
} );

it( 'should work when the limiter is a Function', () => {
assertPosition( {
element, target,
Expand Down Expand Up @@ -316,7 +348,7 @@ describe( 'getOptimalPosition()', () => {

// https://github.com/ckeditor/ckeditor5-utils/issues/148
it( 'should return coordinates (#3)', () => {
limiter.parentNode = getElement( {
const parentNode = getElement( {
top: 100,
left: 0,
bottom: 110,
Expand All @@ -325,6 +357,9 @@ describe( 'getOptimalPosition()', () => {
height: 10
} );

parentNode.appendChild( limiter );
document.body.appendChild( parentNode );

assertPosition( {
element, target, limiter,
positions: [ attachRightBottom, attachLeftBottom ]
Expand All @@ -333,10 +368,12 @@ describe( 'getOptimalPosition()', () => {
left: 10,
name: 'right-bottom'
} );

parentNode.remove();
} );

it( 'should return the first position that completely fits in the limiter', () => {
element = getElement( {
const element = getElement( {
top: 0,
right: 5,
bottom: 5,
Expand All @@ -352,6 +389,8 @@ describe( 'getOptimalPosition()', () => {
left: -5,
name: 'left-bottom'
} );

element.remove();
} );
} );

Expand Down Expand Up @@ -465,7 +504,7 @@ describe( 'getOptimalPosition()', () => {
} );

it( 'should return the very first coordinates if limiter does not fit into the viewport', () => {
limiter = getElement( {
const limiter = getElement( {
top: -100,
right: -80,
bottom: -80,
Expand All @@ -483,10 +522,12 @@ describe( 'getOptimalPosition()', () => {
left: 10,
name: 'right-bottom'
} );

limiter.remove();
} );

it( 'should prefer positions fitting entirely into the viewport', () => {
target = getElement( {
const target = getElement( {
top: 100,
right: 35,
bottom: 120,
Expand All @@ -503,6 +544,8 @@ describe( 'getOptimalPosition()', () => {
left: 35,
name: 'right-bottom'
} );

target.remove();
} );
} );

Expand Down Expand Up @@ -578,23 +621,23 @@ describe( 'getOptimalPosition()', () => {
// second position intersects less with limiter but more with viewport and it should not be ignored.
//
// Target is outside viewport to force checking all positions, not only those completely fitting in viewport.
limiter = getElement( {
const limiter = getElement( {
top: -100,
right: 100,
bottom: 100,
left: -100,
width: 200,
height: 200
} );
target = getElement( {
const target = getElement( {
top: -30,
right: 80,
bottom: -10,
left: 60,
width: 20,
height: 20
} );
element = getElement( {
const element = getElement( {
top: 0,
right: 200,
bottom: 200,
Expand All @@ -610,6 +653,10 @@ describe( 'getOptimalPosition()', () => {
],
fitInViewport: true
}, 'right-bottom' );

limiter.remove();
target.remove();
element.remove();
} );
} );
} );
Expand All @@ -631,18 +678,14 @@ function assertPositionName( options, expected ) {
// @private
// @param {Object} properties A set of properties for the element.
// @param {Object} styles A set of styles in `window.getComputedStyle()` format.
function getElement( properties = {}, styles = {} ) {
const element = {
tagName: 'div',
scrollLeft: 0,
scrollTop: 0,
ownerDocument: document
};
function getElement( rect = {}, styles = {} ) {
expect( rect.right - rect.left ).to.equal( rect.width, 'getElement incorrect horizontal values' );
expect( rect.bottom - rect.top ).to.equal( rect.height, 'getElement incorrect vertical values' );

expect( properties.right - properties.left ).to.equal( properties.width, 'getElement incorrect horizontal values' );
expect( properties.bottom - properties.top ).to.equal( properties.height, 'getElement incorrect vertical values' );
const element = document.createElement( 'div' );
document.body.appendChild( element );

Object.assign( element, properties );
sinon.stub( element, 'getBoundingClientRect' ).returns( rect );

if ( !styles.borderLeftWidth ) {
styles.borderLeftWidth = '0px';
Expand All @@ -652,7 +695,7 @@ function getElement( properties = {}, styles = {} ) {
styles.borderTopWidth = '0px';
}

window.getComputedStyle.withArgs( element ).returns( styles );
Object.assign( element.style, styles );

return element;
}
Expand Down

0 comments on commit f8a9363

Please sign in to comment.