Permalink
Browse files

Fix issue where Element#clonePosition could give wrong results when t…

…he page is scrolled. [close #305]
  • Loading branch information...
1 parent 3caa54b commit 560bb59414fc9343ce85429b91b1e1b82fdc6812 @savetheclocktower savetheclocktower committed Jan 2, 2016
Showing with 84 additions and 20 deletions.
  1. +34 −19 src/prototype/dom/layout.js
  2. +37 −0 test/unit/tests/layout.test.js
  3. +13 −1 test/unit/views/tests/layout.erb
View
53 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);
View
37 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');
View
14 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 {

0 comments on commit 560bb59

Please sign in to comment.