OpenLayers OpenLayers

Ticket #1292 (closed bug: fixed)

Opened 11 months ago

Last modified 4 months ago

keyboardDefaults broken in Safari 3

Reported by: pspencer Assigned to: crschmidt
Priority: critical Milestone: 2.7 Release
Component: Control.KeyboardDefaults Version: 2.5
Keywords: Cc:
State: Complete

Description

In Safari 3, the KeyboardDefaults control does not work. Here is what I have discovered so far:

  • Safari 3 (and perhaps earlier versions) returns both keyCode and charCode, where keyCode matches the OpenLayers event values and charCode is some other number. For instance, the UP key returns a keyCode of 38 and charCode returns 63232. The handler in Keyboard.js prefers charCode over keyCode and so the right value is never passed to the defaultKeyPress function. Reversing the preference in Keyboard.js causes Safari to return the OpenLayers value. I'm not sure what the impact of this change is on other browsers.
  • Safari 3 doesn't seem to support keypress, just keydown and keyup. There is code in Events.js observe() and stopObserving() that looks to see if the name of the event is 'keypress' and changes it to 'keydown' in the case that navigator.appVersion matches /Konqueror|Safari|KHTML/ or element.attachEvent is defined. If I change the KeyboardHandler code to use keydown then it works in Safari, so somehow it is not changing keypress to keydown.

Attachments

keyboardsupport.patch (11.9 kB) - added by tcoulter on 07/30/08 16:07:09.
Revamped keyboard support for more browsers.
keyboard.patch (12.2 kB) - added by crschmidt on 07/30/08 16:22:38.
keyboard.2.patch (13.4 kB) - added by tcoulter on 07/30/08 16:41:14.
New patch with edited ModifyFeature control.
keyboard.3.patch (14.9 kB) - added by pagameba on 07/31/08 08:51:28.
updated patch with working tests for ModifyFeatures
keyboard.4.patch (16.9 kB) - added by pagameba on 07/31/08 11:46:55.
new patch with modification to get things working in IE as well.
keyboard_2.patch (15.9 kB) - added by crschmidt on 07/31/08 13:15:40.

Change History

01/22/08 16:37:22 changed by crschmidt

  • milestone changed from 2.6 Release to 2.7 Release.

04/28/08 16:51:34 changed by pspencer

  • owner set to pspencer.

I'll work on this for 2.7.

07/28/08 14:53:38 changed by euzuro

  • owner changed from pspencer to tcoulter.
  • priority changed from major to critical.

07/30/08 16:06:40 changed by tcoulter

Adding a patch that provides keyboard support for Safari, as well alternate key bindings for similar functions (such as the keypad + key to zoom in). This patch also makes KeyboardDefaults.js standardize on evt.keyCode, and makes Keyboard.js pass the whole event instead of the code. I've resurrected an old test, and edited it to assert this new functionality.

07/30/08 16:07:09 changed by tcoulter

  • attachment keyboardsupport.patch added.

Revamped keyboard support for more browsers.

07/30/08 16:07:56 changed by tcoulter

  • owner changed from tcoulter to crschmidt.
  • state changed from Needs Discussion to Review.

07/30/08 16:22:38 changed by crschmidt

  • attachment keyboard.patch added.

07/30/08 16:41:14 changed by tcoulter

  • attachment keyboard.2.patch added.

New patch with edited ModifyFeature control.

07/31/08 08:32:00 changed by pagameba

I just tested the patch against trunk on Safari 3 using the accessible example (since I reported it, I figured I should), the keyboard handling works as expected with one minor problem that I don't think should stop the patch going in - if the browser is short enough or narrow enough that the page scrolls, then pressing an arrow key also scrolls the page.

These (related) tests fail in Safari:

test_initDeleteCodes fail 1 ok 2 ok Delete code properly turned into an array. ok Default deleteCodes include delete fail Default deleteCodes include 'd'. eq: values differ: got 68, but expected 100 test_handleKeypress planned 10 assertions but got 0; fail 0 ok 0

There is also a failure in the Map tests in Safari but I am fairly certain it is unrelated (seems to be related to themes).

I'll see if I can modify the patch to correct the above test failures (hopefully just by updating the tests?)

07/31/08 08:33:20 changed by pagameba

sorry, I should have said the ModifyFeature control tests are the ones that are failing.

07/31/08 08:51:28 changed by pagameba

  • attachment keyboard.3.patch added.

updated patch with working tests for ModifyFeatures

07/31/08 08:53:24 changed by pagameba

The test just needed to be updated to pass an object with keyCode to mimic an event rather than passing just the code. Also note that the default value used to detect the 'd' key changed from 100 to 68, I didn't check to see if this was valid, I just updated the test to look for the correct value.

07/31/08 11:09:10 changed by crschmidt

Okay, so now all I want from this is to get a manual test of the modifyfeature control in IE.

07/31/08 11:43:24 changed by crschmidt

Keyboard stuff doesn't work at all in IE.

07/31/08 11:44:52 changed by crschmidt

  • state changed from Review to Needs More Work.

07/31/08 11:46:11 changed by pagameba

  • state changed from Needs More Work to Review.

(mid-air collision with chris)

Now that I have IE 7 open (after Vista conveniently decided to reboot to install updates in the middle of my manual test), I tried to use the modifyFeature.html example - delete/d doesn't seem to work for me, but it does work in ffox3 and safari.

In fact, the accessible.html page doesn't work in IE 7 either - argh.

With some nifty alert debugging, it seems that the keyboard handler never gets called in IE - the problem is in the Keyboard handler, IE doesn't support registering events on 'window', only on 'document'.

With this change, IE works with accessible.html and modifyFeature.html (d and del keys). Firefox 3 continues to work with this change, as does Safari.

New patch coming in a sec.

07/31/08 11:46:55 changed by pagameba

  • attachment keyboard.4.patch added.

new patch with modification to get things working in IE as well.

07/31/08 12:46:44 changed by crschmidt

  • state changed from Review to Needs More Work.

#1108 discusses the problem in more detail: specifically, pointing out that the keydown on '+' fires twice in IE (!!), so this is not a complete fix yet.

07/31/08 13:15:06 changed by crschmidt

Grrr, prototype changes all IE/Safari "keypress" events to "keydown" events behidn the scenes, so we get double registration. I'm uploading a new patch that includes paul's changes, but also changes the keyboard handler to not touch keypress at all (since we use keydown everywhere in the code at this point, and I doubt anyone else is really using the keyboard handler as is, or we'd have heard more.)

07/31/08 13:15:40 changed by crschmidt

  • attachment keyboard_2.patch added.

07/31/08 13:32:32 changed by crschmidt

  • state changed from Needs More Work to Review.

Paul:

Wanna give a glance at the latest patch/offer feedback?

(Also, I hate prototype).

07/31/08 13:40:44 changed by pagameba

Yep - just got another mid-air collision! This now works for me in IE 7 with no double event. Also still works in other browsers - let's commit!

07/31/08 13:45:16 changed by crschmidt

  • status changed from new to closed.
  • state changed from Review to Complete.
  • resolution set to fixed.

(In [7644]) * Fixes to the Keyboard Handler to make it work better --

  • drop keypress event (not used, registers as keydown in IE< which results in double events)
return evt instead of evt.keyCode evt.charCode, so apps can do

whatever they like best

  • adjust ModifyFeature to new API
  • Adjust KeyboardDefaults to new API, and include a new set of keyCodes

in switch statement to catch more cases

  • Include keyboard defaults test in list-tests.

Patch from tcoulter, work from Pedro Simonetti (See #1108), Paul Spencer, myself. r=pagameba,me (Closes #1292)