This project is archived and is in readonly mode.

#427 ✓resolved
shanebo

Fx.Scroll positioning wrong when using offset option

Reported by shanebo | October 29th, 2010 @ 06:39 PM | in 1.3.1.1

Nothing seems to work in Fx.Scroll in 1.3. I'm seeing it's likely due to new getPosition code? In this example I'm using offset and toElement and neither are working as 1.2.x did.

http://jsfiddle.net/b6hkm/10/

Comments and changes to this ticket

  • shanebo

    shanebo November 1st, 2010 @ 08:25 PM

    Thanks to FunFactor, who I might add is loaded with fun and surprises just judo'd this bad boy.

    See fix:
    http://jsfiddle.net/b6hkm/11/

    Reverts this commit:
    http://github.com/mootools/mootools-more/commit/b491c3fa

  • shanebo

    shanebo November 1st, 2010 @ 08:27 PM

    • Assigned user changed from “Arian” to “Tim Wienk”

    Here's a cleaner fiddle with only what changed via FunFactor:

    http://jsfiddle.net/timwienk/b6hkm/13/

  • Tim Wienk

    Tim Wienk November 1st, 2010 @ 10:20 PM

    • State changed from “new” to “open”
    • Milestone set to 1.3.1.1
    • Title changed from “Fx.Scroll offset/toElement positioning is very buggy” to “Fx.Scroll positioning wrong when using offset option”
    • Tag changed from fx.scroll, more to fx.scroll, offset, position
    • Milestone order changed from “197538” to “0”

    Simplified: the start method calls the set method periodically, the start method itself already handles the offset option, doing that in the set method again adds the offset twice, which results in the broken behaviour.

    We'll have to discuss what the appropriate fix is. I see two options:

    • Revert b491c3fa and document that the offset option is not meant for the set method.
    • Remove adding the offset to the end position in the start method (since it's done in set), but subtract the offset from the starting position in the start method, because otherwise the first call to set sets the position to currentPosition + offset, which makes the element jump. Patch:
    diff --git a/Source/Fx/Fx.Scroll.js b/Source/Fx/Fx.Scroll.js
    index 82141aa..11e99dd 100644
    --- a/Source/Fx/Fx.Scroll.js
    +++ b/Source/Fx/Fx.Scroll.js
    @@ -70,12 +70,17 @@ Fx.Scroll = new Class({
                            scrollSize = element.getScrollSize(),
                            scroll = element.getScroll(),
                            size = element.getSize();
    +                       offset = this.options.offset,
                            values = {x: x, y: y};
     
    +               // Since `set` is setting +offset, but we do have to start at the currect position, we
    +               // need to subtract from the starting position what will be added by the `set` method.
    +               scroll.x -= offset.x;
    +               scroll.y -= offset.y;
    +
                    for (var z in values){
                            if (!values[z] && values[z] !== 0) values[z] = scroll[z];
                            if (typeOf(values[z]) != 'number') values[z] = scrollSize[z] - size[z];
    -                       values[z] += this.options.offset[z];
                    }
     
                    return this.parent([scroll.x, scroll.y], [values.x, values.y]);
    

    The first option means set will ignore the offset option, the second option feels quite hackish to me.

    I think at this point, I'd vote to revert, since the offset option only makes sense to me when you're calling it with the to* methods, which all use the start method.

    Discuss!

  • Arian

    Arian November 1st, 2010 @ 10:51 PM

    To revert that commit will make the start and set methods inconsistent and would cause unexpected behaviors. Probably not very often, but it's not cool.

    So I'd vote for a proper start method, and apply the offset in the set method. There must be a prettier way then your patch. There are some other (minor) problems with the start method, for example if you try to scroll to far to the right border, and scroll down as well, the Fx to the right will stop earlier than the Fx to the bottom. So someone should write some good tests and so a proper start method.

  • Aaron Newton

    Aaron Newton November 1st, 2010 @ 11:39 PM

    ++ to writing more tests.

    I guess I'm with Arian on this.

  • Tim Wienk

    Tim Wienk November 2nd, 2010 @ 12:33 AM

    Arian, the problem you describe sounds like a problem with the compute method to me, not the start method.

    If the only issue you're having is inconsistency between set and start another option is to move the offset-logic out of the set and start methods completely:

    • The offset option says: "An object with x and y properties of the distance to scroll to within the Element." Which implies it is only relevant for the to* methods, not for set and start which work with coordinates.
    • According to the documentation set and start scroll to the coordinates provided, which may be interpreted as those exact coordinates, so no offsets changing things.

    The resulting patch would then be something like:

    diff --git a/Source/Fx/Fx.Scroll.js b/Source/Fx/Fx.Scroll.js
    index f30a0af..123799e 100644
    --- a/Source/Fx/Fx.Scroll.js
    +++ b/Source/Fx/Fx.Scroll.js
    @@ -55,7 +55,7 @@ Fx.Scroll = new Class({
            set: function(){
                    var now = Array.flatten(arguments);
                    if (Browser.firefox) now = [Math.round(now[0]), Math.round(now[1])]; // not needed anymore in newer firefox versions
    -               this.element.scrollTo(now[0] + this.options.offset.x, now[1] + this.options.offset.y);
    +               this.element.scrollTo(now[0], now[1]);
            },
     
            compute: function(from, to, delta){
    @@ -66,41 +66,46 @@ Fx.Scroll = new Class({
     
            start: function(x, y){
                    if (!this.check(x, y)) return this;
    +               return this.parent(this.element.getScroll(), [x, y]);
    +       },
    +
    +       calculateScroll: function(x, y){
                    var element = this.element,
                            scrollSize = element.getScrollSize(),
                            scroll = element.getScroll(),
                            size = element.getSize(),
    +                       offset = this.options.offset,
                            values = {x: x, y: y};
     
                    for (var z in values){
                            if (!values[z] && values[z] !== 0) values[z] = scroll[z];
                            if (typeOf(values[z]) != 'number') values[z] = scrollSize[z] - size[z];
    -                       values[z] += this.options.offset[z];
    +                       values[z] += offset[z];
                    }
     
    -               return this.parent([scroll.x, scroll.y], [values.x, values.y]);
    +               return [values.x, values.y];
            },
     
            toTop: function(){
    -               return this.start(false, 0);
    +               return this.start.apply(this, this.calculateScroll(false, 0));
            },
     
            toLeft: function(){
    -               return this.start(0, false);
    +               return this.start.apply(this, this.calculateScroll(0, false));
            },
     
            toRight: function(){
    -               return this.start('right', false);
    +               return this.start.apply(this, this.calculateScroll('right', false));
            },
     
            toBottom: function(){
    -               return this.start(false, 'bottom');
    +               return this.start.apply(this, this.calculateScroll(false, 'bottom'));
            },
     
            toElement: function(el){
                    var position = document.id(el).getPosition(this.element),
                            scroll = isBody(this.element) ? {x: 0, y: 0} : this.element.getScroll();
    -               return this.start(position.x + scroll.x, position.y + scroll.y);
    +               return this.start.apply(this, this.calculateScroll(position.x + scroll.x, position.y + scroll.y));
            },
     
            scrollIntoView: function(el, axes, offset){
    

    If you guys have any other options or ideas, please share. Since the set method is called every step again, I think we either have to make up for the offsets before the loop, or not handle the offsets in the set method at all.

    (Yes, that's a long diff, I have the changes stashed, so I can commit if we agree.)

  • Arian

    Arian November 4th, 2010 @ 01:40 PM

    I approve with your patch. If you want to use the set method directly you could use something like scroll.set.apply(scroll, scroll.calculateScroll(49, 36));.

  • Tim Wienk

    Tim Wienk November 4th, 2010 @ 01:59 PM

    Indeed.

    Commit: https://github.com/timwienk/mootools-more/commit/2f5f6b86

    I will push once I (or someone else) have made specs/tests for Fx.Scroll with offset.

    It may be worth considering to make calculateScroll a public method, for the thing you describe. But until we decided that, it'll be undocumented (and therefore private).

  • Tim Wienk

Create your profile

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

The MooTools Extensions

Pages