Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getOffsetParent() returns BODY for new hidden elements in IE8 final #156

Closed
jwestbrook opened this issue Apr 22, 2014 · 21 comments
Closed

Comments

@jwestbrook
Copy link
Collaborator

previous lighthouse ticket #618
by rvagg


If you insert a new element into the DOM that's hidden (display:none;) getOffsetParent() returns BODY no matter where you put it. If it's shown when you insert it it will return the correct element.

This problem does not exist in the IE8 that's in Windows 7 Beta, I'm not sure about the other pre-final releases of IE8 but I suspect this has crept into the final release.

Run attached file to see a demonstration, it uses clonePosition() to demonstrate that getOffsetParent() returns the wrong element for working out the positioning delta and therefore places the element in the wrong place (by a long way).

Work-around is to not use display:none; on your new elements.

See also http://groups.google.com/group/prototype-scriptaculous/browse_thread/thread/186e505aea6873db for mention of this problem.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>
<body>
<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/prototype/1.6.0.3/prototype.js"></script>
<script type="text/javascript" language="javascript">
document.observe('dom:loaded', function() {
    var outp = function(t) {
        $('outp').insert({ bottom: new Element('p').update(t) });
    };
    var outppos = function(ele) {
        outp(ele.id + ' pos: ' + ele.viewportOffset()[1] + ',' + ele.viewportOffset()[0] + ' (should be 20,20)');
    };
    var outpparent = function(ele) {
        outp(ele.id + ' offset parent: ' + ele.getOffsetParent().tagName + '[' + ele.getOffsetParent().id + '] (should be DIV[srcele])');
    };
    var newele = function(prop) {
        var newele = new Element('div', prop).update(prop.id);
        $('srcele').insert({ bottom: newele });
        outpparent(newele);
        newele.clonePosition($('srcele'));
        newele.show();
    };
    newele({ id: 'newhide', style: 'position: absolute; display: none; background-color: #FFFF00; border-color: #00FF00; border: 1px transparent;' });
    newele({ id: 'newshow', style: 'position: absolute; background-color: #FF00FF; border-color: #FF0000; border: 1px transparent;' });
    outppos($('srcele'));
    outppos($('newhide'));
    outppos($('newshow'));
});
</script>

<div id="srcele" style="background-color:#00FFFF; border: 1px solid #0000FF; position: absolute; top: 20px; left: 20px; width: 50px; height: 50px;">src</div>
<div id="outp" style="margin-top: 100px;"></div>

</body>
</html>
@jwestbrook
Copy link
Collaborator Author

Serty Oan
March 27th, 2009 @ 02:19 PM

indeed IE8 offsetParent is the document.body for display none new elements a partial correction in Element.Methods could be :

getOffsetParent: function(element) {
if (element.offsetParent && element.offsetParent != document.body) return $(element.offsetParent);
if (element == document.body) return $(element);

while ((element = element.parentNode) && element != document.body)
  if (Element.getStyle(element, 'position') != 'static')
    return $(element);

return $(document.body);

}

anyway still get a problem as i get in IE 8 :
srcele pos: 20,20 (should be 20,20) newhide pos: 21,21 (should be 20,20) newshow pos: 21,21 (should be 20,20)

@jwestbrook
Copy link
Collaborator Author

rvagg
March 28th, 2009 @ 01:08 AM

21,21 is because of the 1px border on 'srcele' I guess, I put that in there at the last moment and didn't even notice that it introduced a 1px shift, even without your patch.
Maybe a new bug all together? IE7 & IE6 get the 1px right and report 20,20.

@jwestbrook
Copy link
Collaborator Author

rvagg
March 28th, 2009 @ 03:57 AM

After switching the example file to 1.6.1 RC2 (http://prototypejs.org/assets/20... and running with my bunch of test browsers I get the following:

  • IE8 final fails
  • IE8 that's in Windows 7 Beta does what is expected
  • IE 7 & 6 do what is expected
  • Firefox 3.1 & 2 do what is expected
  • Chrome & Chrome Beta (current) do what is expected
  • Safari 3 * 4 Beta do what is expected
  • Opera 9.64 nearly does it right but shifts both new elements 1px down and right due to the border (i.e. reports newhide and newshow as 21,21 but they should be 20,20), as I said I suspect this is probably a separate bug to be addressed.

I don't think there's really anything different between 1.6.0.3 and 1.6.1 RC2 for this bug but I guess this would be an important issue to address for "IE8 compatibility"?

@jwestbrook
Copy link
Collaborator Author

rvagg
June 17th, 2009 @ 02:27 AM

This is still an issue with RC3. So having further dug into the issues involved here I can report the following:
When an element is hidden, IE8 returns a non-null offsetParent (HTMLBodyElement) while other browsers return a null. So in getOffsetParent() IE8 shortcuts at the "if (element.offsetParent) return $(element.offsetParent);" and returns document.body, while the other browsers have to go into the loop to climb up .parentNode's to find a non-static element. Hence IE8 returns document.body while the others return the direct parent element: 'srcele'.
For the non-hidden element, they all behave the same and return an offsetParent of the direct-parent ('srcele'). Which means quick execution of getOffsetParent().
So, it would seem to me that it's only misbehaving in the case of a non-visible element so we could add a tiny bit of overhead to the first 'if' inside getOffsetParent() to check for visibility:

  getOffsetParent: function(element) {
    if (element.offsetParent && Element.visible(element)) return $(element.offsetParent);

(why are we using lighthouse rather than trac now? this is so much harder to use and navigate... or does it have some special features I'm not aware of that make it really attractive?)

@jwestbrook
Copy link
Collaborator Author

BillyRayPreachersSon
June 24th, 2009 @ 12:45 PM

Further debugging shows the problem only seems to exist when certain CSS is in play.
This problem first came to light for me when I found that the scriptaculous autocompleter was displaying way off to the right in IE8. Because I had no idea where the problem was, and only one other person had reported the issue, I initially opened this thread about it:
http://groups.google.com/group/prototype-scriptaculous/browse_thread/thread/8aac295a1affa481/cfd5a26a84816cc5#cfd5a26a84816cc5

If you have a website that uses "margin: 0px auto" to centre itself, and you clone a node's position onto a node that is initially hidden, all browsers except IE8 will play ball. It's also an issue if you remove the auto margins and use left padding on the BODY. Either way, IE6 and IE7, Fx3, etc are all fine - just not IE8.
Note that removing any padding from the body, and not auto-centring renders both boxes as expected, so it seems to be only with this (fairly standard) CSS in play that the problem occurs.
Here's a test harness showing the issue. It replicates the HTML that the scriptaculous autocompleter would use.
The upper drop-down box is initially hidden before having a position cloned onto it, and the lower is not initially hidden. You can clearly see the difference in IE8.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
    <title>Position cloning bug with IE8</title>
    <meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>

    <style type="text/css">
        html, body {
            padding: 0px;
            margin: 0px;
        }
        body {
            text-align: center;
        }
        #centreWrapper {
            width: 800px;
            margin: 0px auto;
            text-align: left;
            background-color: #CCCCCC;
        }


        .itemSelectionWrapper {
            position: relative;
            padding: 20px;
        }
        .itemSelectorRow {
            margin: 10px;
        }


        div.autocomplete {
            position: absolute;
            width: 250px;
            background-color: #FFFFFF;
            border: 1px solid #888888;
            margin: 0px;
            padding: 0px;
        }
        div.autocomplete ul,
        div.autocomplete li {
            list-style-type: none;
            margin: 0px;
            padding: 0px;

        }
    </style>

    <script type="text/javascript" src="prototype161rc3.js"></script>

    <script type="text/javascript">
        document.observe('dom:loaded', showClonedBoxes);

        function showClonedBoxes() {
            cloneAndShow($('theItem'), $('itemSuggestions'));
            cloneAndShow($('anotherItem'), $('anotherItemSuggestions'));
        }

        function cloneAndShow(sourceToClone, clonedNode) {
            Element.clonePosition(clonedNode, sourceToClone, {
                setHeight: false,
                offsetTop: sourceToClone.offsetHeight
            });

            Element.show(clonedNode);
        }
    </script>
</head>

<body>
    <div id="centreWrapper">

        <div class="itemSelectionWrapper">
            <form>
                <fieldset>
                    <legend>Item Selection</legend>

                    <div class="itemSelectorRow">
                        <label>Item</label>
                        <input type="text" name="theItem" id="theItem" value="" size="80" />
                        <div id="itemSuggestions" class="autocomplete" style="display:none;">
                            <ul><li class="selected"><strong>I</strong>tem 1</li><li class=""><strong>I</strong>tem 2</li><li class=""><strong>I</strong>tem 3</li></ul>
                        </div>
                    </div>
                </fieldset>
            </form>
        </div>

        <br />

        <div class="itemSelectionWrapper">
            <form>
                <fieldset>
                    <legend>Another Item Selection</legend>

                    <div class="itemSelectorRow">
                        <label>Another Item</label>
                        <input type="text" name="anotherItem" id="anotherItem" value="" size="80" />
                        <div id="anotherItemSuggestions" class="autocomplete">
                            <ul><li class="selected"><strong>I</strong>tem 1</li><li class=""><strong>I</strong>tem 2</li><li class=""><strong>I</strong>tem 3</li></ul>
                        </div>
                    </div>
                </fieldset>
            </form>
        </div>

    </div>
</body>
</html>

Note, I'm running the latest version of IE 8 under Windows XP.
Dan

@jwestbrook
Copy link
Collaborator Author

Steffen Bartsch
July 6th, 2009 @ 02:13 PM

rvagg's patch works great for my problems with mis-positioned scriptaculous autocompletes in IE8. Is this already on its way into the prototype repository?

@jwestbrook
Copy link
Collaborator Author

x
July 22nd, 2009 @ 05:21 PM

Vote for rvagg's patch too.
Works perfect, tested in FF3, Opera 9.5, IE7, IE8

@jwestbrook
Copy link
Collaborator Author

Shridhar N S
August 28th, 2009 @ 01:20 AM

rvagg's patch works fine for my problems too.

@jwestbrook
Copy link
Collaborator Author

Keith Davis
August 31st, 2009 @ 08:14 PM

how do we vote for patches?
anyway, rvagg's works good for me as well.

@jwestbrook
Copy link
Collaborator Author

Jacob Kjeldahl
October 28th, 2009 @ 02:22 PM

rvagg's patch works for me as well.

@jwestbrook
Copy link
Collaborator Author

Joey Novak
October 30th, 2009 @ 01:16 AM

I checked the git repo and this change isn't in there yet. What do we need to do to get it there? Here is a patch file that fixes the git repo if you check it out.
Joey

diff --git a/src/dom/dom.js b/src/dom/dom.js
index 0f0e247..2a0addf 100644
--- a/src/dom/dom.js
+++ b/src/dom/dom.js
@@ -1291,7 +1291,7 @@ Element.Methods = {
    *  `body` element is returned.
   **/
   getOffsetParent: function(element) {
-    if (element.offsetParent) return $(element.offsetParent);
+    if (element.offsetParent  && Element.visible(element)) return $(element.offsetParent);
     if (element == document.body) return $(element);

     while ((element = element.parentNode) && element != document.body)

@jwestbrook
Copy link
Collaborator Author

Dan
November 21st, 2009 @ 06:25 PM

I tested the patch in IE 8, IE 6, Opera Windows/Linux, Firefox Linux - all seems to work fine

@jwestbrook
Copy link
Collaborator Author

Stephen Heuer
October 15th, 2010 @ 07:37 AM

Why was rvagg's fix never be applied to 1.6? The one that Joey Novak even put into a patch file?
Does anyone know if the new 1.7 rc's have this issue?

@jwestbrook
Copy link
Collaborator Author

lakshmanan
December 23rd, 2010 @ 07:47 PM

rvag's patch "didn't" work for me. I am using Prototype 1.7.rc2 and scriptaculous 1.8.3
I am using scriptaculous autocompleter to give out some autosuggestions to user. The placement of div that will contain the ajax ul response from server is not getting placed rightly under the textfield (that has autocomplete ).
Is there any other solution for this problem

@jwestbrook
Copy link
Collaborator Author

Clément Hallet
February 18th, 2011 @ 12:45 PM

How this patch would be applied on the 1.7 version ?

@jwestbrook
Copy link
Collaborator Author

Keith Davis
February 18th, 2011 @ 06:36 PM

Line 3701:

if (!isInline && element.offsetParent && Element.visible(element)) return 
$(element.offsetParent);
function getOffsetParent(element) {
    element = $(element);

    if (isDocument(element) || isDetached(element) || isBody(element) || isHtml(element))
      return $(document.body);

    var isInline = (Element.getStyle(element, 'display') === 'inline');
    if (!isInline && element.offsetParent && Element.visible(element)) return $(element.offsetParent);

    while ((element = element.parentNode) && element !== document.body) {
      if (Element.getStyle(element, 'position') !== 'static') {
        return isHtml(element) ? $(document.body) : $(element);
      }
    }

    return $(document.body);
  }

@jwestbrook
Copy link
Collaborator Author

Keith Davis
August 13th, 2012 @ 08:55 PM

I cannot believe this wasn't fixed in 1.7.1!!!!

@jwestbrook
Copy link
Collaborator Author

Keith Davis
February 1st, 2013 @ 07:57 PM

Does anyone know if there is any development being on Prototype?

@jwestbrook
Copy link
Collaborator Author

Yes Prototype is still being actively developed - check the github https://github.com/sstephenson/prototype

@jwestbrook
Copy link
Collaborator Author

Anthony Mckale
March 25th, 2013 @ 12:55 PM

This code someone committed works for me, was finding the "Element.visible(element)" hack didn't work unfortunately

function getOffsetParent(element) {
element = $(element);
if (isDocument(element) || isDetached(element) || isBody(element) || isHtml(element))
  return $(document.body);

var isInline = (Element.getStyle(element, 'display') === 'inline');
// WORKAROUND : MSIE 8 populating offsetParent
if (!isInline && element.offsetParent && element.offsetParent != document.body) return $(element.offsetParent);

while ((element = element.parentNode) && element !== document.body) {
  if (Element.getStyle(element, 'position') !== 'static') {
    return isHtml(element) ? $(document.body) : $(element);
  }
}

return $(document.body);
} 

@jwestbrook
Copy link
Collaborator Author

Looks like this patch was already integrated into the 1.7.2 release - closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant