IE offsetLeft inaccuracy in viewportOffset

[jwestbrook]

previous lighthouse ticket #491 by bander

---

IE sometimes gives the wrong offsetLeft. It appears to be basing it on an element other than the offsetParent. I apologize that I don't understand what triggers the problem better, but I have prepared a test case:

http://devsandbox.nfshost.com/prototype/cloneposition.html

I was able to work around it by using getBoundingClientRect:

viewportOffset: function(forElement) {
    var valueT = 0, valueL = 0;

    if (forElement.getBoundingClientRect) {
      var box = forElement.getBoundingClientRect();
      return Element._returnOffset(box.left, box.top);
    }

    var element = forElement;
    do {
      valueT += element.offsetTop  || 0;
      valueL += element.offsetLeft || 0;

[jwestbrook]

bander December 16th, 2008 @ 12:49 AM

It looks like getBoundingClientRect() handles borders differently. I've made the following change to clonePosition to deal with it, which feels like a bad solution, but it handles borders of arbitrary widths in FF3, IE7, Opera 9.5, and Chrome 0.2. I'll add a border to my test case.

  clonePosition: function(element, source) {
    var options = Object.extend({
      setLeft:    true,
      setTop:     true,
      setWidth:   true,
      setHeight:  true,
      offsetTop:  0,
      offsetLeft: 0
    }, arguments[2] || { });

    // find page position of source
    source = $(source);
    var p = source.viewportOffset();

    // find coordinate system to use
    element = $(element);
    var delta = [0, 0];
    var parent = null;
    // delta [0,0] will do fine with position: fixed elements,
    // position:absolute needs offsetParent deltas
    if (Element.getStyle(element, 'position') == 'absolute') {
      parent = element.getOffsetParent();
      delta = parent.viewportOffset();
      delta[0] += parseInt('0'+parent.getStyle('borderLeftWidth'), 10);
      delta[1] += parseInt('0'+parent.getStyle('borderTopWidth'), 10);
    } 

[jwestbrook]

bander December 16th, 2008 @ 12:53 AM

Oh, cool: even with the old viewportOffset code, IE7 clonePosition was broken with a border. That makes me feel better about my solution.

[jwestbrook]

Ian Lotinsky December 20th, 2008 @ 01:22 AM

Good find. Just came across this issue today too. The first fix isn't working for me if I scroll down the page. Also, your second fix assumes that the element's parent is the one and only element with a border. I have a border on a root node and am trying to clone the position of a leaf node several levels deep. My brain is shot after looking at this for so long today, but will try to take a look at it Monday. (I'm guessing the fix is going to be in viewportOffset, not clonePosition.)

[jwestbrook]

bander December 20th, 2008 @ 02:17 AM

Yeah, positioning hurts my brain, too. I'm pretty sure my second solution works with multiple borders, though, actually. It's specifically correcting for the difference between the containing box's position and the position of an element positioned absolutely at top:0, left:0 within it. (It may be more appropriate for me to subtract margin from getBoundingClientRect and then adjust for margin and border on the parent.) You can see this by going to http://www.brunildo.org/test/ap_contbox1.html and, in firebug, kicking the border up to 10px on one of those div.wrapNs. If you change the margin or border of the containing box, the child adjusts, just not if you change the padding. Dang, I thought I'd checked scrolling. I've changed my example page to make it easier to test. I'll look into it soon.

[jwestbrook]

Ian Lotinsky December 24th, 2008 @ 07:55 PM

I fixed my nested element issue with an addition of border widths in viewportOffset:

 viewportOffset: function(forElement) {
    var valueT = 0, valueL = 0;

    var element = forElement;
    do {
      valueT += element.offsetTop  || 0;
      valueL += element.offsetLeft || 0;

      // Safari fix
      if (element.offsetParent == document.body &&
        Element.getStyle(element, 'position') == 'absolute') break;

    } while (element = element.offsetParent);

    element = forElement;
    do {
      if (!Prototype.Browser.Opera || element.tagName == 'BODY') {
        valueT -= element.scrollTop  || 0;
        valueL -= element.scrollLeft || 0;
      }

      if (Prototype.Browser.IE && element.style) {
        valueT += parseInt('0'+$(element).getStyle('borderTopWidth'), 10);
        valueL += parseInt('0'+$(element).getStyle('borderLeftWidth'), 10);
      }
    } while (element = element.parentNode);

    return Element._returnOffset(valueL, valueT);
  },

Since your solutions work for you but not me, can you check and see if mine solves yours? Thanks!

[jwestbrook]

bander December 24th, 2008 @ 08:46 PM

I don't think it will, since my original test page didn't have borders on any element. I'll give it a try, though, when I'm back at work next week. Glad you were able to get it working for you.

[jwestbrook]

bander December 24th, 2008 @ 08:46 PM

By the way, are you using IE6 or IE7?

[jwestbrook]

Ian Lotinsky December 24th, 2008 @ 09:22 PM

Hold on that. I just realized something stupid: I'm on a slightly old release. IE7.

[jwestbrook]

Ian Lotinsky December 24th, 2008 @ 09:41 PM

Okay, I upgraded to 1.6.0.3, applied the fix in #90 and successfully tested my fix in IE6 and IE7. Let me know what you find. Thanks!

[jwestbrook]

Ian Lotinsky December 24th, 2008 @ 09:59 PM

Can you clarify what you mean about borders because I see them in all of your examples (both pages) have DIVs with borders.

[jwestbrook]

an Lotinsky December 24th, 2008 @ 10:03 PM

And I'm not as think as you drunk I am. (I wish you could edit comments in Lighthouse. My last comment was grammatically confusing.)

[jwestbrook]

bander December 30th, 2008 @ 12:25 PM

When I first posted my first example, it didn't have the border. (Sorry, I guess it's rude to keep changing what I've posted like that.) Your fix makes sense conceptually, I think, but it doesn't fix my issue, unfortunately. (It seems to me like it would be more appropriate to add the border widths in the offsetParent traversal rather than the parentNode traversal, but hey, if it fixed it for you.) I'm hitting some additional headaches myself as I try to get drag and drop ghosting to append the drag copy to the body for document-wide dragging and then clone the position of the original. It's still broken, but the following change did seem to help my test page.

   if (Element.getStyle(element, 'position') == 'absolute') {
      parent = element.getOffsetParent();
      // http://prototype.lighthouseapp.com/projects/8886-prototype/tickets/491
      if (parent == document.documentElement || parent == document.body) {
        delta[0] = -document.documentElement.scrollLeft + document.documentElement.clientLeft;
        delta[1] = -document.documentElement.scrollTop + document.documentElement.clientTop;
      } else {
        delta = parent.viewportOffset();
        delta[0] += parseInt('0'+parent.getStyle('borderLeftWidth'), 10);
        delta[1] += parseInt('0'+parent.getStyle('borderTopWidth'), 10);
      }
    }

Anyway, congrats on finding an elegant solution that solved your problem. I'll keep tinkering with this.

[jwestbrook]

bander December 30th, 2008 @ 09:40 PM

I've modified my test page to dynamically switch Prototype versions and toggle the border on the containing parent. Hope this will make debugging a bit easier. (I'd be happy to point the "ian" option to your own hosted copy of your changes if you like.) It looks like there's some kind of position caching going on, so it does require a refresh after each prototype version change, unfortunately.

[jwestbrook]

bander January 2nd, 2009 @ 10:04 PM

My test page (the link in my OP) now forces a refresh when changing Prototype versions. On my IE7, at least, your version actually appears to me further afield than the original, with or without borders.

[jwestbrook]

bander January 2nd, 2009 @ 10:08 PM

Is it possible for you to provide a simplified example for me to test my getClientBoundingRect solution against? I'm curious whether my separate handling for document.documentElement fixed your problem.

[jwestbrook]

Ian Lotinsky January 21st, 2009 @ 08:48 PM

Per our IM conversations, all my testing has been using relative or statically positioned elements. My fix works for my valid markup but not you and vice versa. It's my opinion that clonePosition, viewportOffset, and getOffsetParent are simply broken. I don't have the time to fix it and will be taking a close look at alternative libraries. Thanks.

[jwestbrook]

Nick Stakenburg January 22nd, 2009 @ 12:40 AM

It's my opinion that clonePosition, viewportOffset, and getOffsetParent are simply broken.

You are probably right. I remember fixing a number of bugs with JDD in all of those functions. Our tickets are now closed as 'resolved' but Prototype was reverted back to some older 1.6.0.2 release so none of those fixes made it into the 1.6.0.3 release. We ended up fixing not just viewportOffset and patched away all over the place. Like you notice right now that started showing bugs in other parts of Prototype so the revert was a quick fix to stabilize things. Probably best to start from scratch on dimensions. A more complete set of dimension tests would help there. Maybe Element.Layout is doing that already, haven't looked into it yet.