Closed Bug 696020 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10
Tracking Flags:
Tracking Status
firefox9 + affected
firefox10 + verified

People

(Reporter: jonathanbaron7, Assigned: smaug)

References

Details

(Keywords: regression, verified-beta, Whiteboard: [qa+][qa!:10])

Attachments

(4 files)

testcase
114 bytes, text/html
Details
patch
3.37 KB, patch
enndeakin
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch
4.35 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch
3.55 KB, patch
akeybl
: approval-mozilla-release-
Details | Diff | Splinter Review 
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.
Attached file testcase — 

    
    

    
  
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

    
    

    
  
What spec?

    
    

    
  
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...

    
    

    
  
(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.

    
    

    
  
hmm, are keyevents dispatched to window or document. I'd assume document, or perhaps root element

    
    

    
  
root element it seems to be.
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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

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

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


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

I uploaded the patch to try

    
    

    
  
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+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f2fa4ae74ee1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
  
Depends on: 702064

    
    

    
  
We never got this fixed in Firefox 9.  :(

    
  
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla10

    
  

          status-firefox10:
          --- → fixed

          status-firefox9:
          --- → affected

    
  
No longer depends on: 702064

    
    

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

    
    

    
  
Given the number of duplicates here one day after release, I think we should strongly consider chemspilling for this...

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
  
Depends on: 713614
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]

    
  
Depends on: 702064

    
    

    
  
FWIW we are tacking this closely for a possible 9.0.2.

    
    

    
  
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!!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
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-

    
    

    
  
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?

    
    

    
  
>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.

    
    

    
  
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
Component: Event Handling → User events and focus handling

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: