This project is archived and is in readonly mode.

#461 ✓resolved
alcohol

getOffsets()

Reported by alcohol | November 6th, 2008 @ 10:34 AM | in 2.0 (closed)

http://github.com/mootools/mooto...

Commited changes break certain functionality.

http://www.robbast.nl/testcase -- excuse the poor layout. Above testcase works fine in FF, but is very dodgy in IE.

Reverting the changes made in previously linked commit @ github solves the IE bug.

Comments and changes to this ticket

  • Jimi

    Jimi November 12th, 2008 @ 11:44 AM

    Hello,

    I think that I have the same problem (mootools core : 1.2.1)

    Here's another test case as attached file : - Click Two Times on "Scroll Down" => it should scroll two times

    FF 3/Safari 3/Opera 9 works fine IE 6/7 doesn't

    And as Alcohol said, the pb is the fix in getOffsets():

    http://github.com/mootools/mooto...

    When I remove the Trident fix, it works fine for all.

    The original way (not the Trident way) compute the offset of the contained item without taking into account the item has move. This is the absolute/original offset. And in getPosition, the scroll offset is applied to it, so it result in the correct value for getPosition even the contained item has move.

    With the Trident way the scroll offset is already apply, so in getPosition the scroll offset is applied a second time.

    But I don't what is the true goal of getOffsets so I' can't decide if it's the Trident way the pb or the original way. And which one is best for compatibility.

    For the moment I will comment the Trident way.

    hope it will be helpfull.

    (sorry for my poor English)

  • Valerio

    Valerio February 18th, 2009 @ 03:56 AM

    • State changed from “new” to “open”
  • David Allsopp

    David Allsopp February 27th, 2009 @ 07:46 PM

    I've similarly found a problem with this function - which affected Aaron's documentation browser when rendered in IE7! The Trident patch assumes that the only thing which can scroll is the document so the function is totally broken if you try to query the position of an element within a scrolled

    . The following patch fixed it for me, but I'm not confident that it's either a totally correct patch or that it's any better than just removing the Trident specific code completely.

    --- Element.Dimensions.js 2009-02-27 14:03:37.807000000 +0000 +++ Element.Dimensions.js.new 2009-02-27 14:00:18.373000000 +0000 @@ -61,10 +61,17 @@

    getOffsets: function(){
        if (Browser.Engine.trident){
    
    
    •     var bound = this.getBoundingClientRect(), html = this.getDocument().documentElement;
      
      
    •     var bound = this.getBoundingClientRect(), html = this.offsetParent;
      
      
    •     var x = bound.left, y = bound.top;
      
      
    •     while (html && !isBody(html))
      
      
    •     {
      
      
    •         x += html.scrollLeft - html.clientLeft;
      
      
    •         y += html.scrollTop - html.clientTop;
      
      
    •         html = html.offsetParent;
      
      
    •     }
          return {
      
      
    •         x: bound.left + html.scrollLeft - html.clientLeft,
      
      
    •         y: bound.top + html.scrollTop - html.clientTop
      
      
    •         x: x,
      
      
    •         y: y
          };
      }
      
      
  • David Allsopp

    David Allsopp February 27th, 2009 @ 07:54 PM

    Sorry - didn't realise this wasn't a plain text entry field. Here's the same comment hopefully annotated correctly...

    I've similarly found a problem with this function - which affected Aaron's documentation browser when rendered in IE7! The Trident patch assumes that the only thing which can scroll is the document so the function is totally broken if you try to query the position of an element within a scrolled <div>. The following patch fixed it for me, but I'm not confident that it's either a totally correct patch or that it's any better than just removing the Trident specific code completely.

    
    --- Element.Dimensions.js	2009-02-27 14:03:37.807000000 +0000
    +++ Element.Dimensions.js.new	2009-02-27 14:00:18.373000000 +0000
    @@ -61,10 +61,17 @@
     
     	getOffsets: function(){
     		if (Browser.Engine.trident){
    -			var bound = this.getBoundingClientRect(), html = this.getDocument().documentElement;
    +			var bound = this.getBoundingClientRect(), html = this.offsetParent;
    +			var x = bound.left, y = bound.top;
    +			while (html && !isBody(html))
    +			{
    +				x += html.scrollLeft - html.clientLeft;
    +				y += html.scrollTop - html.clientTop;
    +				html = html.offsetParent;
    +			}
     			return {
    -				x: bound.left + html.scrollLeft - html.clientLeft,
    -				y: bound.top + html.scrollTop - html.clientTop
    +				x: x,
    +				y: y
     			};
     		}
    
  • rick

    rick March 31st, 2009 @ 08:38 PM

    I used a slightly modified version of David's fix and it worked for me. Thanks David.

            if (Browser.Engine.trident){
                var elem = this, bound = elem.getBoundingClientRect();
                
                while (elem = elem.offsetParent) {
                    bound.top += elem.scrollTop - elem.clientLeft;
                    bound.left += elem.scrollLeft - elem.clientTop;
                }
            
                return {
                    x: bound.left,
                    y: bound.top
                };
            }
    
  • rick

    rick April 30th, 2009 @ 11:26 PM

    i found an issue w/ my implementation where it didn't work for the case where the scrolling occurs at the window...here's the updated one that fixes this problem.

            if (Browser.Engine.trident){
                var elem = this, bound = elem.getBoundingClientRect(), html = this.getDocument().documentElement;
                
                while (elem = elem.offsetParent) {
                    bound.top += elem.scrollTop - elem.clientTop;
                    bound.left += elem.scrollLeft - elem.clientLeft;
                }
            
                return {
                    x: bound.left + html.scrollLeft - html.clientLeft,
                    y: bound.top + html.scrollTop - html.clientTop
                };
            }
    
  • joerg

    joerg June 13th, 2009 @ 08:52 PM

    I can confirm this issue in FireFox 3 too.
    I've tried it by changing if(...trident) into if(this.getBoundingClientRect).

  • mltsy

    mltsy June 22nd, 2009 @ 08:25 PM

    I agree that this should be changed.

    As of the Mootools 1.2.2 Core release, getOffsets still returns different values for IE 7 & 8 compared to every other browser when getting offsets for an element within a scrolled container.

    I don't know what this change was intended to fix, but it should not apply to IE 7 and 8, since it causes output to be inconsistent across those browsers. Therefore I think the current condition (Browser.Engine.trident) is not sufficient to accurately identify the conditions for which this block should be executed. What I know is that by removing this conditional block entirely, the getOffsets function has consistent output for elements within scrolled containers across Firefox 3, Safari 4 for Windows, Chrome 2.0, IE 7 and IE 8.

  • fearlex

    fearlex June 30th, 2009 @ 01:00 AM

    • Tag set to internet explorer, 1.2.2, 1.2.3

    This issue is no longer an IE only bug, as of Mootools version 1.2.3, now all engines, behave exactly the same as IE did.

  • fearlex

    fearlex June 30th, 2009 @ 01:17 AM

    Ok, i would like to add that rick's latest fix worked for me, it fixed this issue in all browsers including IE (all versions).

    By replacing this code:

    if (this.getBoundingClientRect){
        var bound = this.getBoundingClientRect(),
        html = document.id(this.getDocument().documentElement),
        scroll = html.getScroll(),
        isFixed = (styleString(this, 'position') == 'fixed');
        return {
            x: parseInt(bound.left, 10) + ((isFixed) ? 0 : scroll.x) - html.clientLeft,
            y: parseInt(bound.top, 10) +  ((isFixed) ? 0 : scroll.y) - html.clientTop
        };
    

    With this:

    if (Browser.Engine.trident){
          var elem = this, bound = elem.getBoundingClientRect(), html = this.getDocument().documentElement;
                
                while (elem = elem.offsetParent) {
                    bound.top += elem.scrollTop - elem.clientTop;
                    bound.left += elem.scrollLeft - elem.clientLeft;
                }
            
                return {
                    x: bound.left + html.scrollLeft - html.clientLeft,
                    y: bound.top + html.scrollTop - html.clientTop
          };
     }
    
  • Kenton

    Kenton August 17th, 2009 @ 11:18 PM

    • Tag changed from internet explorer, 1.2.2, 1.2.3 to 1.2.2, 1.2.3, offsets

    Yes, this has definitely gotten worse in 1.2.3

    I don't understand why Rick's fix isn't implemented yet, anyone care to respond?

  • Kenton

    Kenton August 17th, 2009 @ 11:19 PM

    • Tag changed from 1.2.2, 1.2.3, offsets to internet explorer, 1.2.2, 1.2.3, firefox, offsets, webkit
  • Kenton

    Kenton August 17th, 2009 @ 11:19 PM

    • Tag changed from internet explorer, 1.2.2, 1.2.3, firefox, offsets, webkit to internet explorer, 1.2.2, 1.2.3, firefox, offsets, regression, webkit
  • Michael Lucas-Smith

    Michael Lucas-Smith August 26th, 2009 @ 12:24 AM

    This may be fixed in the commit - but in 1.2.3:

    getPosition subtracts getOffsets from getScrolls. getOffsets already adjusts for scrolls, so getPosition ends up subtracting scrolls twice, making it totally wrong.

  • yougene

    yougene August 28th, 2009 @ 11:58 AM

    just fyi:

    both rick's and fearlex fixes kind of fix the problem with the overflow-offset for me (core-1.2.3 & more-1.2.3.1).

    but both also breaks the default behaviour of OverText for example.

  • rick

    rick August 31st, 2009 @ 04:37 PM

    can you elaborate on the problem you're finding w/ the Overtext yougene?

  • yougene

    yougene August 31st, 2009 @ 05:02 PM

    hey rick!

    And another thing i noticed just today, this only happens in combination with the CNET Mootools plugins.
    I use Aarons MooScroller in this project too, and i had to include the OverText-Compatibility extention to make it work in the fisrt place. Maybe this is more of a CNET bug than a Mootools Core/More one.

    The fix seemed to reset the "OverText" Element. The actual OverText was placed under the OverText Element.

    For exapmple, i got a "var overPlaylist = new OverText('#playlist');" While #playlist was 30px high, the OverText was placed 30px under the #playlist div.

    I kind-of fixed this for now, by setting a negative margin on "overTxtLabel".

  • Christoph Pojer

    Christoph Pojer September 3rd, 2010 @ 11:38 AM

    • State changed from “open” to “resolved”
    • Assigned user cleared.
    • Milestone order changed from “0” to “0”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Attachments

Referenced by

Pages