Opened 12 years ago

Closed 11 years ago

#6876 closed defect (fixed)

Dojo.keys constant wrong for Safari 3.0, 3.1

Reported by: guest Owned by: sjmiles
Priority: high Milestone: 1.2
Component: Events Version: 1.1.0
Keywords: Dojo keys Safari 3.04 Cc: bill, sebastien.peneau@…, jeanfrancois.cunat@…, davidb, sjmiles
Blocked By: Blocking:

Description (last modified by Douglas Hays)

Same problem as described in Ticket #5951 but on a MacBook? laptop with Safari 3.04. When pressing DOWN_ARROW, the evt.keyCode show me "40" but the dojo.keys.DOWN_ARROW 63233 (which is the charCode as seen in _base.events).
The result is that most of the widgets that use nonprintables keys are broken on the latest Safari.

Attachments (2)

event.patch (5.5 KB) - added by James Burke 11 years ago.
Possible patch that does not use IE paths for Safari
event2.patch (5.6 KB) - added by bill 11 years ago.
corrected version of event.patch w/proper disconnect

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by bill

Component: CoreEvents
Owner: changed from anonymous to sjmiles

comment:2 Changed 11 years ago by guest

See the webkit-dev email on the topic. Dojo may have to implement some workarounds for the modification of keyboard events overall, but for this particular bug, the last bit of the email is the key:

One consequence from the above changes is that Mac key codes (such
as 63232 for up arrow) are never exposed via DOM any more.

comment:3 Changed 11 years ago by bill

Milestone: tbd

mark all (open) tickets w/blank milestones to be "tbd"; their milestones need to be set to a version number or to "future"

comment:4 Changed 11 years ago by Douglas Hays

Cc: bill added
Description: modified (diff)
Priority: normalhighest
severity: normalblocker
Summary: Dojo.keys constant wrong for Safari 3.0 on MacBookDojo.keys constant wrong for Safari 3 on MacBook

Running Safari 3.1.2 on WinXP, 40 is being returned as well.
The bigger problem is that arrow keys are no longer generating keypress events, so Safari 3.1 needs to generate a fake keypress just like IE.

comment:5 Changed 11 years ago by Douglas Hays

Milestone: tbd1.2

comment:6 Changed 11 years ago by bill

Summary: Dojo.keys constant wrong for Safari 3 on MacBookDojo.keys constant wrong for Safari 3.0, 3.1

Just updating title to reflect that it's not specific to Safari 3.0.

comment:7 Changed 11 years ago by davidb

Cc: davidb added

comment:8 Changed 11 years ago by bill

Owner: changed from sjmiles to Douglas Hays

Doug is working on this. Apparently Safari works like other browsers now so we can just remove safari-specific path?

comment:9 Changed 11 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

(In [15273]) Fixes #6876 !strict. Force safari to take a similar code path as IE and generate faux keypress events. Removed the mac keycodes since safari 3.x uses the same keycodes as IE and FF.

comment:10 Changed 11 years ago by dylan

A silly question... can we make sure this change doesn't break Adobe AIR, which might be based on a weird version of WebKit?...

comment:11 Changed 11 years ago by bill

(In [15348]) Fixes #7748, refs #6876: reinstate faux onmouseenter / onmouseleave events for safari. !strict

comment:12 Changed 11 years ago by James Burke

Resolution: fixed
Status: closedreopened

I'm still a bit uncomfortable forcing Safari through the IE code paths, in particular the last block in event.js for the dojo._ieDispatcher. It seems bad to use the IE leak branch of the code.

The other branch force around line 193 to get the synthesizing stuff: I would like to avoid that too. I am horribly ignorant in this area though. Are there some tests I can run to verify that keypress needs to by synthesized now for Safari 3?

If it is required that we need all that synthesizing code for Safari, could we maybe change line 208 to:

if(!dojo.config._allow_leaks && !dojo.Safari){

so that we could remove the dojo.isSafari part of the if check that defines dojo._ieDispatcher at the bottom of the page.

comment:13 Changed 11 years ago by James Burke

Cc: sjmiles added

comment:14 Changed 11 years ago by Douglas Hays

If _allow_leaks = true, then IE fails to send fake keypress events. So until that bug is fixed, both IE and Safari need to take the !dojo.config._allow_leaks codepath.

comment:15 Changed 11 years ago by Douglas Hays

Milestone: 1.2future
Priority: highestnormal
severity: blockernormal

Changing to future since I'm not aware of any show-stopper issue for 1.2 with the current code.

comment:16 Changed 11 years ago by Adam Peller

Milestone: future1.2
Owner: changed from Douglas Hays to sjmiles
Priority: normalhigh
Status: reopenednew

Changing the milestone to future would seem to ignore a lot of feedback from committers. Assigning back to the module owner for consideration in 1.2. Scott, please let us know whether you are comfortable with the current patches or whether we should roll this back and give it some more time. I'm leaning heavily towards rolling back [15273] [15348] pending further review and testing. This is base code.

comment:17 Changed 11 years ago by Douglas Hays

If we rollback [15273], then there will be several regressions from 1.1.
Here are some that come to mind:
TimeTextBox/ComboBox/FilteringSelect?: can't popup the menu with the down arrow key and cannot select menu items with the up and down arrow keys Dialog: tabbing will not wrap from last to first and vis-a-versa
Spinner: no spinning with the arrow keys
DropDownButton?: can't popup the menu with the down arrow key
I'm sure there are others that I'm not aware of.

comment:18 Changed 11 years ago by dante

I was under the impression they won't be regressions compared to 1.1, as Safari 3.1 wasn't even released at the time, and the old code worked in 3.0. You are basing the statement of regressions on that fact, and admittedly Safari users typically upgraded 3.1 -- if they are using 3.0 with the old code, it doesn't "regress" in this case. It is bad for us not to support 3.1 immediately, but we've never made any promise of a11y/keyboard support for safari (to quote becka11y). Only in 3.1 was it even a possibility, but adding in code that may or may not have serious adverse side effects without a good period of time for further testing is a Bad Thing (tm) and IMHO we should revert the core code to the last stable revision, and address the immediate issue in the dot release, and introduce the first steps of Safari keyboard access in the 1.3 release.

comment:19 Changed 11 years ago by bill

Hi guys. I wouldn't decide based on dijit, as even w/the change safari keyboard support is lacking. For instance, you can't use the arrow keys to navigate the items in a menu, nor does the tab key work reliably (it skips over ComboButton, for example).

However, given that dojo core used to support safari keyboard, and given that our FAQ says that we support the latest safari, that puts core between a rock and a hard place. (Note that this ticket shows people are using keyboard on safari, unrelated to dijit.)

If you are uncomfortable with the 12 days of testing safari theoretically got after this change, I guess you can roll it back and just document it in the release notes/FAQ that we no longer support keyboard on safari. It's a toss up.

In any case, you should remove the dojo.keys mapping table for safari as that is no longer relevant.

comment:20 Changed 11 years ago by James Burke

To be clear: the keyboard support for onkeypress still works in Safari 3.1 for regular keys, it just does not work for arrow keys.

So rolling back the change still seems like the best option if we want to release today. I am working on a fix that would allow arrow keys to work, but it bloats the code.

According to Scott's comments here: http://thread.gmane.org/gmane.comp.web.dojo.devel/9037/focus=9039

he favors just moving to an event facade model to avoid these sorts of issues.

So if we want to release today, I say roll back, and document that people need to use onkeydown in Safari if they want arrow key support. We could even change this for the dijits if we really wanted to support it (add in an onkeydown that only activates for arrows).

I am trying to do a fix, but it is starting to look very ugly. So let's rollback the changes to event (but remove the mapping as Bill mentioned), then move this bug to future. I'll try to do that after dinner tonight, unless I hear otherwise. If any of the dijit folks want to support arrow keys for dijits, then maybe we can work out a patch just for the dijits.

comment:21 Changed 11 years ago by James Burke

(In [15368]) Refs #6876: a test page for keypress events.

Changed 11 years ago by James Burke

Attachment: event.patch added

Possible patch that does not use IE paths for Safari

comment:22 Changed 11 years ago by James Burke

I just attached a patch that does not use the IE paths. It basically pulls out the code to simulate keypress events for special characters onkeydown.

I also fixed a bug where mouseenter/mouseleave events in non-IE browsers are not disconnected. This issue is in Dojo 1.1.1 too. So technically not a regression that we would have to fix for 1.2, but maybe in 1.2.1.

I duplicated some code instead of delegating to some common functions, since it would introduce two extra function calls in the IE case, and I wanted to keep the function fast.

So it seems to work, except the remove case (triggered by dojo.disconnect). For some reason the onkeydown function that simulates onkeypress for special characters does not get unregistered.

So at this point, I'm burned out on this. I like this newer approach better, but it has less testing and still the disconnect issue to fix. At this point, the current code that uses the IE path has more testing than this path, and I guess we could ship it and just fix any issue that comes up as a side effect of using the IE path in a 1.2.1 release, and maybe consider my patch's approach for 1.3.

It might be nice to pick up the onmouseenter/leave fix in del.remove (at the top of the patch) to fix the dojo.disconnect issue with those simulated events though.

Changed 11 years ago by bill

Attachment: event2.patch added

corrected version of event.patch w/proper disconnect

comment:23 Changed 11 years ago by bill

Hi James,

The issue with disconnect was that you were doing a connect() on "keydown" but not saving the handle. In other words, there are two handles involved and both need to be tracked. I attached an updated patch that fixes that problem.

Note that the IE branch is rather complicated: no matter how many onkeypress handlers are registered, there's only one onkeydown faux event handler... it does reference counting to know when to delete that handler. But in my patch I followed your lead and create a separate keydown (faux keypress generating) handler for every keypress handler the user registers.

Dijit ComboBox and Spinner as well as your eventKeyPress.html test cases seem to be working fine, so I vote to check this in (event2.patch) and release 1.2.

comment:24 Changed 11 years ago by James Burke

Fixed in [15371]: Thanks to Bill for improving the patch. Arrow keys now detectable via onkeypress in Safari 3.1 without using IE code paths. Also, dojo.disconnect for onmouseenter/leave for non-IE browsers should work now.

Bill, thanks for the improved patch and finding the issue. It was getting very late for me last night, and I was going cross-eyed. I like your use of stealthKeyDownHandle. I took your patch and did some slight modifications to reduce the code, now that we have a spiffy stealthKeyDownHandle to use:

  • I got rid of the event = del._normalizeEventName(event); in remove, since we just detect for stealthKeyDownHandle to do the code branch.
  • I removed the var stealthKeyDown declaration and inlined that function into the del._add for the keydown.

Just trying to save some bytes.

On the IE code using just one keydown listener: yeah, I did not want to duplicate the bookkeeping code. Not doing it meant less code, and it was nice to just be able to reference the fp function in the stealthKeyDown closure. And originally I had trouble keying off of evt.currentTarget.onkeypress function as the IE code does, but that was early on and I was probably doing something wrong. I also felt like Safari's performance is so much better than IE that we can get away with it.

However, we can revisit it for 1.3 if anyone feels it is wasteful.

comment:25 Changed 11 years ago by James Burke

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.