The Feature Handler doesn't allow registering a "click" event callback

[elemoine]

Is there a good reason not to handle click events in Handler/Feature.js?

In some context it can be useful to register a click callback as opposed to a mousedown event callback. For example, you have a "click on map" callback registered and you don't this callback to trigger when you click on a drawn feature. Using toaday's SelectFeature control (and Feature Handler), you won't be able to click on your features without triggering the "click on map" callback. To be able to do that, you want to register a "click on feature" callback as opposed to a "mousedown on feature" callback. More precisely you want to do that:

var ctl = new OpenLayers.Control.SelectFeature(vectorLayer, {

callbacks: {

down: null, click: myFeatureClickCallback

}

});

which doesn't work today, because there's no "click" callback in Handler/Feature.js.

Attached patch addresses this.

[tschaub]

Hmm. This is somewhat messing with the idea of handlers.

Users shouldn't have to know about handler callbacks. Users should know about control options. The SelectFeature control currently has one option to control it's behavior - hover. If hover is true, features are selected on hover. If hover is false, features are selected on mousedown.

I think what we really want here is for the feature handler to stop mouseup, mousedown, and click if they correspond with a feature. Your patch does this.

I think we also want the SelectFeature control to select features on click (instead of mousedown) if hover is false. It was an oversight to select on mousedown (as a user may mousedown, move off a feature, and then mouseup - typical behavior is not to act on the target of a mousedown in that case).

Can you modify your patch so that the SelectFeature? This will involve renaming the downFeature method to clickFeature and registering that method with the handler on click.

Basically, all this is to say that controls should behave the way you expect them to by default. You shoudn't have to set two callbacks (most users shouldn't be dealing with handler callbacks) - just to get it to behave sensibly.

Also, we've got to start getting strict about writing tests. We've got such a big backlog of tests that need to be written - I don't think it makes sense to accept any patch without an addition to the tests. All this handler stuff made it in during our vector push - we need to start getting some community support on test writing. I've written a very basic drag handler test. This can be used as the basis for a feature handler test.

[elemoine]

Replying to tschaub:

Hmm. This is somewhat messing with the idea of handlers. Users shouldn't have to know about handler callbacks. Users should know about control options. The SelectFeature control currently has one option to control it's behavior - hover. If hover is true, features are selected on hover. If hover is false, features are selected on mousedown.

Understood.

Let me ask this question: today, Handler/Feature.js defines callbacks for mousedown, mousemove, mouseup, dblclick for none for click. Because of this, one cannot use Handler/Feature.js to register a "click feature" callback. What's the reason for this?

I think what we really want here is for the feature handler to stop mouseup, mousedown, and click if they correspond with a feature. Your patch does this. I think we also want the SelectFeature control to select features on click (instead of mousedown) if hover is false. It was an oversight to select on mousedown (as a user may mousedown, move off a feature, and then mouseup - typical behavior is not to act on the target of a mousedown in that case).

Agreed.

Can you modify your patch so that the SelectFeature? This will involve renaming the downFeature method to clickFeature and registering that method with the handler on click.

Will do.

Basically, all this is to say that controls should behave the way you expect them to by default. You shoudn't have to set two callbacks (most users shouldn't be dealing with handler callbacks) - just to get it to behave sensibly.

Hmm... that may be the reason why you don't want Handler/Feature.js to provide callbacks for both mousedown and click. Can you confirm? If that's indeed the reason, the aforementioned patch should remove the mousedown callback in Handler/Feature.js and replace it by a click callback. Right?

Also, we've got to start getting strict about writing tests. We've got such a big backlog of tests that need to be written - I don't think it makes sense to accept any patch without an addition to the tests. All this handler stuff made it in during our vector push - we need to start getting some community support on test writing. I've written a very basic drag handler test. This can be used as the basis for a feature handler test.

Got it!

[elemoine]

New patch taking Tim's comments into account. Tim, does the patch match what your thinking?

[elemoine]

Yet another patch, which leaves the mousedown and mouseup methods in Handler/Feature.js to stop event propagation if they correspond with a feature.

[elemoine]

Replying to elemoine:

Replying to tschaub:

Also, we've got to start getting strict about writing tests. We've got such a big backlog of tests that need to be written - I don't think it makes sense to accept any patch without an addition to the tests. All this handler stuff made it in during our vector push - we need to start getting some community support on test writing. I've written a very basic drag handler test. This can be used as the basis for a feature handler test.

Got it!

Note that I currently work - at night when time permits :-) - on tests for the Feature Handler. Will post them here when complete.

[elemoine]

The attached patch provides tests for the Feature Handler. Note that these tests correspond to my click-feature-handler-3.diff patch.

[elemoine]

The Feature Handler tests I've just added pass on FF. I haven't tried on other browsers since I don't have any handy.

[elemoine]

Simplified code for Feature Handler tests.

[crschmidt]

Tim --

Please take a look at this when you get a chance.

[crschmidt]

Erik: It looks like this was actually pulled into Tim's sandbox without comment: #951 includes the changes you made here. I'm going to clean up the tests so that this applies against trunk, and turn it into a single patch, but I think that this is a definite "Yes, this looks good." I'm sorry I didn't see that before.

[crschmidt]

Erik --

New patch with updated tests. Looks good! Commit when you get a chance.

[elemoine]

(In [4241]) select features on "click" as opposed to on "mousedown" (closes #891) thanks to all for the review and to crschmidt for updating the patch