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

Don't react to keyboard events when the focus is on editable inputs #903

Merged
merged 1 commit into from
Aug 23, 2013

Conversation

fredj
Copy link
Member

@fredj fredj commented Aug 14, 2013

Don't pan or zoom when the focus is on an editable inputs (input, select or textarea)

@@ -240,7 +240,6 @@ ol.Map = function(options) {
this.handleMapBrowserEvent, false, this);
this.registerDisposable(mapBrowserEventHandler);

// FIXME we probably shouldn't listen on document...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this comment should be removed :)

What should be the behaviour when there are two maps on the page and the user presses an arrow key? Should both maps scroll or just one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(comment re-added)
I had removed the comment because we have to listen on document.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to listen just on the map div? IIRC you need to add the tabindex="0" attribute to the div. There's some discussion on Stack Overflow here and here.

I don't know much about keyboard event handling, so it's entirely possible that I've missed something important here. Don't hesitate to tell me what it is :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without any focus (click or tab), the event target is always document.body.

The tabindex="0" trick allows the div to be focusable and to be able to receive keyboard events; the div still need to be focused (click or tab).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means that we can't just listen on the map div because the user will still have to manually focus the div.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations @fredj. That makes perfect sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to also support the mode where keyboard interaction is active only when the map div has the focus. That's why ol2's keyboard handler has the observedElementoption: https://github.com/openlayers/openlayers/blob/master/lib/OpenLayers/Handler/Keyboard.js#L41.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that the default behavior should be same as ol2 (only react when the map div has the focus) ?
EDIT: in ol2, the default is to listen to document

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, eventually, it will make sense to that an option for that (as in ol2).

@twpayne
Copy link
Contributor

twpayne commented Aug 14, 2013

Looks good - it's an improvement and I'm +1 for merging - but I think that we still don't handle keyboard events correctly.

@elemoine
Copy link
Member

What's implemented in ol2 is also relevant I think.

@fredj
Copy link
Member Author

fredj commented Aug 15, 2013

What's implemented in ol2 is also relevant I think.

The implementation is more or less the same: OpenLayers.Handler.Keyboard listens to the document tag (the default but it's configurable) and the controls uses the event.

In ol2, it's the control responsibility to test if the focus is on an input (see OpenLayers.Control.KeyboardDefaults.defaultKeyPress)

@elemoine
Copy link
Member

Looks good to me. Please merge.

fredj added a commit that referenced this pull request Aug 23, 2013
Don't react to keyboard events when the focus is on editable inputs
@fredj fredj merged commit 637bbeb into openlayers:master Aug 23, 2013
@fredj fredj deleted the keyboard branch August 23, 2013 06:40
afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
Google Maps and WMS layer Backbuffer error. r=@mosesonline
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