Fix issue where Element#clonePosition could give wrong results when the page is scrolled. [close prototypejs#305]

Author: savetheclocktower

src/prototype/dom/layout.js

@@ -1357,6 +1357,9 @@
    *  not it is part of the same [CSS containing
    *  block](http://www.w3.org/TR/CSS21/visudet.html#containing-block-details).
    *
+   *  Also note that `element` must already be `position: absolute` or
+   *  `position: fixed`. This method will not apply a `position` style.
+   *
    *  ##### Options
    *
    *  <table class='options'>
@@ -1416,14 +1419,23 @@
     element = $(element);
     var p, delta, layout, styles = {};
 
+    var isAbsolute = Element.getStyle(element, 'position') === 'absolute';
+    var parent = Element.getOffsetParent(element);
+
     if (options.setLeft || options.setTop) {
+      // We start by measuring the source's viewport offset.
       p = Element.viewportOffset(source);
+
+      // If the element we're altering is `position: fixed`, that's all the
+      // information we need: later we'll apply that offset to the `top` and
+      // `left` properties directly.
       delta = [0, 0];
-      // A delta of 0/0 will work for `positioned: fixed` elements, but
-      // for `position: absolute` we need to get the parent's offset.
-      if (Element.getStyle(element, 'position') === 'absolute') {
-        var parent = Element.getOffsetParent(element);
-        if (parent !== document.body) delta = Element.viewportOffset(parent);
+
+      // But if it's `position: absolute`, we have to know where its offset
+      // parent is positioned and take those measurements into account as
+      // well.
+      if (isAbsolute && parent !== document.body) {
+        delta = Element.viewportOffset(parent);
       }
     }
 
@@ -1440,27 +1452,30 @@
       return { x: x, y: y };
     }
 
-    var pageXY = pageScrollXY();
-
-
-    if (options.setWidth || options.setHeight) {
-      layout = Element.getLayout(source);
-    }
+    // When the offset parent is the document body, we need to account for
+    // scroll offsets when we set `top` and `left`. (Unless the element is
+    // `position: fixed`; in that case we should always ignore scroll
+    // position.)
+    var pageXY = (isAbsolute && parent === document.body) ? pageScrollXY() : { x: 0, y: 0 };
 
     // Set position.
     if (options.setLeft)
       styles.left = (p[0] + pageXY.x - delta[0] + options.offsetLeft) + 'px';
     if (options.setTop)
       styles.top  = (p[1] + pageXY.y - delta[1] + options.offsetTop)  + 'px';
 
-    // Use content box when setting width/height. If padding/border are
-    // different between source and target, that's for the user to fix;
-    // there's no good option for us.
-    if (options.setWidth) {
-      styles.width = layout.get('width')  + 'px';
-    }
-    if (options.setHeight) {
-      styles.height = layout.get('height') + 'px';
+    if (options.setWidth || options.setHeight) {
+      layout = Element.getLayout(source);
+
+      // Use content box when setting width/height. If padding/border are
+      // different between source and target, that's for the user to fix;
+      // there's no good option for us.
+      if (options.setWidth) {
+        styles.width = layout.get('width')  + 'px';
+      }
+      if (options.setHeight) {
+        styles.height = layout.get('height') + 'px';
+      }
     }
 
     return Element.setStyle(element, styles);

test/unit/tests/layout.test.js

@@ -394,6 +394,43 @@ suite("Layout", function(){
       assert.equal(before, after);
     });
 
+    test('#clonePosition (when element is absolutely positioned and has a non-body offset parent)', function () {
+      var opts = { offsetTop: 20, offsetLeft: 0, setWidth: false, setHeight: false };
+
+      var subMenu = $('sub_menu_2');
+      var mainMenu = $('main_menu_2');
+
+      subMenu.clonePosition(mainMenu, opts);
+      var offset = subMenu.viewportOffset().top - mainMenu.viewportOffset().top;
+
+      assert.equal(offset, 20);
+
+      scrollTo(0, 300);
+
+      subMenu.clonePosition(mainMenu, opts);
+      offset = subMenu.viewportOffset().top - mainMenu.viewportOffset().top;
+      assert.equal(offset, 20);
+    });
+
+    test('#clonePosition (when element has fixed position)', function () {
+      var opts = { offsetTop: 20, offsetLeft: 0, setWidth: false, setHeight: false };
+
+      var subMenu = $('sub_menu_3');
+      var mainMenu = $('main_menu_3');
+
+      subMenu.clonePosition(mainMenu, opts);
+      var offset = subMenu.viewportOffset().top - mainMenu.viewportOffset().top;
+
+      assert.equal(offset, 20);
+
+      scrollTo(0, 300);
+
+      subMenu.clonePosition(mainMenu, opts);
+      offset = subMenu.viewportOffset().top - mainMenu.viewportOffset().top;
+      assert.equal(offset, 20);
+
+    });
+
     test('#clonePosition (when elements have the same size)', function() {
       var source = $('clone_position_source');
       var target = $('clone_position_target');

test/unit/views/tests/layout.erb

@@ -301,7 +301,7 @@
 </style>
 
 <style type="text/css" media="screen">
-  #sub_menu {
+  #sub_menu, #sub_menu_2, #sub_menu_3 {
     position: absolute;
     top: 250px;
     left: 250px;
@@ -314,6 +314,18 @@
 <div id="scroller" style="height: 2000px;">Force scroll</div>
 <div id="sub_menu">Sub Menu</div>
 
+<div id="menu_container" style="position: relative;">
+  <div id="main_menu_2" style="background-color: #0f0;">Main Menu</div>
+
+  <div id="sub_menu_2">Sub Menu</div>
+</div>
+
+<div id="menu_container_3" style="position: relative;">
+  <div id="main_menu_3" style="background-color: #0f0;">Main Menu</div>
+</div>
+
+<div id="sub_menu_3" style="position: fixed;">Sub Menu</div>
+
 
 <style type="text/css" media="screen">
   #clone_position_source {