prototypejs / prototype

Prototype JavaScript framework
http://prototypejs.org/
Other
3.54k stars 639 forks source link

Element.clonePosition on chrome/android #305

Closed december1981 closed 8 years ago

december1981 commented 8 years ago

Since upgrading from 1.7.0 to 1.7.3, I notice that Element.clonePosition when used to position absolute elements, does not give expected behavior for Chrome/Android mobile - the absolute position depends on the viewport position, so for instance a dropdown results menu whose position is set using clonePosition based on its search box (cf scriptaculous' Ajax.Autocompleter) could be positioned in error offset by X to the right of the box, if the view is scrolled to the right by that much when clonePosition is called.

december1981 commented 8 years ago

I've investigated further, this looks like a bug in the browser - not returning a correct result for getBoundingClientRect. I've patched my version up locally, the changes required hunks 1,5 below, to basically force using the default Element.viewportOffset for this browser case. This latter incidentally didn't work without hunk 2 - seems like a prototype issue for the manual calculation of viewport offset? The required scrollLeft/Top values were found in the docBody element's scrollLeft/Top attributes.

Hunks 3 and 4 are what I consider a genuine issue in prototype - when cloning the position of source element "source", the element to be positioned "element" should not consider the page scroll if its offset parent is not the doc body. I confirmed this with a test case I had on my site.


--- prototype.js
+++ prototype.js
@@ -17,6 +17,7 @@
       IE:             !!window.attachEvent && !isOpera,
       Opera:          isOpera,
       WebKit:         ua.indexOf('AppleWebKit/') > -1,
+      MobileChrome:   /Android.*Chrome\/[.0-9]* Mobile/.test(ua), // see here for more: https://developer.chrome.com/multidevice/user-agent
       Gecko:          ua.indexOf('Gecko') > -1 && ua.indexOf('KHTML') === -1,
       MobileSafari:   /Apple.*Mobile/.test(ua)
     }
@@ -4136,10 +4137,8 @@

     element = forElement;
     do {
-      if (element != docBody) {
         valueT -= element.scrollTop  || 0;
         valueL -= element.scrollLeft || 0;
-      }
     } while (element = element.parentNode);
     return new Element.Offset(valueL, valueT);
   }
@@ -4277,11 +4276,12 @@
     element = $(element);
     var p, delta, layout, styles = {};

+    var parent = Element.getOffsetParent(element);
+
     if (options.setLeft || options.setTop) {
       p = Element.viewportOffset(source);
       delta = [0, 0];
       if (Element.getStyle(element, 'position') === 'absolute') {
-        var parent = Element.getOffsetParent(element);
         if (parent !== document.body) delta = Element.viewportOffset(parent);
       }
     }
@@ -4301,8 +4301,7 @@
       return { x: x, y: y };
     }

-    var pageXY = pageScrollXY();
-
+    var pageXY = (parent === document.body) ? pageScrollXY() : { x: 0, y: 0 };

     if (options.setWidth || options.setHeight) {
       layout = Element.getLayout(source);
@@ -4416,18 +4415,22 @@
      !Element.descendantOf(element, document.body);
   }

-  if ('getBoundingClientRect' in document.documentElement) {
-    Element.addMethods({
-      viewportOffset: function(element) {
-        element = $(element);
-        if (isDetached(element)) return new Element.Offset(0, 0);
-
-        var rect = element.getBoundingClientRect(),
-         docEl = document.documentElement;
-        return new Element.Offset(rect.left - docEl.clientLeft,
-         rect.top - docEl.clientTop);
-      }
-    });
+  // getBoundingClientRect seems broken for certain versions of chrome/mobile
+  // applying a blanket for now across all versions
+  if (!Prototype.Browser.MobileChrome) {
+    if ('getBoundingClientRect' in document.documentElement) {
+      Element.addMethods({
+        viewportOffset: function(element) {
+          element = $(element);
+          if (isDetached(element)) return new Element.Offset(0, 0);
+
+          var rect = element.getBoundingClientRect(),
+           docEl = document.documentElement;
+          return new Element.Offset(rect.left - docEl.clientLeft,
+           rect.top - docEl.clientTop);
+        }
+      });
+    }
   }
savetheclocktower commented 8 years ago

I can confirm the issue that is fixed by hunks 3 and 4. Thanks for this report. Fix incoming.

If you can give me a reduced test case that demonstrates the issue that is fixed by hunk 2, I can try to address that too, but right now I don't have enough to go on.