Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove defaultPrevented attribut from ol.MapEvent #788

Merged
merged 1 commit into from Jun 17, 2013

Conversation

fredj
Copy link
Member

@fredj fredj commented Jun 12, 2013

@elemoine
Copy link
Member

Looks good to me.

@twpayne
Copy link
Contributor

twpayne commented Jun 17, 2013

I don't think this is quite right. More detailed comment to follow.

@elemoine
Copy link
Member

@twpayne
Copy link
Contributor

twpayne commented Jun 17, 2013

I don't think this is quite right. More detailled comment to follow.

So, here's why.

There are two separate chains of event handling, which is why a ol.MapEvent currently contains a browser event. The two chains are:

  1. The normal browser event handling, which use stopPropagation and preventDefault to stop propagation and suppress the normal browser event handling
  2. The map browser event handling, which uses preventDefault to prevent the propagation of a map events further down the interaction stack

For example, you might have multiple interactions that listen for mousedown events, say feature editing and drag panning. In this case, the feature event is on the top of the interaction stack. If the mousedown event hits a feature, the feature editing interaction calls preventDefault to prevent the event being propagated to the drag panning. Without this, editing a feature would also cause the map to pan. Note that these map events are completely separate from the browser event handling.

So, I think this PR as it is will break things. You need to keep two different preventDefaults, one for map events and one for browser events. They can either be in separate objects (as they are at the moment) or be in the same object but have different names (e.g. preventMapDefault / mapDefaultPrevented).

@twpayne
Copy link
Contributor

twpayne commented Jun 17, 2013

@elemoine
Copy link
Member

Tom, I read your comments multiple times but I still fails to understand where the problem is. @fredj's patch just removes a function and a property that are already in the parent class (goog.events.Event). What harm can this cause? Sorry.

@elemoine
Copy link
Member

Are you sure you're not mixing up with #791?

@twpayne
Copy link
Contributor

twpayne commented Jun 17, 2013

Are you sure you're not mixing up with #791?

Indeed, I am.

So, if I've got it right, this PR is OK, but #791 isn't.

@elemoine
Copy link
Member

100% agreement.

@elemoine
Copy link
Member

@fredj, please merge this if you agree.

fredj added a commit that referenced this pull request Jun 17, 2013
Remove defaultPrevented attribut from ol.MapEvent
@fredj fredj merged commit 153fea5 into openlayers:master Jun 17, 2013
@fredj fredj deleted the mapevent branch June 17, 2013 16:24
@fredj fredj restored the mapevent branch June 17, 2013 16:24
afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
Rework of Google layer leads to map div transparency. r=@bartvde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants