Path: csiph.com!usenet.pasdenom.info!gegeweb.org!newsfeed.kamp.net!newsfeed.kamp.net!87.79.20.101.MISMATCH!newsreader4.netcologne.de!news.netcologne.de!newsfeed.arcor.de!newsspool3.arcor-online.net!news.arcor.de.POSTED!not-for-mail Content-Type: text/plain; charset="UTF-8" Message-ID: <7022211.qX0gn5gzEG@PointedEars.de> From: Thomas 'PointedEars' Lahn Reply-To: Thomas 'PointedEars' Lahn Organization: PointedEars Software (PES) Date: Thu, 25 Apr 2013 01:31:19 +0200 User-Agent: KNode/4.8.5 Content-Transfer-Encoding: 8Bit X-Face: %i>XG-yXR'\"2P/C_aO%~;2o~?g0pPKmbOw^=NT`tprDEf++D.m7"}HW6.#=U:?2GGctkL,f89@H46O$ASoW&?s}.k+&. <1869831.hdSLL0z7eq@PointedEars.de> <6ekgn81cbkef0tc9sdhq0kpmr9a55ka6i7@4ax.com> MIME-Version: 1.0 Lines: 311 NNTP-Posting-Date: 25 Apr 2013 01:31:24 CEST NNTP-Posting-Host: 34f9b61a.newsspool1.arcor-online.net X-Trace: DXC=Th8\c65On8585[]]\]T081ic==]BZ:af>4Fo<]lROoR1nkgeX?EC@@0:HSIQ^WeST2DZm8W4\YJN X-Complaints-To: usenet-abuse@arcor.de Xref: csiph.com comp.lang.javascript: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 but also jsx.dom.createEventListener() in 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. 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.