onkeypress attribute on <body> no longer gets events happening on the window

RESOLVED FIXED in Firefox 10

Status

()

Core
Event Handling
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jonathan Baron, Assigned: smaug)

Tracking

(Depends on: 1 bug, {regression, verified-beta})

Trunk
mozilla10
regression, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9+ affected, firefox10+ verified)

Details

(Whiteboard: [qa+][qa!:10])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
For several years, I have used
<body onkeypress="KeyMove(event)">
to capture key events.  In the last few days (since around October 15, 2011 - I'm not sure when I tried it last), with Minefield, this stopped working.  The JavaScript function KeyMove() is not invoked.  (If I put an alert right at the beginning of it, no alert occurs.)

An example is http://www.sas.upenn.edu/~baron/900/hypoth.htm

What worked up to now was that hitting the PageDown key (for example) moved to the next "slide".  Now it just scrolls down the page.

Comment 1

6 years ago
Regression range

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=95bbaf6cb2a6&tochange=e7854b4d29ba

Comment 2

6 years ago
Created attachment 568602 [details]
testcase

Comment 3

6 years ago
Ok so its caused by:

changeset:   77769:09d60465831c
user:        Boris Zbarsky <bzbarsky@mit.edu>
date:        Wed Sep 28 11:54:50 2011 -0400
summary:     Bug 689564.  Only forward event attributes on body/frameset to the window if we also forward the corresponding on* property.  r=smaug
Blocks: 689564
Keywords: regression
Yeah, I was afraid of this....

We used to forward that to the window, which is not what the spec says to do.

WebKit fires random key events on the body instead of the window.  Olli, what does the spec say about that?
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?
Component: General → Event Handling
Product: Firefox → Core
QA Contact: general → events
Summary: onkeypress or event fails → onkeypress attribute on <body> no longer gets events happening on the window
(Assignee)

Comment 5

6 years ago
What spec?
(Reporter)

Comment 6

6 years ago
Comment on attachment 568602 [details]
testcase

The test case shows that the bug is in "onkeypress" and not in "event".  I wasn't sure.

I don't know if the following helps, but I used the test case on the debug build of 10/16/2011.  I ran it a few times and saved the output.  Sometimes I tested the bug and sometimes I did not.  There were many differences, but the following occurred repeatedly.

At least we now know that the bug was present as early as 10/16.  I might try earlier to get the exact date.

I'm willing to do more tests, but I'm not a C programmer, so I can use advice about what to do.

diff3:> WARNING: NS_ENSURE_TRUE(mDoneSetup) failed: file /builds/slave/m-cen-lnx64-dbg/build/editor/composer/src/nsEditingSession.cpp, line 1382
diff3:> WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file /builds/slave/m-cen-lnx64-dbg/build/content/base/src/nsContentUtils.cpp, line 2548
diff3:> WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file /builds/slave/m-cen-lnx64-dbg/build/content/xbl/src/nsXBLProtoImplMethod.cpp, line 327
diff3:< Assertion failed at /builds/slave/m-cen-lnx64-dbg/build/gfx/cairo/cairo/src/cairo-hash.c:196: hash_table->live_entries == 0
Jonathan, Cork already found the exact date.

Olli, is there any spec that covers targeting keyboard events at all?  If not, I guess we get to see what other browsers do...
(Assignee)

Comment 8

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #7) 
> Olli, is there any spec that covers targeting keyboard events at all?  If
> not, I guess we get to see what other browsers do...
If HTML spec doesn't say anything how key events should work in HTML context, then no.
And at least I don't recall seeing anything about key events in that spec.
(Assignee)

Comment 9

6 years ago
hmm, are keyevents dispatched to window or document. I'd assume document, or perhaps root element
(Assignee)

Comment 10

6 years ago
root element it seems to be.
(Assignee)

Updated

6 years ago
Assignee: nobody → Olli.Pettay
As long as we're here, click events have a similar concern: when clicking outside the body, at least webkit targets the click to the body.

The other option is to forward some of the key and mouse event handlers (but not mouseenter/leave) to the window in general and get html5 changed in that regard.
(Assignee)

Comment 12

6 years ago
I think mouse events should not be forwarded to body if they are over html element.
But for key events using body as the default target (or if it is not there rootElement) makes
sense since that is how activeElement works.
(Reporter)

Comment 13

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #7)
> Jonathan, Cork already found the exact date.
> 
> Olli, is there any spec that covers targeting keyboard events at all?  If
> not, I guess we get to see what other browsers do...

I have always regarded this as a feature of Firefox.  I know it has not worked in IE, and I find now that it does not work in Google-chrome, although it does work in Opera. Thus, it is not clear that the best solution is to see what other browsers do (except for Opera).  I don't know how many others do what I do, but I got the idea from Flanagan's book "Javascript: The definitive guide".  I was just assuming that other browsers were defective because it didn't work, and I use Firefox, or require students to use it, when this feature is needed.
> since that is how activeElement works.

Ah, yes.  Consistency there would be good.

Can we do that for 9?  Then again, given comment 13 this may not be a worry.

> Thus, it is not clear that the best solution is to see what other browsers do 

The best solution is probably whatever gets us interoperability fastest.

Given that this does not work in WebKit and Trident, I'm not that worried about site compat issues.

Note that for your use case, instead of forcing students to use a particular browser you could just put the listener on the window if that's what where you actually want it to be.
(Assignee)

Comment 15

6 years ago
(In reply to Jonathan Baron from comment #13)
> I have always regarded this as a feature of Firefox.
What 'this' exactly?
(Reporter)

Comment 16

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #14)

> Note that for your use case, instead of forcing students to use a particular
> browser you could just put the listener on the window if that's what where
> you actually want it to be.

Thanks.

It works if I put it in the html tag right at the beginning:
<html onkeypress="SomeJavascriptFunction()">.

"It" means trapping key presses as a way of controlling other things.  I've also used this to record reaction times.
(Reporter)

Comment 17

6 years ago
(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Jonathan Baron from comment #13)
> > I have always regarded this as a feature of Firefox.
> What 'this' exactly?

Trapping keys.  Sorry.  But I guess it does work in other browsers.
(Assignee)

Comment 18

6 years ago
Created attachment 568731 [details] [diff] [review]
patch

Neil, what do you think about this?

I do the GetBody thing on purpose to get similar behavior to
activeElement.

I uploaded the patch to tryserver
Attachment #568731 - Flags: superreview?(bzbarsky)
Attachment #568731 - Flags: review?(enndeakin)
Comment on attachment 568731 [details] [diff] [review]
patch

sr=me
Attachment #568731 - Flags: superreview?(bzbarsky) → superreview+
Attachment #568731 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 20

6 years ago
Looks like I need to update browser_keyevents_during_autoscrolling.js
Basically change if (aEvent.target != root)  to if (aEvent.target != root && aEvent.target != root.ownerDocument.body) {
or something like that.

and test_focus.xul needs to be updated too.


But I do still think we should make the change.
(Assignee)

Comment 21

6 years ago
Created attachment 568959 [details] [diff] [review]
patch

Oops, silly me, the patch changed !eventTarget->GetPrimaryFrame() case
for XUL.

I uploaded the patch to try
(Assignee)

Comment 22

6 years ago
Comment on attachment 568959 [details] [diff] [review]
patch

This seems to pass tests on try.
Attachment #568959 - Flags: review?(bzbarsky)
Comment on attachment 568959 [details] [diff] [review]
patch

r=me
Attachment #568959 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f2fa4ae74ee1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 702064
Duplicate of this bug: 712796
We never got this fixed in Firefox 9.  :(

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla10

Updated

6 years ago
Duplicate of this bug: 712843

Updated

6 years ago
status-firefox10: --- → fixed
status-firefox9: --- → affected

Updated

6 years ago
No longer depends on: 702064
(Assignee)

Comment 28

6 years ago
Uh, was this ever triaged for 9? This has still tracking-firefox9?
Though, I should have just asked approval anyway.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 712897

Updated

6 years ago
Duplicate of this bug: 712903

Updated

6 years ago
tracking-firefox10: ? → +
tracking-firefox9: ? → +
Given the number of duplicates here one day after release, I think we should strongly consider chemspilling for this...
(Assignee)

Comment 32

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=00d11969b518

Comment 33

6 years ago
Few users complained about this regression on various Firefox support boards after FF9 release. Is there a chance to see a chemspill release including that patch before FF10 release?
(In reply to Olli Pettay [:smaug] from comment #32)
> https://tbpl.mozilla.org/?tree=Try&rev=00d11969b518

Pushing relbranches to try doesn't work, aiui. You need to merge to default or whatever.

Comment 35

6 years ago
So we changed this to fix yandex maps and to match the spec but there are sites out there that rely on this, right? Or, is this an actual bug?
We made the changes in bug 689564 to align with the spec and to fix yandex maps.

That caused issues on various other sites because the way we dispatched events was different than some other browsers.  This had used to work because we also did event handlers differently from those other browsers (which bug 689564 fixed).

The patch in this bug aligned where we dispatch the events with other browsers; I believe smaug filed a spec bug to make sure the spec defines this.
(Assignee)

Comment 37

6 years ago
https://www.w3.org/Bugs/Public/show_bug.cgi?id=14534
(Assignee)

Comment 38

6 years ago
I'm right now in a place where 3G network connection is unfortunately *really* bad.
If this should be landed to mozilla-release, could someone else do it, please.
The patch does apply cleanly (IIRC fuzz=1, which hg import can handle automatically) to FF9 code.

Updated

6 years ago
Duplicate of this bug: 713205
(Assignee)

Updated

6 years ago
Duplicate of this bug: 713541

Updated

6 years ago
Duplicate of this bug: 713390

Updated

6 years ago
Depends on: 713614
(Assignee)

Updated

6 years ago
No longer depends on: 713614
Whiteboard: [qa+]
Tested on:
http://www.sas.upenn.edu/~baron/900/hypoth.htm => page down key moves to the next slide
http://killersudokuonline.com/play.html?puzzle=D3dy2xk2181 => arrow keys move the yellow box

This is verified fixed on Firefox 10 Beta2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0
status-firefox10: fixed → verified
Keywords: verified-beta
Whiteboard: [qa+] → [qa+][qa!:10]

Updated

6 years ago
Depends on: 702064

Comment 43

6 years ago
FWIW we are tacking this closely for a possible 9.0.2.

Comment 44

6 years ago
I think from my 'dumb user' point of view,. an early 9.0.2 fix is extremely desirable.

For what its worth :)

Oh, and Happy New Year to the team!!
(Assignee)

Comment 45

6 years ago
Created attachment 585482 [details] [diff] [review]
patch

This should apply without any --fuzz to mozilla-release
Attachment #585482 - Flags: approval-mozilla-release?
Comment on attachment 585482 [details] [diff] [review]
patch

[Triage Comment]
We do not expect to roll a 9.0.2 after discussion in yesterday's channel meeting. We'l make sure to triage all minus'd approval‑mozilla‑release bugs if that story changes for whatever reason.
Attachment #585482 - Flags: approval-mozilla-release? → approval-mozilla-release-

Comment 47

6 years ago
It's all very well for you guru chappies, but I am having to resurrect IE6 in a windows virtual machine to do my online puzzles.

That is 'cruel and unusual' punishment for supporting Mozilla. I will be re-drafting my Will.
(In reply to tnp from comment #47)
> It's all very well for you guru chappies, but I am having to resurrect IE6
> in a windows virtual machine to do my online puzzles.

Do you have any particular URLs we can use to test your usecase?

Comment 49

6 years ago
>Do you have any particular URLs we can use to test your usecase?

if I may chip in: http://killersudokuonline.com/play.html?puzzle=D2krrey2197

(I submitted a bug report with that puzzle url, but it got merged in here)
In short, the yellow edit box does not move, making it impossible to play via keyboard.
Duplicate of this bug: 716350

Comment 51

5 years ago
Should the example at https://developer.mozilla.org/en/DOM/event.which#Example be changed to avoid this bug, or should a note be added that the example specifically fails in Firefox 9?
It appears as though this may have caused a regression (bug 848762).
Depends on: 848762
You need to log in before you can comment on or make changes to this bug.