53896 - Spacebar when buttons, checkboxes, radiobuttons have focus just scrolls page

Status: VERIFIED FIXED

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

Description

[Blake Ross]

Build ID: just pulled

Steps to Reproduce:
(1) Go to http://www.cnet.com/frontdoor/0-1.html?st.10001.10001-ron.yhed.1
(2) Click in the Search field and tab twice to give the Go! button focus
(3) Press spacebar

The button action triggers as it should, but the page also scrolls.  This is a 
regression; it used to be that the spacebar keypress ended with the button.  
Sounds like it isn't being killed now, and bubbling up to other places...

Comment 1

[Loco]

Updating QA Contact.

Comment 2

[rods (gone)]

*** Bug 25164 has been marked as a duplicate of this bug. ***

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 4

[Blake Ross]

nominating for rtm.  this has regressed even more, and now you simply cannot do 
anything in webpages with the keyboard because spacebar no longer works to 
toggle any controls.  see also bug 51600.

Comment 5

[Blake Ross]

nisheeth, could you please consider plussing this?

Comment 6

[Peter Trudelle]

->saari, who has dealt with similar problems many times.

Comment 7

[saari (gone)]

Are you nominating this for RTM because of the scrolling, the button not
responding to space, or both?

Comment 8

[Blake Ross]

I'm nominating for rtm for checkboxes, radiobuttons, and buttons not responding 
to space (not just buttons).  It would be extremely annoying to have the page 
also scroll, but it'd be better than the current state (space not working at 
all).

So rtm would be to make space work, regardless of what else happens.

Comment 9

[saari (gone)]

patches for both problems

Index: nsXBLWindowKeyHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xbl/src/nsXBLWindowKeyHandler.cpp,v
retrieving revision 1.5
diff -u -r1.5 nsXBLWindowKeyHandler.cpp
--- nsXBLWindowKeyHandler.cpp 2000/10/04 00:41:53 1.5
+++ nsXBLWindowKeyHandler.cpp 2000/10/10 21:25:32
@@ -254,7 +254,6 @@
rec = do_QueryInterface(elt);
rv = currHandler->ExecuteHandler(rec, aKeyEvent);
if (NS_SUCCEEDED(rv)) {
- aKeyEvent->PreventDefault();
return NS_OK;
}
}

Index: nsXBLPrototypeHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xbl/src/nsXBLPrototypeHandler.cpp,v
retrieving revision 1.9
diff -u -r1.9 nsXBLPrototypeHandler.cpp
--- nsXBLPrototypeHandler.cpp 2000/10/04 00:41:51 1.9
+++ nsXBLPrototypeHandler.cpp 2000/10/10 21:29:35
@@ -271,6 +271,7 @@
mHandlerElement->GetAttribute(kNameSpaceID_None, kOnCommandAtom, handlerT
ext);
if (handlerText.IsEmpty())
return NS_ERROR_FAILURE; // For whatever reason, they didn't give us an
ything to do.
+ aEvent->PreventDefault(); // Preventing default for XUL key handlers
}
}

Index: htmlBindings.xml
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/htmlBindings.xml,v
retrieving revision 1.8
diff -u -r1.8 htmlBindings.xml
--- htmlBindings.xml    2000/09/22 19:03:39     1.8
+++ htmlBindings.xml    2000/10/10 21:48:03
@@ -9,10 +9,19 @@
       <handler event="keypress" key=" ">
       <![CDATA[
         var v = document.commandDispatcher.focusedElement;
-
-                         if (v && (v.localName == 'TEXTAREA'))
-                           return true;
-
+        if (v && (v.localName == 'TEXTAREA')) {
+          return true;
+        }
+        if (v && v.localName == 'INPUT') {
+          var str = v.getAttribute('type').toLowerCase();
+          if (str == 'button' || str == 'submit' || str == 'reset') {
+            return true;
+          }
+          str = v.getAttribute('TYPE').toLowerCase();
+          if (str == 'button' || str == 'submit' || str == 'reset') {
+            return true;
+          }
+        }
         var controller =
           document.commandDispatcher.getControllerForCommand('cmd_scrollPageDow
n');
           controller.doCommand('cmd_scrollPageDown');
@@ -20,7 +29,6 @@
         return true;
       ]]>
       </handler>
-
       <handler event="keypress" keycode="VK_PAGE_UP" command="cmd_scrollPageUp"
/>
       <handler event="keypress" keycode="VK_PAGE_DOWN" command="cmd_scrollPageD
own"/>

Comment 10

[Dan M]

r=danm

Comment 11

[David Hyatt]

a=hyatt

Comment 12

[Blake Ross]

great!

nisheeth, please plus this ASAP so PDT can approve.

Comment 13

[Peter Trudelle]

Very visible, maddening bug, fix is not one-liner, but is small and
well-understood.  Would be good to get in for N6.  rtm+ since it is ready to go,
cc joki for possible further review, cc jrgm for further testing before landing.

Comment 14

[Michael La Guardia]

Please check this into the trunk and bake it for a cycle and then re-nominate.
marking need info

Comment 15

[Blake Ross]

saari: let me know if you want me to check this in.

Comment 16

[Blake Ross]

Saari, hope you don't mind that I checked this into the trunk.  trudelle and I 
agreed that it should be in tomorrow's builds so PDT could evaluate regressions 
and so forth.

Comment 17

[saari (gone)]

Blake, no problem at all, thanks for doing that!

Comment 18

[John Morrison]

This is working for the input element types that were 
specified mac/linux/win32). The XBL/js just needs to 
also check <input> for types 'checkbox | radio | file',
and should also check for element <button> which can 
have a type attribute of 'submit | button | reset'

Comment 19

[Blake Ross]

Attachment: new patch for what John pointed out

Comment 20

[Blake Ross]

Thanks for catching that; attached a new patch.  Why are the two checks for 
`TYPE' and `type' needed?

Comment 21

[John Morrison]

I don't believe they are required (doing .getAttribute in two cases). I did
a simple test case in HTML for type='submit' using "TYPE"|"TyPe"|"type", and
.getAttribute('type') returns "submit" in all cases. I can't find a statement
in DOM2 that says that is mandated, though.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

HTML: Internally we handle everything in lower case. Searching for element & 
attributes is case-insensitive, I seem to recall. Contact jst if you want to be 
sure.

Comment 23

[Dan M]

r=danm on Blake's patch 16828

Comment 24

[Phil Peterson]

Looks like you need a super review for the new patch. Marking [rtm need info].
I'll be online for a while if you get one tonight -- don't want to lose a day
since you tested on the trunk as requested.

Comment 25

[Brendan Eich [:brendan]]

a=[e-mail] on the patch as is for the branch.

For the trunk, let's avoid the unnecessary uppercase getAttributes, use switch
(better performance), and factor common guard expressions:

        if (v) {
          switch (v.localName) {
            case 'TEXTAREA':
              return true;
    	    case 'INPUT':
              switch (v.getAttribute('type').toLowerCase()) {
		case 'button':
		case 'submit':
		case 'reset':
		case 'checkbox':
		case 'radio':
		case 'file':
            	  return true;
              }
	      break;
            case 'BUTTON':
              switch (v.getAttribute('type').toLowerCase()) {
                case 'submit':
		case 'button':
		case 'reset':
                  return true;
              }
	      break;
          }

Does that look better? I think we can do a fast r=/a= if you can do the above in
a trunk patch.  Thanks,

/be

Comment 26

[Blake Ross]

Brendan's trunk patch looks good to me. r=blake

Marking rtm+ since we now have a super reviewer for the branch patch as 
requested.

Comment 27

[saari (gone)]

Brendan's patch looks great, even tested it. Anyone want to ++ this?

Comment 28

[Blake Ross]

might as well just get brendan's patch into the trunk, there's no greater 
risk...

so we have r=blake, saari.  we need an sr= and we're good to go.  cc'ing hyatt 
so hopefully he can do that.  Meanwhile, Chris, can you put Brendan's patch in 
the form of a branch diff?  Thanks!

Comment 29

[saari (gone)]

Form of a branch diff

Index: htmlBindings.xml
===================================================================
RCS file: /m/pub/mozilla/xpfe/global/resources/content/htmlBindings.xml,v
retrieving revision 1.8
diff -u -2 -r1.8 htmlBindings.xml
--- htmlBindings.xml	2000/09/22 19:03:39	1.8
+++ htmlBindings.xml	2000/10/13 00:33:25
@@ -10,8 +10,30 @@
       <![CDATA[
         var v = document.commandDispatcher.focusedElement;
-
		
-
		  if (v && (v.localName == 'TEXTAREA'))
-
		    return true;
-
+
+
+        if (v) {
+          switch (v.localName) {
+            case 'TEXTAREA':
+              return true;
+    	      case 'INPUT':
+              switch (v.getAttribute('type').toLowerCase()) {
+
	            case 'button':
+
	            case 'submit':
+
	            case 'reset':
+
	            case 'checkbox':
+
	            case 'radio':
+
	            case 'file':
+            	    return true;
+              }
+
            break;
+            case 'BUTTON':
+              switch (v.getAttribute('type').toLowerCase()) {
+                case 'submit':
+
	            case 'button':
+
	            case 'reset':
+                  return true;
+              }
+
            break;
+          }
         var controller =
           document.commandDispatcher.getControllerForCommand('cmd_scrollPageDown');

Comment 30

[David Hyatt]

a=hyatt

Comment 31

[saari (gone)]

Fix in on branch and trunk

Comment 32

[Blake Ross]

Chris, Brendan's patch introduced a new block if without adding the closing 
brace at the end.  Because of this, a JS error is displayed in the console 
every time you press spacebar when certain HTML and XUL widgets have the 
focus.  Can you add that brace (one line fix) and check it into the branch 
under the auspicies of this ++ bug?

Comment 33

[David Krause]

Here's the error.

JavaScript error:
 line 33: missing } in compound statement

Comment 34

[(not reading, please use [e-mail] instead)]

expect a flood of duplicates on

JavaScript error:
line 33: missing } in compound statement


here's the fix:

Index: htmlBindings.xml
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/htmlBindings.xml,v
retrieving revision 1.10
diff -p -r1.10 htmlBindings.xml
*** htmlBindings.xml	2000/10/13 21:36:31	1.10
--- htmlBindings.xml	2000/10/15 04:05:33
***************
*** 35,40 ****
--- 35,41 ----
}
break;
}
+ 	}
var controller =

document.commandDispatcher.getControllerForCommand('cmd_scrollPageDown');
controller.doCommand('cmd_scrollPageDown');

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

r=shaver to get rid of that annoyance.

Comment 36

[timeless]

r=timeless

Comment 37

[Blake Ross]

*** Bug 56835 has been marked as a duplicate of this bug. ***

Comment 38

[Blake Ross]

*** Bug 56827 has been marked as a duplicate of this bug. ***

Comment 39

[Blake Ross]

ok, this needs to be fixed because it also caused bug 56835.  I'll check it in 
with r=timeless, sr=shaver.

Comment 40

[saari (gone)]

Fix is fine by me

Comment 41

[R.K.Aa.]

*** Bug 56642 has been marked as a duplicate of this bug. ***

Comment 42

[Blake Ross]

oops...was checking this in as saari was.  remarking FIXED

Comment 43

[Blake Ross]

*** Bug 56987 has been marked as a duplicate of this bug. ***

Comment 44

[R.K.Aa.]

When will the fix be in the nightlies? Testing with 2000101708 i still see bug
56642.

Comment 45

[Jerry Baker]

*** Bug 57133 has been marked as a duplicate of this bug. ***

Comment 46

[R.K.Aa.]

and gone. I don't see it anymore in 2000101809.

Comment 47

[Loco]

VERIFIED on Mac 10-18-08, NT 10-18-08, Linux 10-18-09 branch builds.  Adding 
vtrunk KW.  Yay!

Comment 48

[Asa Dotzler [:asa]]

Verified Fixed on Mozilla trunk builds spacebar no longer scrolls page when
focus is on buttons, checkboxes or radio butons.
linux 101908 RedHat 6.2
win32 101904 NT 4
mac 101904 Mac OS9
Setting bug to Verified and removing vtrunk keyword

Comment 49

[Blake Ross]

saari, the brace fix still needs to go into the trunk.

Comment 50

[Akkana Peck]

Reopening to get the fix into the trunk; removing rtm++ since it's already in
the branch.

Comment 51

[Asa Dotzler [:asa]]

oops, sorry about that premature Verification.  I didn't read down far enough in
the bug before I followed the steps to repro.

Comment 52

[saari (gone)]

Oops, I thought it was in the trunk. Okay

Comment 53

[timeless]

Sorry all, we got tired of waiting. Fix checked in. verifyme [trunk]

Comment 54

[Blake Ross]

remarking ++ for regression testing

Comment 55

[Loco]

Adding vtrunk KW again just in case.

Comment 56

[Scott Brodmerkle]

*** Bug 57670 has been marked as a duplicate of this bug. ***

Comment 57

[Asa Dotzler [:asa]]

OK, verifying the second part.  No console errors :)
Verified Fixed on Mozilla trunk builds
linux 102308 RedHat 6.2
win32 102304 NT 4
mac 102312 Mac OS9
Setting bug to Verified and removing vtrunk keyword

Comment 58

[Casey Perkins]

After having not seen this for a long time, I noticed this behavior occurred in
the nightly build I downloaded today (2001042908). Trying to use the spacebar to
submit a form scrolled the page instead. Enter still works to submit a form.

Comment 59

[Brendan Eich [:brendan]]

Arik, is it possible your XBL patch, recently landed, regressed this?  Just
asking, I'm not sure at all.

/be

Comment 60

[Blake Ross]

Reopening.

Comment 61

[saari (gone)]

argh! I'm cursed, forever doomed with this filth.

Comment 62

[Blake Ross]

No worries :-)

Arik, you somehow messed up the checkin to nsXBLPrototypeHandler.cpp:

http://bonsai.mozilla.org/cvsview2.cgi?
diff_mode=context&whitespace_mode=show&subdir=mozilla/content/xbl/src&command=DI
FF_FRAMESET&file=nsXBLPrototypeHandler.cpp&rev1=1.23&rev2=1.24&root=/cvsroot

compare with

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31655

So you effectively removed the conditional node checks from htmlBindings.xml 
without translating them into the equivalent C++, which would cause this 
problem.

(Please be sure you fix on the 0.9 also if necessary).

Comment 63

[Blake Ross]

Reclosing this, will handle in 71760.

Comment 64

[Brendan Eich [:brendan]]

Looks like the branch got the full patch for nsXBLPrototypeHandler.cpp from bug
71760, but the trunk didn't.  Tsk.

/be

Comment 65

[Richard Hurt]

Well, the spacebar doesn't scroll now, but it doesn't activate the button
either.  I confirmed this on WinNT 4.0 (sp5) build : 2001043007

Comment 66

[Boris Zbarsky [:bzbarsky]]

*** Bug 78383 has been marked as a duplicate of this bug. ***

Comment 67

[Boris Zbarsky [:bzbarsky]]

*** Bug 79483 has been marked as a duplicate of this bug. ***

Comment 68

[Boris Zbarsky [:bzbarsky]]

*** Bug 79253 has been marked as a duplicate of this bug. ***

Comment 69

[Blake Ross]

Reclosing, this no longer scrolls.

Comment 70

[Boris Zbarsky [:bzbarsky]]

bug 79483 is handling the regression of not triggering things.

Comment 71

[Jacek Piskozub]

Verifying fixed with 2001063003 on WindowsME.