#1139 new

Element.clonePosition delta wrong

Reported by capripot | September 12th, 2010 @ 11:31 PM


I don't know if it's a really bug but the Element.clonePosition not work as expected when you change the margin of the < body > element.
For exemple, I usally set margin left and right to auto on the body element to center it and economize a "uggly wrapper div" in my code.

But with the Autocompleter from scriptaculous that use the clonePosition function, the result div is not in the right place.

When i've searched, i found the problem is in prototypejs.
Line 2536, you have this lines :

if (parent == document.body) {
  delta[0] -= document.body.offsetLeft;
  delta[1] -= document.body.offsetTop;

and under Safari 5 and Firefox 3.6.9 / Max OS X 10.6, these values are equals to 0
But when you get the reals offset with


you get correct values.

So if you replace the above lines by

if (parent == document.body) {
  d2 = Element.viewportOffset(document.body)
  delta[0] -= d2.left;
  delta[1] -= d2.top;

You have the correct position.

But you have just do this with "delta" var (because of "parent == document.body"), so you just have to do that.

if (Element.getStyle(element, 'position') == 'absolute') {
  parent = Element.getOffsetParent(element);
  delta = Element.viewportOffset(parent);
if (parent == document.body) {
  delta[0] -= delta.left;
  delta[1] -= delta.top;

or just

if (parent == document.body)
  delta[0] = delta[1] = 0;

That's code work perfectly.
What do you think about this patch ?

-- Maybe we can correct the values offsetLeft and offsetTop of the body element, it could be better. But i don't know how to do this and the final don't use it. --

Thanks for the reading, and i hope a response :)

Comments and changes to this ticket

  • Matt Haggard

    Matt Haggard February 1st, 2011 @ 12:09 AM

    Does this test addition pass with your patch?

  • Matt Haggard

    Matt Haggard February 1st, 2011 @ 12:25 AM

    Nope, this test still doesn't pass even after applying this patch:

    diff --git a/src/prototype/dom/dom.js b/src/prototype/dom/dom.js
    index 713826f..eabf0a8 100644
    --- a/src/prototype/dom/dom.js
    +++ b/src/prototype/dom/dom.js
    @@ -2609,8 +2609,7 @@ Element.Methods = {
         // correct by body offsets (fixes Safari)
         if (parent == document.body) {
    -      delta[0] -= document.body.offsetLeft;
    -      delta[1] -= document.body.offsetTop;
    +      delta = [0, 0];
         // set position
    $ rake test BROWSERS=firefox,chrome,safari TESTS=dom
    Started tests in Google Chrome.
    Finished in 1.435653 seconds.
      Failures: /tmp/dom_test.html
    98 tests, 924 assertions, 5 failures, 0 errors.
    Started tests in Firefox.
    Finished in 2.788141 seconds.
      Failures: /tmp/dom_test.html
    98 tests, 926 assertions, 2 failures, 0 errors.
    Started tests in Safari.
    Finished in 1.168346 seconds.
      Failures: /tmp/dom_test.html
    98 tests, 927 assertions, 2 failures, 0 errors.

    So you don't have to click on the patch file, here it is for reference:

    diff --git a/test/unit/dom_test.js b/test/unit/dom_test.js
    index 1c67a53..943e239 100644
    --- a/test/unit/dom_test.js
    +++ b/test/unit/dom_test.js
    @@ -1577,6 +1577,25 @@ new Test.Unit.Runner({
         // unregistered.
         this.assert(!trigger, "fired event should not have triggered handler");
    +  },
    +  testClonePosition: function() {
    +    var element = new Element('div').update('dolly');
    +    $(document.body).insert(element);
    +    element.absolutize();
    +    var reference = $('test-clone-position-reference');
    +    element.clonePosition(reference);
    +    elayout = element.getLayout();
    +    rlayout = reference.getLayout();
    +    this.assertEqual(elayout.get('width'), rlayout.get('width'), "Element widths should be the same");
    +    this.assertEqual(elayout.get('height'), rlayout.get('height'), "Element heights should be the same");
    +    this.assertEqual(elayout.get('left'), rlayout.get('left'), "Layout.left should be the same");
    +    this.assertEqual(elayout.get('top'), rlayout.get('top'), "Layout.top should be the same");
    +    this.assertEqual(elayout.get('bottom'), rlayout.get('bottom'), "Layout.bottom should be the same");
    +    this.assertEqual(elayout.get('right'), rlayout.get('right'), "Layout.right should be the same");
    diff --git a/test/unit/fixtures/dom.html b/test/unit/fixtures/dom.html
    index 99395ee..fa8a790 100644
    --- a/test/unit/fixtures/dom.html
    +++ b/test/unit/fixtures/dom.html
    @@ -275,4 +275,9 @@
     <div id='elementToViewportDimensions' style='display: none'></div>
    -<div id="auto_dimensions" style="height:auto"></div>
    \ No newline at end of file
    +<div id="auto_dimensions" style="height:auto"></div>
    +<div style="margin: 32px;"></div>
    +<div style="margin: 20px;">
    +  <div id="test-clone-position-reference">sheep</div>

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

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

The Prototype JavaScript library.

Shared Ticket Bins