OpenLayers OpenLayers

Ticket #880 (closed bug: fixed)

Opened 1 year ago

Last modified 1 year ago

Event object conflict

Reported by: euzuro Assigned to: crschmidt
Priority: major Milestone: 2.5 Release
Component: Events Version: 2.4
Keywords: Cc:
State:

Description

As reported by Paul S to dev@ 

 As part of removing prototype.js
dependency, it appears that you have kept most of Prototype's event
handling code, including the following (lines 327-331 of Events.js):

if (window.Event) {
  OpenLayers.Util.extend(window.Event, OpenLayers.Event);
} else {
  var Event = OpenLayers.Event;
}

If you include OpenLayers.js AFTER including prototype.js, this
effectively replaces most of prototype's Event object with the
OpenLayers.Event object, which has undesirable results (i.e. calling
Event.observe is actually calling OpenLayers.Event.observe)

Is there a particular reason why we are putting OpenLayers.Event into
the global namespace?

Attachments

event.patch (0.6 kB) - added by crschmidt on 08/04/07 08:45:48.
event_stop.patch (438 bytes) - added by crschmidt on 09/12/07 16:16:47.
events.patch (458 bytes) - added by crschmidt on 09/13/07 18:58:27.
events.2.patch (1.2 kB) - added by euzuro on 09/13/07 19:10:00.
take 5! here it is, everyone safe and sound
events.3.patch (1.2 kB) - added by euzuro on 09/13/07 19:14:27.
TAKE 6!! SAFER AND SOUNDER!!!

Change History

08/03/07 11:03:01 changed by crschmidt

  • owner changed from euzuro to crschmidt.

Taking this one, because it shouldn't be done anymore. I'll get a patch up for this soon.

08/03/07 11:04:17 changed by crschmidt

  • status changed from new to assigned.

08/04/07 08:45:28 changed by crschmidt

The reason for originally having Event in the main namespace is that we had encouraged people (through 2.0, I think) to use Event.stop in their client code. We now only use OpenLayers.Event internally, but we wanted to ensure that users who had used 'Event.stop' would not have their applications break.

Looking at the prototype code, it seems like Prototype will extend any Event object it finds. This means that we should, I think, be able to change:

if (window.Event) {

OpenLayers.Util.extend(window.Event, OpenLayers.Event);

} else {

var Event = OpenLayers.Event;

}

to just:

if (!window.Event) {

var Event = OpenLayers.Event;

}

That way, users who are not using Prototype and are using Event.stop will get their stop still.

Then, in 3.0, we remove it.

08/04/07 08:45:48 changed by crschmidt

  • attachment event.patch added.

08/04/07 08:46:14 changed by crschmidt

  • keywords set to review.

Comments welcome.

08/24/07 23:44:09 changed by tschaub

  • keywords changed from review to commit.

If we can't get rid of Event entirely, I think this makes sense.

Please commit.

08/25/07 03:39:29 changed by crschmidt

  • keywords deleted.
  • status changed from assigned to closed.
  • resolution set to fixed.

(In [4040]) "Event object conflict: If you include OpenLayers.js AFTER including prototype.js, this effectively replaces most of prototype's Event object with the OpenLayers.Event object, which has undesirable results (i.e. calling Event.observe is actually calling OpenLayers.Event.observe)." Fixed by only creating Event is Event does not already exist. (Closes #880)

09/12/07 15:02:11 changed by euzuro

(In [4246]) use the namespace corrected event.stop(). (See #880)

09/12/07 16:11:38 changed by euzuro

  • status changed from closed to reopened.
  • resolution deleted.

as cr5 and i just discovered. um... this isnt working correctly

09/12/07 16:16:12 changed by crschmidt

The problem is that the window.Event variable exists. We only care about keeping one function from Event, so instead we'll just check for that -- it's the only one we ever used in our examples/API as a suggestion for users back in the prototype days. Patch incoming.

09/12/07 16:16:47 changed by crschmidt

  • attachment event_stop.patch added.

09/12/07 16:17:43 changed by crschmidt

  • keywords set to review.

Hard to test, since we'd have to muck about before the OpenLayers stuff is loaded. Can put one together if it's important.

09/13/07 18:58:27 changed by crschmidt

  • attachment events.patch added.

09/13/07 18:59:11 changed by crschmidt

so erik pointed out that only applying stop, was totally arbitrary

*and*

we have applyDefaults

which does what we actually want.

So, new patch does that

09/13/07 19:10:00 changed by euzuro

  • attachment events.2.patch added.

take 5! here it is, everyone safe and sound

09/13/07 19:14:27 changed by euzuro

  • attachment events.3.patch added.

TAKE 6!! SAFER AND SOUNDER!!!

09/13/07 19:16:30 changed by crschmidt

  • keywords changed from review to commit.

heh, heh

Erik and I investigated a little more and found out htat IE has no window.Event like FF. so this patch is what we should have done a while back, but I forgot about applyDefaults.

Go! Commit! Fix!

09/13/07 19:17:46 changed by euzuro

  • keywords deleted.
  • status changed from reopened to closed.
  • resolution set to fixed.

(In [4271]) Here we have finally solved the smashing of the Event object problem. Once and forall. God save the queen when 3.0 comes we're getting rid of this sloppiness. See r4040 for more info on why we've done all this. (Closes #880)