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

T/148: getOptimalPosition utility should consider limiter ancestors with CSS overflow. Closes #148. #149

Merged
merged 6 commits into from
Apr 19, 2017
Merged

T/148: getOptimalPosition utility should consider limiter ancestors with CSS overflow. Closes #148. #149

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77888f2
Implemented Rect#getVisible method. Defined Rect#_obj property.
oleq Apr 18, 2017
ee4b394
Fix: The getOptimalPosition utility should consider limiter ancestors…
oleq Apr 18, 2017
c55ea7e
Tests: Refactoring, moved manual FocusTracker tests to a separate dir…
oleq Apr 18, 2017
027d510
Fixed: Range#getVisible throws when used for document#body.
oleq Apr 18, 2017
24e1d14
Performance: Speed up Rect#getVisible nearly twice by avoiding window…
oleq Apr 18, 2017
1123afd
Renamed Rect#_obj to Rect#_source. Code refactoring.
oleq Apr 19, 2017
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
2 changes: 1 addition & 1 deletion src/dom/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export function getOptimalPosition( { element, target, positions, limiter, fitIn
if ( !limiter && !fitInViewport ) {
[ name, bestPosition ] = getPosition( positions[ 0 ], targetRect, elementRect );
} else {
const limiterRect = limiter && new Rect( limiter );
const limiterRect = limiter && new Rect( limiter ).getVisible();
const viewportRect = fitInViewport && Rect.getViewportRect();

[ name, bestPosition ] =
Expand Down
54 changes: 54 additions & 0 deletions src/dom/rect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ export default class Rect {
* @param {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} obj A source object to create the rect.
*/
constructor( obj ) {
/**
* The object this rect is for.
*
* @protected
* @readonly
* @member {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} #_obj
*/
Object.defineProperty( this, '_obj', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why defineProperty ?
Maybe _context instead of _obj?

Copy link
Member Author

Choose a reason for hiding this comment

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

defineProperty to set enumerable false. Without it iterating over the Rect would also return _obj and make some pieces of code more complex.

As for _obj, maybe not _context because it makes people think about this and stuff but _from?

// obj._obj if already the Rect instance
value: obj._obj || obj,
writable: false,
enumerable: false
} );

if ( isElement( obj ) || isRange( obj ) ) {
obj = obj.getBoundingClientRect();
}
Expand Down Expand Up @@ -179,6 +193,46 @@ export default class Rect {
return this.width * this.height;
}

/**
* Returns a new rect, a part of the original rect, which is actually visible to the user,
* e.g. an original rect cropped by parent element rects which have `overflow` set in CSS
* other than `"visible"`.
*
* If there's no such visible rect, which is when the rect is limited by one or many of
* the ancestors, `null` is returned.
*
* @returns {module:utils/dom/rect~Rect|null} A visible rect instance or `null`, if there's none.
*/
getVisible() {
const obj = this._obj;
let visibleRect = this.clone();

// There's no ancestor to crop <body> with the overflow.
if ( obj != global.document.body ) {
let parent = obj.parentNode || obj.commonAncestorContainer;

// Check the ancestors all the way up to the <body>.
while ( parent && parent != global.document.body ) {
const parentRect = new Rect( parent );
const intersectionRect = visibleRect.getIntersection( parentRect );

if ( intersectionRect ) {
if ( intersectionRect.getArea() < visibleRect.getArea() ) {
// Reduce the visible rect to the intersection.
visibleRect = intersectionRect;
}
} else {
// There's no intersection, the rect is completely invisible.
return null;
}

parent = parent.parentNode;
}
}

return visibleRect;
}

/**
* Returns a rect of the web browser viewport.
*
Expand Down
25 changes: 25 additions & 0 deletions tests/dom/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,31 @@ describe( 'getOptimalPosition()', () => {
name: 'left'
} );
} );

// https://github.com/ckeditor/ckeditor5-utils/issues/148
it( 'should return coordinates (#3)', () => {
const overflowedAncestor = getElement( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Webstorm says Local variable 'overflowedAncestor' is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is odd. It's used.

Copy link
Contributor

Choose a reason for hiding this comment

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

This warning is about

const overflowedAncestor = getelement();

limiter.parentNode = overflowedAncestor;

vs

limiter.parentNode = getelement();

but it's a detail.

top: 100,
left: 0,
bottom: 110,
right: 10,
width: 10,
height: 10
}, {
overflow: 'scroll'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like overflow: scroll is no longer needed by this test but OTOH it better explains this test case.

} );

limiter.parentNode = overflowedAncestor;

assertPosition( {
element, target, limiter,
positions: [ attachRight, attachLeft ]
}, {
top: 100,
left: 10,
name: 'right'
} );
} );
} );

describe( 'with fitInViewport on', () => {
Expand Down
213 changes: 213 additions & 0 deletions tests/dom/rect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ describe( 'Rect', () => {
} );

describe( 'constructor()', () => {
it( 'should store passed object in #_obj property', () => {
const obj = {};
const rect = new Rect( obj );

expect( rect._obj ).to.equal( obj );
} );

it( 'should accept HTMLElement', () => {
const element = document.createElement( 'div' );

Expand Down Expand Up @@ -97,6 +104,14 @@ describe( 'Rect', () => {
expect( clone ).not.equal( rect );
assertRect( clone, rect );
} );

it( 'should preserve #_obj', () => {
const rect = new Rect( geometry );
const clone = rect.clone();

expect( clone._obj ).to.equal( rect._obj );
assertRect( clone, rect );
} );
} );

describe( 'moveTo()', () => {
Expand Down Expand Up @@ -320,6 +335,204 @@ describe( 'Rect', () => {
} );
} );

describe( 'getVisible()', () => {
let element, range, ancestorA, ancestorB;

beforeEach( () => {
element = document.createElement( 'div' );
range = document.createRange();
ancestorA = document.createElement( 'div' );
ancestorB = document.createElement( 'div' );

ancestorA.append( element );
document.body.appendChild( ancestorA );
} );

afterEach( () => {
ancestorA.remove();
ancestorB.remove();
} );

it( 'should return a new rect', () => {
const rect = new Rect( {} );
const visible = rect.getVisible();

expect( visible ).to.not.equal( rect );
} );

it( 'should not fail when the rect is for document#body', () => {
testUtils.sinon.stub( document.body, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

assertRect( new Rect( document.body ).getVisible(), {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );
} );

it( 'should return the visible rect (HTMLElement), partially cropped', () => {
testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
top: 50,
right: 150,
bottom: 150,
left: 50,
width: 100,
height: 100
} );

assertRect( new Rect( element ).getVisible(), {
top: 50,
right: 100,
bottom: 100,
left: 50,
width: 50,
height: 50
} );
} );

it( 'should return the visible rect (HTMLElement), fully visible', () => {
testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
top: 0,
right: 150,
bottom: 150,
left: 0,
width: 150,
height: 150
} );

assertRect( new Rect( element ).getVisible(), {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );
} );

it( 'should return the visible rect (HTMLElement), partially cropped, deep ancestor overflow', () => {
ancestorB.append( ancestorA );
document.body.appendChild( ancestorB );

testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
top: 50,
right: 100,
bottom: 100,
left: 0,
width: 50,
height: 50
} );

testUtils.sinon.stub( ancestorB, 'getBoundingClientRect' ).returns( {
top: 0,
right: 150,
bottom: 100,
left: 50,
width: 100,
height: 100
} );

assertRect( new Rect( element ).getVisible(), {
top: 50,
right: 100,
bottom: 100,
left: 50,
width: 50,
height: 50
} );
} );

it( 'should return the visible rect (Range), partially cropped', () => {
range.setStart( ancestorA, 0 );

testUtils.sinon.stub( range, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
top: 50,
right: 150,
bottom: 150,
left: 50,
width: 100,
height: 100
} );

assertRect( new Rect( range ).getVisible(), {
top: 50,
right: 100,
bottom: 100,
left: 50,
width: 50,
height: 50
} );
} );

it( 'should return null if there\'s no visible rect', () => {
testUtils.sinon.stub( element, 'getBoundingClientRect' ).returns( {
top: 0,
right: 100,
bottom: 100,
left: 0,
width: 100,
height: 100
} );

testUtils.sinon.stub( ancestorA, 'getBoundingClientRect' ).returns( {
top: 150,
right: 200,
bottom: 200,
left: 150,
width: 50,
height: 50
} );

expect( new Rect( element ).getVisible() ).to.equal( null );
} );
} );

describe( 'getViewportRect()', () => {
it( 'should reaturn a rect', () => {
expect( Rect.getViewportRect() ).to.be.instanceOf( Rect );
Expand Down
0 tests/manual/focustracker.html → tests/manual/focustracker/focustracker.html
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion tests/manual/focustracker.js → tests/manual/focustracker/focustracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

/* global document */

import FocusTracker from '../../src/focustracker';
import FocusTracker from '../../../src/focustracker';

const focusTracker = new FocusTracker();
const counters = document.querySelectorAll( '.status b' );
Expand Down
0 tests/manual/focustracker.md → tests/manual/focustracker/focustracker.md
View file
Open in desktop
File renamed without changes.
38 changes: 38 additions & 0 deletions tests/manual/tickets/148/1.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<div class="wrapper">
<div class="limiter">
<div class="target"></div>
</div>

<div class="source"></div>
</div>


<style>
.wrapper {
overflow: scroll;
outline: 1px solid #000;
height: 400px;
position: relative;
padding: 20px;
margin-top: 100px;
}

.limiter {
height: 500px;
overflow: hidden;
}

.target {
height: 150px;
background: #ccc;
width: 50px;
margin: 170px auto 0;
}

.source {
height: 50px;
width: 50px;
background: red;
position: absolute;
}
</style>
Loading