This project is archived and is in readonly mode.

#1048 ✓resolved
Arian

getPosition inconsistencies between 1.3 and 1.2

Reported by Arian | October 19th, 2010 @ 10:02 PM | in 1.4.0 (closed)

Element.getPosition is broken in 1.3 if you try to get the position of an element which is in another scrollable element.

Behavior in 1.2: http://www.jsfiddle.net/8EyNT/1/

Behavior in 1.3: http://www.jsfiddle.net/8EyNT/2/

I think this is because Element.getOffsets handles the scrolling already, but this is handled in getPosition again.
So this is a proposed fix: http://github.com/arian/mootools-core/commit/45366026a65eeef6b4c449...

Apparently there aren't any Specs for this, so after this change every Spec passes.

Inherent to this problem is MooTools More Ticket #416. It breaks Fx.Scroll:toElement.

Comments and changes to this ticket

  • Fábio M. Costa

    Fábio M. Costa October 20th, 2010 @ 03:04 AM

    Looks like 1.3 has the correct implementation, no?

  • JacobThornton

    JacobThornton October 20th, 2010 @ 06:40 AM

    fwiw i would expect the result of 1.2.

  • Arian

    Arian October 20th, 2010 @ 11:55 AM

    Well, there are two problems:

    • The 1.3 implementation is different than the 1.2 implementation which will break stuff, like Fx.Scroll
    • The 1.3 implementation behaves like 1.2 when the relative element is document.body: http://www.jsfiddle.net/8EyNT/3/

    So if I want to fix MooTools More ticket #416 I need to check if the element is document.body and if not, Element.getScroll should be added to Element.getPosition to get the right position.

  • Fábio M. Costa

    Fábio M. Costa October 20th, 2010 @ 05:01 PM

    Ok we should keep it working like 1.2 IMHO. Don't fix it on more please. Applying your fix fixes Fx.Scroll?

  • Arian

    Arian October 20th, 2010 @ 06:21 PM

    Yes it fixes Fx.Scroll, however I didn't test more than just Fx.Scroll, so it probably needs a second look and some more testing.

    For the More 1.3.0.1 release we have to fix this in more till this is fixed in Core.

  • JacobThornton

    JacobThornton October 21st, 2010 @ 01:41 AM

    Arian's change kinda seems kinda strange to me because it changes code that was present in 1.2 -- i suspect the breaking change happened in getOffsets by removing elemScroll

  • gonchuki

    gonchuki October 22nd, 2010 @ 09:30 AM

    getOffsets was broken, it's part of a series of fixes I did on the Dimensions module. Check the logs for tickets #952 and #637. Read carefully as I did a cross check against all other popular javascript libraries to verify that the fix was indeed doing what was expected. Basically the issue was that (in your exampple), if you scrolled the wrapper element, then a getPosition on said element would return an incorrect value (the physical position of the element on screen was not changing, but its coordinates changed)

    related commit: http://github.com/mootools/mootools-core/commit/6c9fda19070de685ebb...

    tl;dr - 1.2 was broken. code was fixed in 1.3 and anything that relied on the broken implementation needs an update.

    If the explanation above does not make sense I can try rewording it, I'm always bad at explaining stuff.

  • gonchuki

    gonchuki October 22nd, 2010 @ 09:34 AM

    also, maybe there's still some weird stuff when document.body is the parent, I didn't try inspecting the code but seems you might have uncovered an inconsistency in how the fixed 1.3 code works there.

  • Fábio M. Costa

    Fábio M. Costa October 23rd, 2010 @ 04:04 PM

    • Assigned user changed from “Fábio M. Costa” to “Christoph Pojer”

    @gonchuki i understand your work and like them. But we should revert them as this change will break lots of existing scripts done with moo 1.2. We can get them back on 2.0.
    So please, someone from the -core team, revert the changes.

  • gonchuki

    gonchuki October 23rd, 2010 @ 05:59 PM

    I don't get your suggestion for rolling back to a broken state. If we do that, then it's like we are "supporting" this wrong calculations and fostering the creation of even more plugins that rely on a broken implementation.
    As I said, there might be some extra work to be done to be consistent, but that extra work should not include breaking the coordinates system again.

  • mattcoz

    mattcoz October 24th, 2010 @ 08:18 PM

    Another issue is that it all breaks down when the HTML element has an overflow value other than visible. When that's the case, getPosition should not automatically return 0,0 for the body element. This does not apply to Opera though, as they don't follow the standard in this case.

  • mattcoz

    mattcoz October 24th, 2010 @ 11:21 PM

    Actually, I don't think it should ever automatically return 0,0 for the body element.

    http://jsfiddle.net/zTFtK/

    That just doesn't seem right to me.

    http://jsfiddle.net/PBZzg/

    Isn't that more accurate? Am I thinking about this wrong?

  • shanebo

    shanebo November 1st, 2010 @ 07:32 PM

    Any news on this? Fx.Scroll is such a commonly used class and it's all sorts of jacked in 1.3 more. See related: https://mootools.lighthouseapp.com/projects/24057/tickets/427-fxscr...

  • Christoph Pojer

    Christoph Pojer February 23rd, 2011 @ 12:52 PM

    • Milestone changed from 1.3.1 to 1.4.0
    • Milestone order changed from “5” to “0”

    Does anyone have time to look into this and provide a proper patch for 1.3 that doesn't have any side effects?

  • gonchuki

    gonchuki February 23rd, 2011 @ 02:06 PM

    I'll take a look.
    Tried testing this last week but didn't manage to get anything 'wrong' with the actual code. I will give it another shot and see if I can reproduce the issue with the body element.

  • Christoph Pojer
  • ibolmo

    ibolmo January 19th, 2012 @ 09:53 AM

    • State changed from “open” to “resolved”

    I'm uncertain if this is fixed with the latest. I'm setting it as resolved, but please open a new Github Issue: http://github.com/mootools/mootools-core/issues if you're still having trouble.

Create your profile

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

Shared Ticket Bins

Pages