IE leaks, circular references, addevent et al

D

David Mark

David said:
David Mark wrote:
[...]
Handler can also be a host object, so the whole thing might fail, but
that's a rather moot point, of course, and can be mentioned in
documentation too.
That goes without saying I think (I mean using a host object as a
listener).  Not supported in any event.
Alright.
There's another thing I meant to ask you. If I'm not mistaken, event
mechanism in "My Library" augments target with either "uniqueID" or
"_targetId" strings.
What do you know?  There are some expsndos in there.
Yes and this is not a good practice.

I know. However, avoiding this practice seems to make things much harder
(and some things - even impossible).

I recently stumbled upon a post by Zakas, where he gives a solution to
Who?

detect iframe load completion in a rather unfortunate sniff-based manner:

var iframe = document.createElement("iframe");
iframe.src = "simpleinner.htm";

if (navigator.userAgent.indexOf("MSIE") > -1 && !window.opera){
     iframe.onreadystatechange = function(){
         if (iframe.readyState == "complete"){
             alert("Local iframe is now loaded.");
         }
     };} else {

     iframe.onload = function(){
         alert("Local iframe is now loaded.");
     };

}

document.body.appendChild(iframe);

Oh boy.
To avoid sniffing here, I see 2 possible (and, undoubtedly, much more
robust) solutions.

1) Use inference based on property (corresponding to intrinsic event
handler) existence, as in - `typeof iframe.onreadystatechange !=
'undefined'` or `'onreadystatechange' in iframe`, etc. The downside here
is that it is still an inference and can result in false positives.
Yes.


2) Combine 2 branches into one and manage them manually (also recently
mentioned here by Cornford):

iframe.onload = function(){
   iframe.onreadystatechange = null;
   alert('loaded');};

iframe.onreadystatechange = function(){
   iframe.onload = null;
   if (iframe.readyState == 'complete') {
     alert('loaded');
   }

};

Yes, I did a lot of that in My Library. Mousewheel, context clicks,
etc.
This is seemingly a more robust way. The downside here (and is my actual
point) is that if `iframe` element doesn't support "readystatechange"
event in some client, assignment to that "onreadystatechange" property
is essentially a host object augmentation. It's no different than
`iframe.foo = function(){ ... }` or `iframe._uniqueId = '_id12345'`.

Isn't it?

That's correct, but are you planning to use DOM0?
[...]
Because I was avoiding expandos in IE by using their built-in unique
identifier, which doesn't work at all for document objects.  All could
have been avoided, of course.

Understood.

One thing you could do is use the DOM0 properties to stash the
normalized listeners (or a handle that points to them). Then you are
just augmenting a Function object.
At the same time, removing duplicates handling mechanism, obviously,
makes things more fragile. I understand that there's a *fine line*
between what needs to be in abstraction and what should be considered an
overkill; I'm only looking at it from the API perspective: duplicate
submission can happen inadvertently without user knowing about it. If
such thing is silently ignored, it will surely be a cause of hard to
find bugs and all kinds of oddities.

For one, you document that duplicates are not allowed. If you are
really paranoid about it, during development, you could throw an error
in that case. Any oddities are on the calling code.
Another thing to consider (and is something that can cause even more
confusion and bugs) is inconsistency of course. `addEventListener` (in
DOM L2, at least) guarantees that duplicates are ignored.

That's what I'm getting at. Calling code shouldn't bother to do it.
I don't think
having `addEvent` which behaves differently in W3C browsers and IE is a
good idea.

Why not? They goal is not to emulate addEventListener exactly, but to
provide an API for attaching and detaching listeners. None of this
really matters though as you can keep the duplicate detection in
without using expandos.
If we were to choose a particular set of traits for event handling
mechanism, than we might as well drop not only duplicate handling, but
`this` normalization too (which would prevent leaks handling in the
first place) :)

The leak problem has been solved a ways back and did not involve
expandos. Setting - this - is definitely a useful feature, but not
always needed. It always comes back to the context.
In any case, it looks like most abstractions try to follow
`addEventListener` semantics, and that's completely understandable, of
course.
Sure.


Determine that object is a `window`, then store it as a reference in
array of such `window` objects? Later, that array can be iterated over
in order to determine `id` of a given `window` object.

Not sure if it's a good idea and how practical it is, though.

I wouldn't bother to write such a wrapper for window objects. ;)
But tested on a variety of clients nevertheless, right?

Yes. By myself two years ago and many others since. Doesn't prove
much though. Expandos are still ill-advised.
Ah yes, I see early returns in few places as soon as `document.expando`
is false.

A curiosity. Should also be part of the documentation (i.e. don't set
that property to false).
 
R

Richard Maher

Hi All,

kangax said:
Richard Maher wrote:


So you decided to augment host objects after all.
Was there an option B? I couldn't find that hashCode() anywhere; what do you
use?

Anyway, thanks everyone for the very useful advice, and I think I've taken
the substantive points on board below.

Cheers Richard Maher

Take III :-

var listenerRegistry = function() {

var globalObj = this,
actionListeners = {},
targetId = 1,
listenerId = 1;

var checkIn = function(){

if (window.addEventListener)
return function(element, eventName, handler) {
element.addEventListener(eventName, handler, false);
};

if (window.attachEvent)
return function(element, eventName, handler) {
if (!handler._fnId)
handler._fnId = listenerId++;

if (!element._evtId)
element._evtId = targetId++;
else
if (actionListeners[element._evtId + eventName +
handler._fnId])
return;

var normalizedHandler =
function() {
handler.call(
actionListeners[element._evtId +
eventName + handler._fnId].el,
globalObj.window.event);
};

actionListeners[element._evtId + eventName + handler._fnId]
=
{el : element, fn : normalizedHandler};

if (!element.attachEvent('on' + eventName,
normalizedHandler))
throw new Error("Unable to attach listener");
};

throw new Error("Unsupported Browser");
}();

var checkOut = function(){

if (window.removeEventListener)
return function(element, eventName, handler) {
element.removeEventListener(eventName, handler, false);
};

if (window.detachEvent)
return function(element, eventName, handler) {
if (!element._evtId || !handler._fnId)
throw new Error("No such event registered on this
Object");

if (!actionListeners[element._evtId + eventName +
handler._fnId])
throw new Error("Unable to detach listener");

element.detachEvent('on' + eventName,
actionListeners[element._evtId +
eventName + handler._fnId].fn);
actionListeners[element._evtId + eventName +
handler._fnId].el = null;
actionListeners[element._evtId + eventName +
handler._fnId].fn = null;
actionListeners[element._evtId + eventName + handler._fnId]
= null;
};

throw new Error("Unsupported Browser");
}();

return {
checkIn : checkIn,
checkOut : checkOut
};
}();
 
D

David Mark

Hi All,




Was there an option B? I couldn't find that hashCode() anywhere; what do you
use?

It's pretty simple. You need to store normalized listeners per
element, event type and function. If yours is a wrapper for
attachEvent/addEventListener, you've got the corresponding DOM0
properties open to store objects (or handles). Then you are
augmenting a (dummy in most cases) Function object rather than the
host object.

You could also return the normalized listener on attach (to make
removing them simpler). But, the prevailing sentiment has been that
such design compromises aren't "cool". If anything defines the last
few years, it is "coolness" over competence. Always seems to be about
"what if we want to do this or that in the future", when the present
is falling apart.

But I'd think about what you really need from your design before
getting into all of these issues. For instance, how often do you need
to remove listeners? I've found it is far less often than adding
them. In other words, just because you have a fancy add, doesn't mean
you need a fancy remove.

[snip]
 
D

David Mark

http://www.nczonline.net/

"Professional JavaScript" author

I see.
[...]
That's correct, but are you planning to use DOM0?

Well, obviously this is only an issue with using DOM0 (whether
explicitly or if `addEvent` abstraction falls back to it after not
finding anything better in some crippled clients).

My point is that "augmenting host object" has somewhat blurry
definition. We all understand that assigning "_foobar" property to an
element is not a good idea; yet we assign event handler properties not
knowing whether client does in fact support corresponding events.

Do we?

http://thinkweb2.com/projects/prototype/detecting-event-support-without-browser-sniffing/


For
all we know, such property (e.g. `oncontextmenu`) can be no better that
`_foobar` one and can lead to all the same consequences.

I wouldn't blindly assign to that particular property.
[...]
but I
don't understand:
1) Why any target that has falsy `tagName` gets special treatment and is
injected with unique `"_api" + uniqueTargetHandle`
Because I was avoiding expandos in IE by using their built-in unique
identifier, which doesn't work at all for document objects.  All could
have been avoided, of course.
Understood.
One thing you could do is use the DOM0 properties to stash the
normalized listeners (or a handle that points to them).  Then you are
just augmenting a Function object.

Do you mean something along:

function f(){};
f._handlerId = _handlerId++;
someElement.onclick = f;

Something like that.
[...]
Why not?  They goal is not to emulate addEventListener exactly, but to
provide an API for attaching and detaching listeners.  None of this
really matters though as you can keep the duplicate detection in
without using expandos.

Yep. There's a fine range of ways to implement `addEvent` abstraction.
Some features simply lead to more fragile implementation.


The leak problem has been solved a ways back and did not involve
expandos.  Setting - this - is definitely a useful feature, but not
always needed.  It always comes back to the context.

Yes, it always does.

[...]
I wouldn't bother to write such a wrapper for window objects.  ;)

Sure. Supporting multiple contexts is like supporting quirksmode or xhtml..

Certainly for attaching listeners to window objects.
If you're interested, I noticed that some things might fail in older
Safari (e.g. 2.0) due to missing `compatMode` there, which seems to be
used in `getContainerElement`. You need to feature test quirks mode when
`compatMode` is missing (so that code doesn't erroneously fall into
`body-acting-as-root` mode).

This?

// Result is ambiguous in all but modern browsers, so don't assume too
much from it.
getContainerElement = function(docNode) {
docNode = docNode || global.document;
return (docNode.documentElement && (!docNode.compatMode ||
docNode.compatMode.indexOf('CSS') != -1))?
docNode.documentElement:getBodyElement(docNode);
};

See the comment. :)

If you look at the library code, it doesn't assume much from this
(e.g. the result doesn't necessarily house the scroll* properties).
It's a function that could (and probably should) be factored out as
its presence confuses more than it clears up.
IIRC, one of the snippets in Cornford's "not browser detect" article
uses `compatMode` without test as well.

Probably for measuring the viewport. Richard published a really nice
(and involved) version of that at one point. I'm sure it didn't
assume much from compatMode (or the lack thereof).

One thing you can look at for the viewport measurement is if
documentElement.clientWidth === 0. That's a browser that doesn't
render the documentElement at all (e.g. IE < 6). I still wouldn't
assume the scroll* properties are on that object though (they are in
IE, of course).
 
D

David Mark

One thing you can look at for the viewport measurement is if
documentElement.clientWidth === 0.  That's a browser that doesn't
render the documentElement at all (e.g. IE < 6).  I still wouldn't
assume the scroll* properties are on that object though (they are in
IE, of course).

That object meaning the body.
 
D

David Mark

"Professional JavaScript" author

I see.




[...]
This is seemingly a more robust way. The downside here (and is my actual
point) is that if `iframe` element doesn't support "readystatechange"
event in some client, assignment to that "onreadystatechange" property
is essentially a host object augmentation. It's no different than
`iframe.foo = function(){ ... }` or `iframe._uniqueId = '_id12345'`.
Isn't it?
That's correct, but are you planning to use DOM0?
Well, obviously this is only an issue with using DOM0 (whether
explicitly or if `addEvent` abstraction falls back to it after not
finding anything better in some crippled clients).
My point is that "augmenting host object" has somewhat blurry
definition. We all understand that assigning "_foobar" property to an
element is not a good idea; yet we assign event handler properties not
knowing whether client does in fact support corresponding events.

Do we?

http://thinkweb2.com/projects/prototype/detecting-event-support-witho...
For
all we know, such property (e.g. `oncontextmenu`) can be no better that
`_foobar` one and can lead to all the same consequences.

I wouldn't blindly assign to that particular property.




[...]
but I
don't understand:
1) Why any target that has falsy `tagName` gets special treatment and is
injected with unique `"_api" + uniqueTargetHandle`
Because I was avoiding expandos in IE by using their built-in unique
identifier, which doesn't work at all for document objects.  All could
have been avoided, of course.
Understood.
One thing you could do is use the DOM0 properties to stash the
normalized listeners (or a handle that points to them).  Then you are
just augmenting a Function object.
Do you mean something along:
function f(){};
f._handlerId = _handlerId++;
someElement.onclick = f;

Something like that.




[...]
I don't think
having `addEvent` which behaves differently in W3C browsers and IE is a
good idea.
Why not?  They goal is not to emulate addEventListener exactly, butto
provide an API for attaching and detaching listeners.  None of this
really matters though as you can keep the duplicate detection in
without using expandos.
Yep. There's a fine range of ways to implement `addEvent` abstraction.
Some features simply lead to more fragile implementation.
Yes, it always does.
and clearly it is because the design was trying to do too much.
Surely there is a happy medium between that and requiring an ID for
every node (and how would that work for documents and windows anyway?)
Determine that object is a `window`, then store it as a reference in
array of such `window` objects? Later, that array can be iterated over
in order to determine `id` of a given `window` object.
Not sure if it's a good idea and how practical it is, though.
I wouldn't bother to write such a wrapper for window objects.  ;)
Sure. Supporting multiple contexts is like supporting quirksmode or xhtml.

Certainly for attaching listeners to window objects.




If you're interested, I noticed that some things might fail in older
Safari (e.g. 2.0) due to missing `compatMode` there, which seems to be
used in `getContainerElement`. You need to feature test quirks mode when
`compatMode` is missing (so that code doesn't erroneously fall into
`body-acting-as-root` mode).

This?

// Result is ambiguous in all but modern browsers, so don't assume too
much from it.
      getContainerElement = function(docNode) {
        docNode = docNode || global.document;
        return (docNode.documentElement && (!docNode.compatMode ||
docNode.compatMode.indexOf('CSS') != -1))?
docNode.documentElement:getBodyElement(docNode);
      };

See the comment.  :)

And looking carefully, this will work with Safari 2, as it assumes
documentElement if there is no document.compatMode. For what that's
worth (see below).
If you look at the library code, it doesn't assume much from this
(e.g. the result doesn't necessarily house the scroll* properties).
It's a function that could (and probably should) be factored out as
its presence confuses more than it clears up.




Probably for measuring the viewport.  Richard published a really nice
(and involved) version of that at one point.  I'm sure it didn't
assume much from compatMode (or the lack thereof).

One thing you can look at for the viewport measurement is if
documentElement.clientWidth === 0.  That's a browser that doesn't
render the documentElement at all (e.g. IE < 6).  I still wouldn't
assume the scroll* properties are on that object though (they are in
IE, of course).

Curious enough to look at the one in My Library. Turns out the
viewport measurement stuff uses a wrapper for getContainerElement.
Goes to show how limited getContainerElement is by itself.

getScrollElement = function(docNode) {
docNode = docNode || global.document;
var body = getBodyElement(docNode);
var se = getContainerElement(docNode);

if (body && se != body && se.clientWidth === 0 && typeof
body.scrollWidth == 'number') { // IE5.x
se = body;
}
return se;
};

And that scrollWidth test is out of place. It's not really what we
are interested here. Name is unfortunate as well. What we want is
the outermost rendered element. It is determined later whether that
element houses - for example - scroll* or client* properties.
 
D

David Mark

What happens if Safari is in quirks mode? From what I can see it will
then assume `document.documentElement` is a root (as long as
`documentElement` results in something truthy).

Safari 2.x in quirks mode? No idea. Does it render the
documentElement? If it does, you would think that is the one to
measure. If it has a clientWidth property that is a number over 0,
that would be the assumption.

I think it is best to avoid quirks mode if you sincerely wish to
target Safari 2.

And it should go without saying that measuring the viewport is a shaky
thing hinge a design on (in any mode). We aren't even getting in to
the Opera versions that return the wrong clientHeight. Why bother?
Failure is disaster. Even success at this is often a failure (e.g.
see Reddit sign-up). Just put popups at the top. Horizontal
centering is far less troublesome.
Ah, yes it does.





So this logic should really be in `getContainerElement` (which would
probably be better named as `getRootElement`) too, right?

The thing is that getContainerElement is not really needed. If you
look at each place it is used (e.g. above), you will see it doesn't
accomplish anything on its own. Each call could be "inlined" or
removed. But the above function is for a specific purpose (viewport
measurement). Despite what is an unfortunate tacked on name, it
doesn't mean to imply that the returned object has the appropriate
scroll* properties. If you look at that stuff, it doesn't assume
either until it sees what each does on scrolling, so that call to
getContainerElement could be removed.

I would be interested to know the results of Safari 2.x quirks mode
though. I'm done with My Library (obviously), but if there is a hole
in my logic, I would like to know about it. I know Dojo has two
places it measures the viewport and I'm getting ready to import mine
(or likely a pared down version) as a replacement for both.
 
R

Richard Maher

Hi David,

Thanks again for your help.

Not sure the full ramifications or implementation of your first paragraph,
although it sounds interesting. I see the solution in paragraph 2 as also
being worth a guernsey. But so far my butchery of your other solution seems
to be simple enough to "Does what it says on the tin." so I'm content.

Cheers Richard Maher

PS. And yes I top posted 'cos this silly thing wasn't
auto-quoting/indenting, and honestly most fibre-eaters don't care :)

Hi All,




Was there an option B? I couldn't find that hashCode() anywhere; what do you
use?

It's pretty simple. You need to store normalized listeners per
element, event type and function. If yours is a wrapper for
attachEvent/addEventListener, you've got the corresponding DOM0
properties open to store objects (or handles). Then you are
augmenting a (dummy in most cases) Function object rather than the
host object.

You could also return the normalized listener on attach (to make
removing them simpler). But, the prevailing sentiment has been that
such design compromises aren't "cool". If anything defines the last
few years, it is "coolness" over competence. Always seems to be about
"what if we want to do this or that in the future", when the present
is falling apart.

But I'd think about what you really need from your design before
getting into all of these issues. For instance, how often do you need
to remove listeners? I've found it is far less often than adding
them. In other words, just because you have a fancy add, doesn't mean
you need a fancy remove.

[snip]
 
D

David Mark

David said:
David Mark wrote:
David Mark wrote:
David Mark wrote:
David Mark wrote:
[...]
3) Whether you had any problems with all this host objects augmentation
in any environment.
Not that I kmow of.  Of course, I never used my library.  ;) I do
But tested on a variety of clients nevertheless, right?
Yes.  By myself two years ago and many others since.  Doesn't prove
much though.  Expandos are still ill-advised.
If you're interested, I noticed that some things might fail in older
Safari (e.g. 2.0) due to missing `compatMode` there, which seems tobe
used in `getContainerElement`. You need to feature test quirks modewhen
`compatMode` is missing (so that code doesn't erroneously fall into
`body-acting-as-root` mode).
This?
// Result is ambiguous in all but modern browsers, so don't assume too
much from it.
      getContainerElement = function(docNode) {
        docNode = docNode || global.document;
        return (docNode.documentElement && (!docNode.compatMode ||
docNode.compatMode.indexOf('CSS') != -1))?
docNode.documentElement:getBodyElement(docNode);
      };
See the comment.  :)
And looking carefully, this will work with Safari 2, as it assumes
documentElement if there is no document.compatMode.  For what that's
worth (see below).
What happens if Safari is in quirks mode? From what I can see it will
then assume `document.documentElement` is a root (as long as
`documentElement` results in something truthy).
Safari 2.x in quirks mode?  No idea.  Does it render the
documentElement?  If it does, you would think that is the one to
measure.  If it has a clientWidth property that is a number over 0,
that would be the assumption.

It looks like even in quirks mode, Safari has `document.documentElement`
as a root element, so it's all good.

Good deal.
Ah, that's an elegant solution.

Thanks. Mac-style isn't it? Windows centers vertically.
[...]




The thing is that getContainerElement is not really needed.  If you
look at each place it is used (e.g. above), you will see it doesn't
accomplish anything on its own.  Each call could be "inlined" or
removed.  But the above function is for a specific purpose (viewport
measurement).  Despite what is an unfortunate tacked on name, it
doesn't mean to imply that the returned object has the appropriate
scroll* properties.  If you look at that stuff, it doesn't assume
either until it sees what each does on scrolling, so that call to
getContainerElement could be removed.
Understood.



I would be interested to know the results of Safari 2.x quirks mode
though.  I'm done with My Library (obviously), but if there is a hole
in my logic, I would like to know about it.  I know Dojo has two
places it measures the viewport and I'm getting ready to import mine
(or likely a pared down version) as a replacement for both.

`document.documentElement.clientWidth` is not 0 in Safari 2 quirks. It
has some value that looks like screen width, but I don't know how
precise it is.

The viewport width I hope. And hopefully precise as well. :)
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,770
Messages
2,569,584
Members
45,075
Latest member
MakersCBDBloodSupport

Latest Threads

Top