Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.javascript > #19423 > unrolled thread

Problem with IE, either with "window.event" or failure to prevent default return on click

Back to article view | Back to comp.lang.javascript

Contents

  Problem with IE, either with "window.event" or failure to prevent default return on click Mason Barge <masonbarge@gmail.com> - 2013-04-23 09:57 -0400
    Re: Problem with IE, either with "window.event" or failure to prevent default return on click Thomas 'PointedEars' Lahn <PointedEars@web.de> - 2013-04-24 11:02 +0200
      Re: Problem with IE, either with "window.event" or failure to prevent default return on click Mason Barge <masonbarge@gmail.com> - 2013-04-24 17:44 -0400
        Re: Problem with IE, either with "window.event" or failure to prevent default return on click Thomas 'PointedEars' Lahn <PointedEars@web.de> - 2013-04-25 01:31 +0200
          Re: Problem with IE, either with "window.event" or failure to prevent default return on click Martin Leese <please@see.Web.for.e-mail.INVALID> - 2013-04-25 09:37 -0600
          Re: Problem with IE, either with "window.event" or failure to prevent default return on click Mason Barge <masonbarge@gmail.com> - 2013-04-25 12:44 -0400
            Re: Problem with IE, either with "window.event" or failure to prevent default return on click Thomas 'PointedEars' Lahn <PointedEars@web.de> - 2013-04-26 11:25 +0200

#19423 — Problem with IE, either with "window.event" or failure to prevent default return on click

 favorite
	

Yes I'm a noob, but I'm at my wit's end as I've tried everything I can
think of. This works fine in browsers other than IE (including IE9)
and I'm not sure if the function fails or if it works but the "return
false" mechanism fails. I've written it out in longhand trying to fix
it:

I have a blue-bordered box on a page, with a row of file folder tabs
at the top. Each tab is associated with content in a div where display
is "hidden" except one. The external javascript changes the appearance
of the tabs, so that the appropriate one looks like it is in front,
and also changes the content of the box.

    function changeText(e) {
    e = e || window.event;
    var content = document.getElementById('bb');
    var a1 = document.getElementById('a1');
    var a2 = document.getElementById('a2');
    var a3 = document.getElementById('a3');
    var c1 = document.getElementById('c1');
    var c2 = document.getElementById('c2');
    var c3 = document.getElementById('c3');
    var content1 = document.getElementById('content1');
    var content2 = document.getElementById('content2');
    var content3 = document.getElementById('content3');
    if (!e.target) {
         e.target = e.srcElement || document;
    }

    if (e.target.id == 'a1') {
        c1.className = 'bbta bbt';
        c2.className = 'bbtina bbt';
        c3.className = 'bbtina bbt';
        content1.className = 'display_block';
        content2.className = 'display_none';
        content3.className = 'display_none';
        if (e.preventDefault) {
           e.preventDefault(); // other older browsers
        } else {
            e.returnValue = false; // IE8 and below
        }
    } else if (e.target.id == 'a2') {
        var head = document.getElementsByTagName("head")[0];  
        head.innerHTML += "<style type='text/css'>p.style11{color:
#020;  text-decoration: none; font-weight: 300;} p.style11 a:link,
p.style11 a:visited {color: #020;  text-decoration: underline;
font-weight: 300;}p.style11 a:hover, p.style11 a:active {color:
#0C436A;  text-decoration: none; font-weight: 300;}</style>";
        c1.className = 'bbtina bbt';
        c2.className = 'bbta bbt';
        c3.className = 'bbtina bbt';
        content1.className = 'display_none';
        content2.className = 'display_block';
        content3.className = 'display_none';
        if (e.preventDefault) {
           e.preventDefault(); // other older browsers
        } else {
            e.returnValue = false; // IE8 and below
        }
        return false;
    } else if (e.target.id == 'a3') {
        var head = document.getElementsByTagName("head")[0];
        head.innerHTML += "<style type='text/css'>p.style11{color:
#020;  text-decoration: none; font-weight: 300;} p.style11 a:link,
p.style11 a:visited {color: #020;  text-decoration: underline;
font-weight: 300;}p.style11 a:hover, p.style11 a:active {color:
#0C436A;  text-decoration: none; font-weight: 300;}</style>";
        c1.className = 'bbtina bbt';
        c2.className = 'bbtina bbt';
        c3.className = 'bbta bbt';
        content1.className = 'display_none';
        content2.className = 'display_none';
        content3.className = 'display_block';
        if (e.preventDefault) {
           e.preventDefault(); // other older browsers
        } else {
            e.returnValue = false; // IE8 and below
        }
    }
}

window.onload = function () {
    var bluebox = document.getElementById ('bb_holder');
    var tab = bluebox.querySelectorAll('div.bbt');
    for (var i=0; i < tab.length; i++) {
        tab[i].addEventListener('click', changeText, false);
    }
};

[toc] | [next] | [standalone]

#19432

Mason Barge wrote:

>  favorite
> 
> 
> Yes I'm a noob,

Never *ever* start a posting like that.

> but I'm at my wit's end as I've tried everything I can
> think of. This works fine in browsers other than IE (including IE9)
> and I'm not sure if the function fails or if it works but the "return
> false" mechanism fails. 

Find it out, then.  Until you are willing to do that, discussing your code 
is a waste of time even though it has obvious flaws.

<http://www.catb.org/~esr/faqs/smart-questions.html#idp5140624>

-- 
PointedEars

Twitter: @PointedEars2
Please do not Cc: me. / Bitte keine Kopien per E-Mail.

[toc] | [prev] | [next] | [standalone]

#19448

On Wed, 24 Apr 2013 11:02:41 +0200, Thomas 'PointedEars' Lahn
<PointedEars@web.de> wrote:

>Mason Barge wrote:
>
>>  favorite
>> 
>> 
>> Yes I'm a noob,
>
>Never *ever* start a posting like that.
>
>> but I'm at my wit's end as I've tried everything I can
>> think of. This works fine in browsers other than IE (including IE9)
>> and I'm not sure if the function fails or if it works but the "return
>> false" mechanism fails. 
>
>Find it out, then.  Until you are willing to do that, discussing your code 
>is a waste of time even though it has obvious flaws.

Okay, I found the problem and fixed it.  I would still be happy to
hear about the flaws.


function defaultFalse(e) {
    e = e || window.event;
    if (e.preventDefault) {
       e.preventDefault(); // other older browsers
    } else {
        e.returnValue = false; // IE8 and below
    }
}

function changeText(e) {
    e = e || window.event;
    var content = document.getElementById('bb');
    var a1 = document.getElementById('a1');
    var a2 = document.getElementById('a2');
    var a3 = document.getElementById('a3');
    var c1 = document.getElementById('c1');
    var c2 = document.getElementById('c2');
    var c3 = document.getElementById('c3');
    var content1 = document.getElementById('content1');
    var content2 = document.getElementById('content2');
    var content3 = document.getElementById('content3');
    if (!e.target) {
         e.target = e.srcElement || document;
    }
    
    if (e.target.id == 'a1') {
        c1.className = 'bbta bbt';
        c2.className = 'bbtina bbt';
        c3.className = 'bbtina bbt';
        content1.className = 'display_block';
        content2.className = 'display_none';
        content3.className = 'display_none';
    } else if (e.target.id == 'a2') {
        var newstyle = document.createElement('style');
        var newstyle_text =
document.createTextNode("type='text/css'>p.style11{color: #020;
text-decoration: none; font-weight: 300;} p.style11 a:link, p.style11
a:visited {color: #020;  text-decoration: underline; font-weight:
300;}p.style11 a:hover, p.style11 a:active {color: #0C436A;
text-decoration: none; font-weight: 300;}");
        newstyle.appendChild(newstyle_text);
        document.head.appendChild(newstyle);
        c1.className = 'bbtina bbt';
        c2.className = 'bbta bbt';
        c3.className = 'bbtina bbt';
        content1.className = 'display_none';
        content2.className = 'display_block';
        content3.className = 'display_none';
    } else if (e.target.id == 'a3') {
        var newstyle = document.createElement('style');
        var newstyle_text =
document.createTextNode("type='text/css'>p.style11{color: #020;
text-decoration: none; font-weight: 300;} p.style11 a:link, p.style11
a:visited {color: #020;  text-decoration: underline; font-weight:
300;}p.style11 a:hover, p.style11 a:active {color: #0C436A;
text-decoration: none; font-weight: 300;}");
        newstyle.appendChild(newstyle_text);
        document.head.appendChild(newstyle);
        c1.className = 'bbtina bbt';
        c2.className = 'bbtina bbt';
        c3.className = 'bbta bbt';
        content1.className = 'display_none';
        content2.className = 'display_none';
        content3.className = 'display_block';
    }
    defaultFalse(e);
}

window.onload = function () {
    var bluebox = document.getElementById ('bb_holder');
    var tab = bluebox.querySelectorAll('div.bbt');
    for (var i=0; i < tab.length; i++) {
        tab[i].addEventListener('click', changeText, false);
    }
};

[toc] | [prev] | [next] | [standalone]

#19453

Mason Barge wrote:

> Okay, I found the problem and fixed it.

Sort of.

> I would still be happy to hear about the flaws.

OK, fasten your seatbelt ;-)

> function defaultFalse(e) {
>     e = e || window.event;

Better:

  if (typeof e == "undefined")
  {
    e = window.event;
  }

  if (e)
  {

Because you cannot assume that if one (the W3C) DOM implementation is not 
available, the other (the MSHTML DOM) must be available.

>     if (e.preventDefault) {

That is insufficient and error-prone a feature test.  Insufficient because a 
true-value does not remotely tell you that it is something that you can 
call.  Error-prone because it is a method of a host object, and may not 
“want” to be subjected to a type-converting test (unrecoverable exception).  
The least you should do is

  if (typeof e.preventDefault == "function")

But the safest way is to use an isHostMethod() implementation which 
considers the known possibilities, like

  if (jsx.object.isHostMethod(e, "preventDefault"))

See 

<http://PointedEars.de/websvn/filedetails.php?repname=JSX&path=%2Ftrunk%2Fobject.js>

but also jsx.dom.createEventListener() in 

<http://Pointedears.de/websvn/filedetails.php?repname=JSX&path=%2Ftrunk%2Fdom%2Fevent.js> 

which makes it unnecessary to write that every time, and accomplishes it in 
a safe way:

  /* wraps event object differences between DOM implementations */
  var listener = jsx.dom.createEventListener(function (e) {
    …
    /* wraps the ways in which the default action can be prevented */
    return e.preventDefault();
    …
  });

  /* wraps the ways in which an event listener can be added */
  jsx.dom.addEventListener(target, eventType, listener);

>        e.preventDefault(); // other older browsers

That is a misconception.  preventDefault() is a *new* way to prevent the 
default action (as opposed to the proprietary way); it was first specified 
in W3C DOM Level 2 Events.  Unfortunately, until W3C DOM Event 
implementations are ubiquitous, it needs to be used carefully.

So far, the proprietary approach is supported best, and is best current (but 
unfortunately not common) practice.  jsx.dom.addEventListener() uses it as 
fallback for addEventListener(), even for *multiple* event listeners for the 
same event on the same target (which it by default cannot not do, by 
contrast to addEventListener()).  BTW, avoid Microsoft's attachEvent(), it 
is _not_ equivalent to addEventListener().  (See also the JSdoc in 
JSX:event.js.)

>     } else {
>         e.returnValue = false; // IE8 and below

This is *dead code* because you are using addEventListener().  That method 
is not implemented by MSHTML 8 and below, as used by IE 8 and below, and not 
available in IE 9 in Browser Compatibility Mode.

But likely none of this is necessary.

>     }

  }

> }
> 
> function changeText(e) {
>     e = e || window.event;

See above.

>     var content = document.getElementById('bb');
>     var a1 = document.getElementById('a1');
>     var a2 = document.getElementById('a2');
>     var a3 = document.getElementById('a3');
>     var c1 = document.getElementById('c1');
>     var c2 = document.getElementById('c2');
>     var c3 = document.getElementById('c3');
>     var content1 = document.getElementById('content1');
>     var content2 = document.getElementById('content2');
>     var content3 = document.getElementById('content3');

Instead of retrieving element object references again and again on click, 
retrieve them only once if your document structure does not change.  To 
avoid global variables, use closures:

  var changeText = (function () {
    var content;

    return function (e) {
      if (!content)
      {
        content = document.getElementById('bb');
      }

      … content …
    };
  }());

(document.getElementById() is not safe with all versions of IE/MSHTML, 
though.  If you really want to be backwards-compatible [before IE 5.5, 
presumably], you need a DOM wrapper for that.)

Better yet, get elements that belong together by class name or common 
ancestor, not separate IDs.  And then cache and iterate over the 
corresponding NodeLists.  See below.

>     if (!e.target) {
>          e.target = e.srcElement || document;
>     }

Same bad inference as above.  And never ever to attempt changing host 
objects like that referred to by “e”.

What sense does it make to use “document” as fallback anyway?  Just to have 
an object and not cause a TypeError?  The reasonable way is to return early 
when necessary conditions are not met, not to continue on something else as 
if nothing happened.

  /* or see jsx.dom.createEventListener() for the really safe way */
  var target = e.target || e.srcElement;

  if (!target)
  {
    /* because “false” may still prevent the default action here */
    return true;
  }

>     if (e.target.id == 'a1') {
>         c1.className = 'bbta bbt';
>         c2.className = 'bbtina bbt';
>         c3.className = 'bbtina bbt';
>         content1.className = 'display_block';
>         content2.className = 'display_none';
>         content3.className = 'display_none';
>     } else if (e.target.id == 'a2') {
>         var newstyle = document.createElement('style');
>         var newstyle_text =
> document.createTextNode("type='text/css'>p.style11{color: #020;
> text-decoration: none; font-weight: 300;} p.style11 a:link, p.style11
> a:visited {color: #020;  text-decoration: underline; font-weight:
> 300;}p.style11 a:hover, p.style11 a:active {color: #0C436A;
> text-decoration: none; font-weight: 300;}");
>         newstyle.appendChild(newstyle_text);
>         document.head.appendChild(newstyle);
>         c1.className = 'bbtina bbt';
>         c2.className = 'bbta bbt';
>         c3.className = 'bbtina bbt';
>         content1.className = 'display_none';
>         content2.className = 'display_block';
>         content3.className = 'display_none';
>     } else if (e.target.id == 'a3') {
>         var newstyle = document.createElement('style');
>         var newstyle_text =
> document.createTextNode("type='text/css'>p.style11{color: #020;
> text-decoration: none; font-weight: 300;} p.style11 a:link, p.style11
> a:visited {color: #020;  text-decoration: underline; font-weight:
> 300;}p.style11 a:hover, p.style11 a:active {color: #0C436A;
> text-decoration: none; font-weight: 300;}");
>         newstyle.appendChild(newstyle_text);
>         document.head.appendChild(newstyle);
>         c1.className = 'bbtina bbt';
>         c2.className = 'bbtina bbt';
>         c3.className = 'bbta bbt';
>         content1.className = 'display_none';
>         content2.className = 'display_none';
>         content3.className = 'display_block';
>     }
>     defaultFalse(e);
> }

I have full-quoted this code so that you can take a minute and read it from 
the perspective of a complete, but potentially knowledgable stranger.  I am 
sure you will agree then that it deserves to be called “an unmaintainable 
mess”.  Do you know in a month from now at a glance why it was written this 
way?  Don't Repeat Yourself (DRY), and use the right tool for the right job.

I have not dug through that mess (and I do not care to), but I assume you 
want to format elements based on selection, which is a common problem.
In that case, consider this approach instead:

  1. Define *one* class name to be indicative of selection.

  2. Remove that class name from all pertinent elements.
     (see also jsx.dom.css.removeClassName())

  3. Add that classname to the target element.
     (see also jsx.dom.css.addClassName())

  4. Let an existing ("static") stylesheet do the formatting based on the
     assigned class names of the elements.  (Or, if absolutely necessary,
     add the stylesheet dynamically *once*.)
 
But if reasonably possible, *avoid* a scripted or script-only solution at 
all, so that your Web site degrades gracefully.  For example, CSS3 offers 
the :target pseudo-class (I have used it for the TOC of the ECMAScript 
Support Matrix; no script in *that* corner of the Matrix :)).

> window.onload = function () {

Try to avoid this anti-pattern; if possible, at least use a DOM wrapper so 
that you do not have to rely on that proprietary property.

The “body” element has a standards-compliant, all-time backwards-compatible, 
interoperable “onload” event-handler attribute that you should use instead.

>     var bluebox = document.getElementById ('bb_holder');
                                           ^
Keep your code style easily readable and consistent.  This allows you to 
maintain it more easily.  For example, if you follow a consistent code 
style, you can do elaborate refactoring with step-by-step search and replace 
instead of having to type the same things over and over again.  (Refactoring 
ECMAScript code based on inference is particularly hard for development 
tools because the languages are so dynamic.  But you know what you code 
means.)

>     var tab = bluebox.querySelectorAll('div.bbt');

That is an anti-pattern as well.  Not all CSS selectors are supported in all 
DOM implementations.  In particular, MSHTML 8 will not support CSS3 
selectors, and MSHTML before 8 will very likely not support all CSS 2.1 
selectors (not the case here, just to let you know). 

And IE/MSHTML before version 8, and version 9 in version 7 Compatibility 
Mode, do not implement this method, and will throw a TypeError exception. 

Try to find another way.  Consider, for example, a DOM wrapper for 
document.getElementsByClassName() like jsx.dom.getElemByClassName().

>     for (var i=0; i < tab.length; i++) {

  for (var i = 0, len = tab.length; i < len; ++i)

avoids repeated property accesses (to tab.length, which gives a considerable 
performance boost) and should be (marginally) faster with incrementing (as 
the old value can be discarded; but implementations vary almost as much as 
with i++).

>         tab[i].addEventListener('click', changeText, false);

See above about (in)compatibility.

>     }

The “click” event bubbles.  Instead of adding the same listener to a number 
of elements (from which you need to remove it before unloading, to avoid 
memory leaks), add it (by markup – that is, the standards-compliant, all-
time backwards-compatible, interoperable “onclick” event-handler attribute 
–, if possible) to *one* common ancestor of those elements and determine the 
event target in that listener.

<http://www.w3.org/TR/DOM-Level-3-Events/#dom-event-architecture>

If you are using event-handler attributes or event-handler properties, you 
can simply prevent the default action for the “click” event then by 
returning “false” to the event handler –

  o.onclick = function (e) {
    …
    return false;
  };

or

  <… onclick="if (typeof event != 'undefined') return parent_click(event)">

– where parent_click(…) would contain the handling code (to avoid spaghetti 
code) that would need to return “false” itself in that case.  [Note that the 
“event” value is in event-handler attribute values apparently is 
proprietary, but (backwards-)compatible.  The gauntlet is only used to 
prevent a ReferenceError exception being thrown in the very unlikely case 
that it is unsupported.  Omit it at your own risk.]


Probably I have forgotten to mention something else.


HTH

-- 
PointedEars

Twitter: @PointedEars2
Please do not Cc: me. / Bitte keine Kopien per E-Mail.

[toc] | [prev] | [next] | [standalone]

#19467

Thomas 'PointedEars' Lahn wrote:
...
> (document.getElementById() is not safe with all versions of IE/MSHTML, 
> though.  If you really want to be backwards-compatible [before IE 5.5, 
> presumably], you need a DOM wrapper for that.)

As IE 5.50 is compatible even with
Windows 95, I puzzle why anybody would want
such backwards compatibility.

-- 
Regards,
Martin Leese
E-mail: please@see.Web.for.e-mail.INVALID
Web: http://members.tripod.com/martin_leese/

[toc] | [prev] | [next] | [standalone]

#19469

On Thu, 25 Apr 2013 01:31:19 +0200, Thomas 'PointedEars' Lahn
<PointedEars@web.de> wrote:

>Mason Barge wrote:
>
>> Okay, I found the problem and fixed it.
>
>Sort of.
>
>> I would still be happy to hear about the flaws.
>
>OK, fasten your seatbelt ;-)

Thank you for taking all the time to write that out.

[toc] | [prev] | [next] | [standalone]

#19483

Mason Barge wrote:

> Thomas 'PointedEars' Lahn wrote:
>> Mason Barge wrote:
>>> I would still be happy to hear about the flaws.
>> OK, fasten your seatbelt ;-)
> 
> Thank you for taking all the time to write that out.

You are welcome.  Just ask if anything is unclear.

-- 
PointedEars

Twitter: @PointedEars2
Please do not Cc: me. / Bitte keine Kopien per E-Mail.

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.javascript

csiph-web