#156 ✓inactive
Ricky

Fixed bug in $() and getElementById

Reported by Ricky | June 9th, 2008 @ 10:20 PM

IE7 and Opera 9.27 (but not IE8, I'm told) will find <tag name="whatever"> when you are looking for <tag id="whatever">.

The easiest way to see the ie7 bug is the attached file animalcow.html, which is standalone except that it uses prototype.js. Use 1.6.0.2 to see the bug.

This fix overrides and replaces getElementById() to be W3C compliant (and ie8 compliant) and therefore fixes $().

The fix is in dom.js, in the beginning. The test code for ie7 and opera is in dom_test.js, and the html of the tests is in dom.html. All changes are in the attached patch.

I think I can attach 2 different files. We'll see.

The key is to use the non-standard document.all on browsers that have it. Luckily, this seems to be the same set as the browsers that have the bug.

I've tested the fixed prototype.js on Windows XP SP3 only, with Firefox 3 RC2 IE7 Opera 9.27 Safari 3.1.1

All 4 browser say '24 assertions, 0 failures, 0 errors' for dom_test.html in testDollarFunction.

Credit is due to: sixteen small stones and to web bug track

PROS: Fixes the bug.

CONS: My changes to dom_test.js seem verbose. The code changes the dom routine, getElementById(), which could break some already broken code. You can change that by changing a function name and a couple-of-line edit to dom.js. This is a philosophical question for the prototype developers.

Comments and changes to this ticket

  • Juriy Zaytsev

    Juriy Zaytsev June 10th, 2008 @ 03:56 AM

    Interesting. How does it affect performance?

  • Tobie Langel

    Tobie Langel June 16th, 2008 @ 02:10 AM

    This has been discussed before and wasn't integrated into core for performance reasons.

    Whatever is decided this time, I'm not very keen on the idea of overwriting the existing `document.getelementById` method.

  • Tobie Langel

    Tobie Langel June 16th, 2008 @ 02:10 AM

    • State changed from “new” to “enhancement”
  • Juriy Zaytsev

    Juriy Zaytsev June 22nd, 2008 @ 01:08 AM

    Changes like this is always a tough decision.

    On one hand, such quirks could waste hours of valuable time (if a developer is working on an outdated/developed-by-many-people project) and it is a library's job to prevent this from happening.

    On the other hand, one small change in $ could hog the entire library, bringing an app to its knees (performance-wise).

    We could solve problems like these with a global switch, separate function or an optional argument (passed to $).

    Introducing separate function is problematic - considering that $ is a global variable (and has such unique name).

    Optional argument to a $ is problematic as well - function already accepts N number of them, so these changes would break compatibility.

    Global switch seems like a pretty good solution. Something like $.IGNORE_NAMES = true; (which would be false by default)

    Frameworks like YUI already use global configuration. I'm not sure why prototype avoids this.

    Thoughts?

  • Ricky

    Ricky June 22nd, 2008 @ 09:07 AM

    Unless we do something, the $() is not working as advertised. This has got to be a bug. We should change the documentation or change the code.

    For IE7, the broken getElementById function actually works according to Microsoft's IE7 documentation, so one could argue it's not a bug. It violates the W3C spec, so you could argue the other way, too. I'll let the philosophers decide that one.

  • John-David Dalton

    John-David Dalton July 2nd, 2008 @ 05:10 PM

    • Tag set to dom, ie, ie6, ie7, needs_patch, needs_tests, opera

    I don't see this as a big deal.

    Its a bug in the browser and any tinkering with it to try to fix it will only slow down this core method.

    We could do

    function $(element) {
      if (arguments.length > 1) {
        for (var i = 0, elements = [], length = arguments.length; i < length; i++)
          elements.push($(arguments[i]));
        return elements;
      }
      if (Object.isString(element)) {
        var id = element;
        element = document.getElementById(id);
    
        if (document.all && id === element.getAttribute('name') &&
            id !== element.getAttribute('id')) {
          var elements = document.all[id], length = elements.length || 0;
          while (length--) {
            if (elements[length].getAttribute('name') !== id &&
                elements[length].getAttribute('id') === id)
              return Element.extend(elements[length]);
          }
          return null;
        }
      }
      return Element.extend(element);
    }
    
  • Juriy Zaytsev

    Juriy Zaytsev July 2nd, 2008 @ 05:51 PM

    John,

    Using global switch gives control to the hands of developers.

    There's no slowdown unless you know what you are doing.

    Regarding browser bugs, 80% of what's in prototype.js is a workaround for them ;)

    I actually find this addition useful.

  • John-David Dalton

    John-David Dalton July 2nd, 2008 @ 08:20 PM

    Oh ya! I like switches too :)

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:10 AM

    • Tag changed from dom, ie, ie6, ie7, needs_patch, needs_tests, opera to ie, ie6, ie7, needs_patch, needs_tests, opera, section:dom

    [not-tagged:"dom" tagged:"section:dom" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:26 AM

    • Tag changed from ie, ie6, ie7, needs_patch, needs_tests, opera, section:dom to ie, ie6, ie7, missing:tests, needs_patch, opera, section:dom

    [not-tagged:"needs_tests" tagged:"missing:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:28 AM

    • Tag changed from ie, ie6, ie7, missing:tests, needs_patch, opera, section:dom to ie, ie6, ie7, missing:patch, missing:tests, opera, section:dom

    [not-tagged:"needs_patch" tagged:"missing:patch" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:36 AM

    • Tag changed from ie, ie6, ie7, missing:patch, missing:tests, opera, section:dom to ie, ie6, ie7, missing:patch, needs:tests, opera, section:dom

    [not-tagged:"missing:tests" tagged:"needs:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:37 AM

    • Tag changed from ie, ie6, ie7, missing:patch, needs:tests, opera, section:dom to ie, ie6, ie7, needs:patch, needs:tests, opera, section:dom

    [not-tagged:"missing:patch" tagged:"needs:patch" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 04:05 AM

    • Tag changed from ie, ie6, ie7, needs:patch, needs:tests, opera, section:dom to needs:patch, needs:tests, opera, section:dom

    [not-tagged:"ie" not-tagged:"ie6" not-tagged:"ie7" not-tagged:"ie8" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 12:44 PM

    • Tag changed from needs:patch, needs:tests, opera, section:dom to needs:patch, needs:tests, section:dom

    [not-tagged:"opera" bulk edit command]

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    [responsible:none bulk edit command]

  • Franco Monsalvo

    Franco Monsalvo March 31st, 2010 @ 11:34 PM

    Ricky can you please add some benchmarks to compare the performance between your patch and the current implementation?
    Thanks.

  • Franco Monsalvo

    Franco Monsalvo April 11th, 2010 @ 12:54 AM

    • State changed from “enhancement” to “inactive”

    I'm closing this ticket since it has been inactive for quite a while now.
    if you think this is still in issue in the current version of Prototype, please feel free to reopen the ticket

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

Pages