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

Already on GitHub? Sign in to your account

use getBoundingClientRect in _getDOMObjectPosition #114

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

nwah commented Mar 8, 2013

Updates _getDOMObjectPosition to use getBoundingClientRect() to compute an element's position. This is the technique used in jQuery, Zepto, etc. The main benefit is that the location will be computed correctly for elements that are position:fixed, or inside a position:fixed element. It should also be a lot faster.

It has good browser support (http://www.quirksmode.org/dom/w3c_cssom.html), but isn't currently supported in jsdom (I checked with the latest version, 0.5.1). This caused some of the tests to break, so I the old implementation in as a fallback.

Should fix issue #79 and a couple others.

Owner

JamesMGreene commented Mar 8, 2013

Ah, now we're talking! :)

@JamesMGreene JamesMGreene commented on an outdated diff Mar 9, 2013

src/javascript/ZeroClipboard/utils.js
@@ -164,8 +164,20 @@ var _getDOMObjectPosition = function (obj) {
info.zIndex = parseInt(zi, 10);
}
- while (obj) {
+ // Use getBoundingClientRect where available (almost everywhere).
+ // See: http://www.quirksmode.org/dom/w3c_cssom.html
+ if (typeof obj.getBoundingClientRect !== 'undefined') {
+ // compute left / top offset (works for position:fixed too)
+ var rect = obj.getBoundingClientRect();
+ // clientLeft/clientTop are to fix IE's 2px offset in standards mode
+ info.left = rect.left + window.pageXOffset - document.documentElement.clientLeft;
@JamesMGreene

JamesMGreene Mar 9, 2013

Owner

This line will result in NaN in IE7-8, which do support obj.getBoundingClientRect() but not window.pageXOffset (undefined), ergo, e.g. 4 + undefined - 3NaN).

I believe the corrected version would be as follows:

var pageXOffset = window.pageXOffset || document.documentElement.scrollLeft || 0;
var leftBorderWidth = document.documentElement.clientLeft || 0;
info.left = rect.left + pageXOffset - leftBorderWidth;
@JamesMGreene

JamesMGreene Mar 9, 2013

Owner

@jonrohan Just for future awareness, too, there are a few other things to watch out for here:

  1. This won't work for IE7-8 if the document has no doctype (i.e. it is in quirks mode). jQuery does not support quirks mode either, e.g. with their $.offset method.
  2. IE7 gives incorrect scrollLeft values if the page is zoomed (e.g.)
  3. While this will work for positioning over position:fixed elements, we are still open to problems. For example:
    1. User hovers over position:fixed element that is glued. ZeroClipboard places a client Flash object above it.
    2. User scrolls mousewheel a few times: pageYOffset and the position of the Flash object [relative to the viewport] both change but the user's mouse is still hovering over the position:fixed element. Therefore, a few deltas on the mousewheel will end up with no ZC object over the position:fixed element. Perhaps need to consider repositioning on scroll/mousewheel events if the user was hovering over a glued element...?

@JamesMGreene JamesMGreene commented on an outdated diff Mar 9, 2013

src/javascript/ZeroClipboard/utils.js
@@ -164,8 +164,20 @@ var _getDOMObjectPosition = function (obj) {
info.zIndex = parseInt(zi, 10);
}
- while (obj) {
+ // Use getBoundingClientRect where available (almost everywhere).
+ // See: http://www.quirksmode.org/dom/w3c_cssom.html
+ if (typeof obj.getBoundingClientRect !== 'undefined') {
+ // compute left / top offset (works for position:fixed too)
+ var rect = obj.getBoundingClientRect();
+ // clientLeft/clientTop are to fix IE's 2px offset in standards mode
+ info.left = rect.left + window.pageXOffset - document.documentElement.clientLeft;
+ info.top = rect.top + window.pageYOffset - document.documentElement.clientTop;
@JamesMGreene

JamesMGreene Mar 9, 2013

Owner

This line will result in NaN in IE7-8, which do support obj.getBoundingClientRect() but not window.pageYOffset (undefined), ergo, e.g. 4 + undefined - 3NaN).

I believe the corrected version would be as follows:

var pageYOffset = window.pageYOffset || document.documentElement.scrollTop || 0;
var topBorderWidth = document.documentElement.clientTop || 0;
info.top = rect.top + pageYOffset - topBorderWidth;
@JamesMGreene

JamesMGreene Mar 9, 2013

Owner

@jonrohan Just for future awareness, too, there are a few other things to watch out for here:

  1. This won't work for IE7-8 if the document has no doctype (i.e. it is in quirks mode). jQuery does not support quirks mode either, e.g. with their $.offset method.
  2. IE7 gives incorrect scrollTop values if the page is zoomed (e.g.)
  3. While this will work for positioning over position:fixed elements, we are still open to problems. For example:
    1. User hovers over position:fixed element that is glued. ZeroClipboard places a client Flash object above it.
    2. User scrolls mousewheel a few times: pageYOffset and the position of the Flash object [relative to the viewport] both change but the user's mouse is still hovering over the position:fixed element. Therefore, a few deltas on the mousewheel will end up with no ZC object over the position:fixed element. Perhaps need to consider repositioning on scroll/mousewheel events if the user was hovering over a glued element...?
Contributor

nwah commented Mar 9, 2013

@JamesMGreene good catch. Just pushed those changes you suggested.

Owner

JamesMGreene commented Mar 10, 2013

@jonrohan Since our browser support offer is only IE8+ (and IE7 with Sizzle), what are our thoughts on pushing the lingering "fallback" method here into a getBoundingClientRect shim function for jsdom? That way, we can clean up the source a bit and this fallback code will only ever need to exist for the unit tests... and hopefully not even that once we switch over to using PhantomJS instead. 😉

Owner

jonrohan commented Mar 10, 2013

Honestly the jsdom position tests are pretty jank. I'd be willing to just kill the position tests, until we get a better testing system in. I don't want to sacrifice better code because of the testing framework.

@JamesMGreene JamesMGreene was assigned May 7, 2013

Owner

JamesMGreene commented May 10, 2013

Per Issue #149, this code will likely need to be updated to also account for the height and width, e.g.

  info.width = rect.width;
  info.height = rect.height;

myitcv commented May 14, 2013

As an update. Per #149 there is a bug in webkit that means getBoundingClientRect does not return the correct value when css zoom is used in webkit browsers.

The following hack fixes that:

var _getZoom = function(obj) {
  var zoom = 1;
  if(RegExp(' AppleWebKit/').test(navigator.userAgent)){
  while(obj)
  {
      zoom = zoom * _getStyle(obj,'zoom');
      obj = obj.offsetParent;
  }
  }
  return zoom;
};

which can then be used in a slightly updated _getDOMObjectPosition:

if (typeof obj.getBoundingClientRect !== "undefined") {
  var rect = obj.getBoundingClientRect();
  var pageXOffset = window.pageXOffset || document.documentElement.scrollLeft || 0;
  var pageYOffset = window.pageYOffset || document.documentElement.scrollTop || 0;
  var leftBorderWidth = document.documentElement.clientLeft || 0;
  var topBorderWidth = document.documentElement.clientTop || 0;
  var zoom = _getZoom(obj);
  info.width = rect.width * zoom;
  info.height = rect.height * zoom;
  info.left = (rect.left + pageXOffset - leftBorderWidth) * zoom;
  info.top = (rect.top + pageYOffset - topBorderWidth) * zoom;
  return info;
}

@JamesMGreene JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request Jul 6, 2013

@JamesMGreene Noah Burney + JamesMGreene Use 'getBoundingClientRect' to get element position and dimensions.
Closes #114.
Fixes #79.
Fixes #119.
Fixes #166.
Fixes #167.
Fixes #149.
b1be91a

@JamesMGreene JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request Jul 6, 2013

@JamesMGreene Noah Burney + JamesMGreene Use 'getBoundingClientRect' to get element position and dimensions.
Closes #114.
Fixes #79.
Fixes #119.
Fixes #166.
Fixes #167.
Fixes #149.
30f3a5d
Owner

JamesMGreene commented Jul 6, 2013

Usurped by PR #187.

@JamesMGreene JamesMGreene was assigned Jul 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment