T/148: getOptimalPosition utility should consider limiter ancestors with CSS overflow. Closes #148. #149
Conversation
… with CSS overflow. Closes #148.
src/dom/rect.js
Outdated
|
|
||
| // Check the ancestors all the way up to the <body>. | ||
| while ( parent && parent != global.document.body ) { | ||
| const parentOverflow = global.window.getComputedStyle( parent ).overflow; |
There was a problem hiding this comment.
I wonder if checking each parent as below (const intersectionRect = visibleRect.getIntersection( parentRect );) wouldn't be actually faster than using this window.getComputedStyle. Will benchmark.
There was a problem hiding this comment.
...and I was right. window.getComputedStyle is slow. Twice the performance after 24e1d14.
oleq
changed the title
…#getComputedStyles.
src/dom/rect.js
Outdated
| * @readonly | ||
| * @member {HTMLElement|Range|ClientRect|module:utils/dom/rect~Rect|Object} #_obj | ||
| */ | ||
| Object.defineProperty( this, '_obj', { |
There was a problem hiding this comment.
Why defineProperty ?
Maybe _context instead of _obj?
There was a problem hiding this comment.
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?
tests/dom/position.js
Outdated
|
|
||
| // https://github.com/ckeditor/ckeditor5-utils/issues/148 | ||
| it( 'should return coordinates (#3)', () => { | ||
| const overflowedAncestor = getElement( { |
There was a problem hiding this comment.
Webstorm says Local variable 'overflowedAncestor' is redundant.
There was a problem hiding this comment.
This warning is about
const overflowedAncestor = getelement();
limiter.parentNode = overflowedAncestor;
vs
limiter.parentNode = getelement();
but it's a detail.
tests/dom/position.js
Outdated
| width: 10, | ||
| height: 10 | ||
| }, { | ||
| overflow: 'scroll' |
There was a problem hiding this comment.
Looks like overflow: scroll is no longer needed by this test but OTOH it better explains this test case.
|
I left few comments but works 👌 |
Suggested merge commit message (convention)
Fix: The getOptimalPosition utility should consider limiter ancestors with CSS overflow. Closes ckeditor/ckeditor5#4948.