Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tests no longer un in PhantomJS (after 1.0.0 upgrade) #91

Closed
aukevanleeuwen opened this issue Jun 9, 2015 · 2 comments
Closed

Tests no longer un in PhantomJS (after 1.0.0 upgrade) #91

aukevanleeuwen opened this issue Jun 9, 2015 · 2 comments

Comments

@aukevanleeuwen
Copy link

After upgrading from 0.7.1 to 1.0.0 my tests (that include a Drop instance) are failing. I investigated a little bit and it fails in the getComputedStyle method.

Current implementation (1.0.1):

function getScrollParent(el) {
  var _getComputedStyle = getComputedStyle(el);

  var position = _getComputedStyle.position;

  if (position === 'fixed') {
    return el;
  }

  var parent = el;
  while (parent = parent.parentNode) {
    var style = undefined;
    try {
      style = getComputedStyle(parent);
    } catch (err) {}

    if (typeof style === 'undefined') {
      return parent;
    }

    var overflow = style.overflow;
    var overflowX = style.overflowX;
    var overflowY = style.overflowY;

    if (/(auto|scroll)/.test(overflow + overflowY + overflowX)) {
      if (position !== 'absolute' || ['relative', 'absolute', 'fixed'].indexOf(style.position) >= 0) {
        return parent;
      }
    }
  }

  return document.body;
}

Previous implementation (0.7.1):

  getScrollParent = function(el) {
    var parent, position, scrollParent, style, _ref;
    position = getComputedStyle(el).position;
    if (position === 'fixed') {
      return el;
    }
    scrollParent = void 0;
    parent = el;
    while (parent = parent.parentNode) {
      try {
        style = getComputedStyle(parent);
      } catch (_error) {}
      if (style == null) {
        return parent;
      }
      if (/(auto|scroll)/.test(style['overflow'] + style['overflowY'] + style['overflowX'])) {
        if (position !== 'absolute' || ((_ref = style['position']) === 'relative' || _ref === 'absolute' || _ref === 'fixed')) {
          return parent;
        }
      }
    }
    return document.body;
  };

The 1.0.1 version fails on the line if (typeof style === 'undefined') {. It used to check for if (style == null) { which will both check for null and undefined. And apparently when it checks the body in PhantomJS it returns null for the body element.
@aukevanleeuwen
Copy link
Author

I investigated a bit more: that last line isn't entirely correct, it doesn't return null for the <body> element, but it doesn't find any overflow styles there either, thus it traverses up to (by heart) HtmlElement and even further to HtmlDocument or something similar. In the end it will return null nonetheless and therefore will eventually fail on var overflow = style.overflow;

I suggest making it a bit more bullet proof and checking for if (style == null) again.

@geekjuice
Copy link
Contributor

Thanks for catching that issue. I see no harm in checking for null. Just for reference though, do you have the stack trace of that error? I was under the impression that getComputedStyle returns undefined and not null when no styles are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@aukevanleeuwen @geekjuice