#676 enhancement
bs4 (at davywavy)

code consistency: tagName, undefined

Reported by bs4 (at davywavy) | May 13th, 2009 @ 06:26 AM

I noticed that prototype uses element.tagName almost exclusively except for line 1844 in RC2 (the definition for Element.Methods.update) which uses nodeName:

update: (function(){

var SELECT_ELEMENT_INNERHTML_BUGGY = (function(){
  var el = document.createElement("select"),
      isBuggy = true;
  el.innerHTML = "<option value=\"test\">test</option>";
  if (el.options && el.options[0]) {
    isBuggy = el.options[0].nodeName.toUpperCase() !== "OPTION";
  }
  el = null;
  return isBuggy;
})();

In the interest of consistency I propose that prototype use only element.tagName

================================================================================

In a similar interest I noticed that most of prototype RC2 tests for undefined values using:
if( typeof obj === 'undefined' )
or:
if( typeof obj !== 'undefined' )

which is indeed rock solid. I propose that prototype use this throughout in the interest of consistency. This will require a change to
line 520: if (value != undefined) value = decodeURIComponent(value);
line 4082:
(shouldn't that be: if( typeof value !== 'undefined' ) ?) line 1704: this.updater.options.onComplete = undefined;
(shouldn't that be: delete this.updater.options.onComplete; ?) line 1780: ? (typeof elForm.elements.test == "undefined")
line 1855:
line 3607:
(should be === 'undefined' ... notice the use of === and single quotes ) line 2308: element.madePositioned = undefined;
(shouldn't that be: delete element.
madePositioned; ?) line 2920: if (typeof window.Element != 'undefined') {
line 2971:
(should be !== ) line 3586: node.countedByPrototype = undefined;
(shouldn't that be: delete node.
countedByPrototype; ?)

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel May 13th, 2009 @ 01:09 PM

    • Tag changed from cleanup to cleanup, needs_patch
    • State changed from “new” to “enhancement”

    Could you please provide patches for all of those? As far as I can see, some of your suggested changes would break things, notably:

    520: if (value != undefined) rejects both undefined AND null values.
    2308, 3586: deleting properties of elements throws errors in IE.

    So please make sure to run tests!

  • Juriy Zaytsev

    Juriy Zaytsev May 13th, 2009 @ 11:21 PM

    @bs4 (at davywavy)

    delete o.prop is not the same as o.prop = undefined. Former one deletes a property, while latter - only assigns a value of a global undefined property to o.prop (making it possible to "detect" o.prop existence with, say, in operator).

    delete should also only be used with native objects, as Tobie mentioned (since MSHTML DOM does not implement [[Delete]] on some of the host objects and is known to throw errors)

    I would not use undefined for comparison (or anything else) at all. First, global undefined property can be overwritten (and so break the entire library); second, global undefined is missing from some of the older browsers. Even though both points are somewhat moot, it's better to be safe than sorry (specially when simple alternatives, such as using void operator, are available).

    It's also worth noting that Object.isUndefined(foo) is not quite the same as typeof foo == 'undefined' since former evaluates foo (and so may throw error if foo is not found in the scope chain), while latter doesn't. When evaluating a reference, internal [[GetValue]] method is called. The problem is that this [[GetValue]] also throws on some of the host objects in IE (presumably, when these objects are implemented as ActiveX controls).

    typeof prevents these issues quite reliably.

  • Tobie Langel

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

    • Tag changed from cleanup, needs_patch to missing:patch

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

  • Tobie Langel

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

    • Tag changed from missing:patch to needs:patch

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

  • T.J. Crowder

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

    [responsible:none bulk edit command]

  • Tisho Georgiev

    Tisho Georgiev March 1st, 2010 @ 08:55 PM

    • Tag changed from needs:patch to needs:patch, section:dom
  • Tisho Georgiev

    Tisho Georgiev March 1st, 2010 @ 08:56 PM

    • Tag changed from needs:patch, section:dom to general, needs:patch

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