#1040 ✓ resolved
Dan Popescu

getPixelValue(width) failure when called from Layout's _begin and width is a percent value

Reported by Dan Popescu | April 23rd, 2010 @ 10:11 AM | in 1.7

Latest GitHub version

In Layout, in _begin function, there are 2 calls to getPixelValue(width), where width=element.getStyle('width')
If width is a percentage value (like '100%') getPixelValue(width) fails

Comments and changes to this ticket

  • Dan Popescu

    Dan Popescu April 23rd, 2010 @ 10:27 AM

    • Tag changed from dom to dom, needs:patch

    This can be fixed by replacing getPixelValue(width) by getPixelValue(element,'width')

      if (width && (positionedWidth === width)) {
    --    newWidth = getPixelValue(width);
    ++    newWidth = getPixelValue(element,'width');
      } else if (width && (position === 'absolute' || position === 'fixed')) {
    --    newWidth = getPixelValue(width);
    ++    newWidth = getPixelValue(element,'width');
      } else {
    
  • Riccardo De Agostini

    Riccardo De Agostini May 14th, 2010 @ 05:41 PM

    Bug still present in 1.7_rc2

  • Andrew Dupont

    Andrew Dupont May 18th, 2010 @ 12:14 AM

    • Milestone set to 1.7
    • State changed from “new” to “bug”
    • Assigned user set to “Andrew Dupont”

    Will fix for the next RC. Thanks!

  • Andrew Dupont

    Andrew Dupont May 18th, 2010 @ 03:01 AM

    Actually, I can't reproduce this. I wrote a few simple unit tests for percentage widths (handling both statically-positioned and absolutely-positioned elements), and they all pass without making the above modification. Can you tell me more about how to reproduce this bug, and in which browser(s)?

  • Dan Popescu

    Dan Popescu May 18th, 2010 @ 09:47 AM

    This shows the problem

    
    <!DOCTYPE HTML PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
    <html id="" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml" lang="en">
      <head>
        <meta http-equiv="content-type" content="text/html; charset=UTF-8"/>
        <title>Title</title>
        <script type="text/javascript" src="prototype.js"></script>
      </head>
      <body>
        <div style='width: 100%; display: none;'>
          <div id='badwidth' style='width: 100%; display: none;'>
            Width: 100%
          </div>
        </div>
      </body>
    </html>
    
    
    $('badwidth').getWidth()
    
  • Dan Popescu

    Dan Popescu May 18th, 2010 @ 09:55 AM

    Latest github prototype, Windows Vista 32
    - firefox 3.6.3, firebug 1.6X.0a10 - google chrome 5.0.396.0 dev - safari 4.0.5 (531.22.7)

    The error is:
    ReferenceError: Can't find variable: element - prototype.js:3299

  • Riccardo De Agostini

    Riccardo De Agostini May 18th, 2010 @ 10:37 AM

    Here's a failing test case. Tested with Firefox 3.6.3 using Firebug.

    Although the code in this example looks (and is) quite useless, it summarizes what happened in an application of mine last Friday; I was going to open a ticket after finding out where the actual problem lied, and my patch was but I saw that there was already this one opened for the same bug.

    <!doctype html>
    <html>
      <head>
        <title>Prototype Ticket #1040 test</title>
        <script type="text/javascript" src="prototype-1.7_rc2.js"></script>
      <body>
        <div id="container" style="position: fixed; left: 0; top: 0; right: 0; bottom: 0">
          <div id="test" style="width:50%">This div is <span id="val">quite a few</span> pixel wide.</div>
        </div>
        <script type="text/javascript">
          document.observe("dom:loaded", (function() {
            var container = $("container");
            container.hide();
            var layout = new Element.Layout("test", true); // <-- Fail
            var w = layout.get("width");
            $("val").update(w);
            container.show();
          }));
        </script>
      </body>
    </html>
    

    Dan's patch works fine for me too.

    Also, I noticed that in the very same function, element is not declared as a local variable.

  • Riccardo De Agostini

    Riccardo De Agostini May 18th, 2010 @ 10:54 AM

    Oops... I started updating, then answered some phone calls and in the meantime Dan wrote his own report.

    Maybe the fact that element is not local explains the ReferenceError; the "%" issue still stands, though.

  • Dan Popescu

    Dan Popescu May 18th, 2010 @ 12:02 PM

    As I understand, getPixelValue can be used as:
    1) some sort of text to number conversion function (if used without an element)
    2) a calculator for the requested property of an element (if used with element and property parameters)

    The problem with 'dd%' is that it cannot be computed independent of an element.

  • Andrew Dupont

    Andrew Dupont May 18th, 2010 @ 07:48 PM

    The problem with 'dd%' is that it cannot be computed independent of an element.

    I slap my forehead in embarrassment. (It's been too long since I wrote this code!)

    Dan, your test is wrong because we don't support measuring elements whose parents are hidden. (It becomes a recursion nightmare.) That test might work after this patch is applied, but you're on your own :)

    Thanks to you both!

  • Dan Popescu

    Dan Popescu May 19th, 2010 @ 10:33 AM

    I understand that.
    What I want to say is that at least there should be some checks in getPixelValue so that it doesn't throw an error.
    It's a missed case (getPixelView called with only a text parameter that is a percent value on an hidden element with hidden parent) and then some code that should be executed only if element and property parameters are available is executed.

    Thank you,
    Dan Popescu

  • Andrew Dupont

    Andrew Dupont May 24th, 2010 @ 04:33 AM

    Dan, I think your patch will address the issue. (I was slow to apply it because I wanted to make sure it wouldn't cause any regressions.) Also, I've made sure to declare an element variable in getPixelValue and ensure it's set. That should prevent exceptions. It should also properly convert percentage widths to pixel widths in all supported scenarios.

    Riccardo: I should point out that your test case is just like Dan's. We don't support measurement of an element when its parent is hidden. We might do so for a later release; if you would like to write the patch to enable it, I'll be happy to include it.

  • Riccardo De Agostini

    Riccardo De Agostini May 24th, 2010 @ 09:35 AM

    Thanks, Andrew. I modified my application so it won't need to measure elements whose parent is hidden; everything works fine with my patch which, I suppose, is nearly identical to yours (minus test cases, CHANGELOG update and the like, obviously, so I think I'll undo it and apply your patch as soon as it's on GitHub).

    A patch to measure children of hidden elements is in my plans, I just don't think it'll be ready for version 1.7.0 because of approaching work deadlines. I already have some working code for something similar, but alas, no test cases, no docs, and a coding style which I strenuously defend but is different from the Prototype standard (which of course I'm going to follow for any patch I intend to submit).

  • GitHub Robot

    GitHub Robot May 28th, 2010 @ 12:56 AM

    • State changed from “bug” to “resolved”

    (from [ec15b2c0a916725dc56c1d536dc383eaf109b97c]) Ensure Element.Layout gives accurate measurements for percentages on all elements. Fix inaccurate measurements on position: fixed elements with percentages. [#1040 state:resolved] (Dan Popescu, Riccardo De Agostini, Andrew Dupont) http://github.com/sstephenson/prototype/commit/ec15b2c0a916725dc56c...

  • Andrew Dupont

    Andrew Dupont May 28th, 2010 @ 12:59 AM

    Patch applied; let me know if something is still amiss. Also noticed we weren't handling position: fixed properly, so I put in some logic that computes percentages relative to the viewport in those cases (instead of the parent).

  • Riccardo De Agostini

    Riccardo De Agostini May 28th, 2010 @ 09:39 AM

    • Tag changed from dom, needs:patch to dom

    Patch reapplied on my fork, replacing my own patch. Seems to work flawlessly so far, although I admit I don't have time for exhaustive tests. Thanks again, Andrew, you're officially one of my heroes! :-D

  • Dan Popescu

    Dan Popescu May 31st, 2010 @ 11:44 AM

    Sorry for the delay. Everything seems in order now, thank you.

  • goldrolex

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

Tags

Referenced by

Pages