Skip to content

Commit

Permalink
Skip to offset parent in getContainer, #12
Browse files Browse the repository at this point in the history
  • Loading branch information
@ausi
ausi committed Jun 11, 2017
1 parent e15df77 commit 3a61134
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 2 deletions.
30 changes: 29 additions & 1 deletion cq-prolyfill.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,8 +1017,36 @@ function getContainer(element, prop) {
}

else {
var parentContainer = getContainer(element.parentNode, prop);

var parentNode = element.parentNode;

// Skip to next positioned ancestor for absolute positioned elements
if (getComputedStyle(element).position === 'absolute') {
while (
parentNode.parentNode
&& parentNode.parentNode.nodeType === 1
&& ['relative', 'absolute'].indexOf(getComputedStyle(parentNode).position) === -1
) {
parentNode = parentNode.parentNode;
}
}

// Skip to next ancestor with a transform applied for fixed positioned elements
if (getComputedStyle(element).position === 'fixed') {
while (
parentNode.parentNode
&& parentNode.parentNode.nodeType === 1
&& [undefined, 'none'].indexOf(
getComputedStyle(parentNode).transform
|| getComputedStyle(parentNode).MsTransform
|| getComputedStyle(parentNode).WebkitTransform
) !== -1
) {
parentNode = parentNode.parentNode;
}
}

var parentContainer = getContainer(parentNode, prop);
while (getComputedStyle(parentNode).display === 'inline') {
parentNode = parentNode.parentNode;
}
Expand Down
29 changes: 28 additions & 1 deletion tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,9 +588,15 @@ QUnit.test('evaluateQuery', function(assert) {

});

/*global getContainer, containerCache: true, createCacheMap*/
/*global getContainer, styleCache: true, containerCache: true, createCacheMap*/
QUnit.test('getContainer', function(assert) {

styleCache = {
width: {},
height: {},
};
containerCache = createCacheMap();

var element = document.createElement('div');
element.innerHTML = '<span><div style="float: left"><a>';
document.body.appendChild(element);
Expand All @@ -614,6 +620,27 @@ QUnit.test('getContainer', function(assert) {
assert.strictEqual(getContainer(link, 'height'), span, '<span> display block percentage height');
assert.ok(containerCache.has(link));

element.style.position = 'relative';
link.style.position = 'absolute';
containerCache = createCacheMap(); // Clear cache
assert.strictEqual(getContainer(link, 'width'), element, '<div> positioned ancestor');

span.style.position = 'absolute';
containerCache = createCacheMap(); // Clear cache
assert.strictEqual(getContainer(link, 'width'), element, '<div> positioned ancestor with non-intrinsic size');

span.style.width = '100px';
containerCache = createCacheMap(); // Clear cache
assert.strictEqual(getContainer(link, 'width'), span, '<span> positioned ancestor with fixed size');

link.style.position = 'fixed';
containerCache = createCacheMap(); // Clear cache
assert.strictEqual(getContainer(link, 'width'), document.documentElement, '<html> fixed ancestor');

element.style.cssText = '-webkit-transform: translateX(0); -ms-transform: translateX(0); transform: translateX(0);';
containerCache = createCacheMap(); // Clear cache
assert.strictEqual(getContainer(link, 'width'), element, '<div> ancestor with transform applied');

document.body.removeChild(element);

});
Expand Down

4 comments on commit 3a61134

@rjgotten
Copy link

@rjgotten rjgotten commented on 3a61134 Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

FYI: calling window.getComputedStyle is expensive and the properties on the CSSStyleDeclaration instance returned from it are live, which means each time you pull a property's value from it, you pay an additional performance cost by having to query the DOM's layout and potentially needing a reflow.

For each element on which you access the computed style, you should pull those properties you expect to actually use into local variables (or a new object literal saved as a local variable) and then not touch the object again. That way you can prevent expensive layout reflows from having to happen inbetween reads of the style object's properties.

A helper function that encapsulates this behavior can assist. E.g.

function getComputedStyle( element, props ) {
  var
    n     = props instanceof Array ? props.length : 0,
    style = n !== 0 ? window.getComputedStyle( element, null ) : null,
    obj   = {},
    prop;

  while ( n !== 0 ) {
    prop = props[ --n ];
    obj[ prop ] = style[ prop ];
  }
  
  return obj;
}

@ausi
Copy link
Owner Author

@ausi ausi commented on 3a61134 Jun 12, 2017

Choose a reason for hiding this comment

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

Thanks for checking. But getComputedStyle is only expensive if there are pending changes to the DOM. getContainer() only reads from the DOM’s layout state, it never writes. All references to CSSStyleDeclarations are gone once the script starts setting classes.

There is actually a test in tests-functional.js:500 that ensures that not a single reflow/layout happens while the container queries get evaluated.

But I think you are still right in that calling getComputedStyle multiple times for the same element in the same method doesn’t make much sense.

The helper function is not necessary I think.

@ausi
Copy link
Owner Author

@ausi ausi commented on 3a61134 Jun 12, 2017

Choose a reason for hiding this comment

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

I opened issue #47 for this topic, so that I don’t forget to fix it :)

@rjgotten
Copy link

Choose a reason for hiding this comment

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

There is actually a test in tests-functional.js:500 that ensures that not a single reflow/layout happens while the container queries get evaluated.

Ah. Cool. Missed that.
Sorry. :)

Please sign in to comment.