501135 - pressing space in a checkbox should toggle it

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[timeless]

fennec 1.0b2 OS X
steps:
1. load test url
2. tap the checkbox
3. press space

actual results:
nothing happens when you press space

expected results:
checkbox should be toggled

Comment 1

[Antonio Gomes (tonikitoo)]

that could be also the case of radiobuttons and normal buttons ... i.e. overall html input elements

Comment 2

[Ben Combee (:bcombee)]

I looked through our sources -- I didn't see any special bindings we do for spacebar.  Also, we do focus on input fields and I was able to tab between a field and a checkbox.  I do see the bug happening, but I don't have a good idea of the mechanism.

Comment 3

[Ben Combee (:bcombee)]

OK, I've a theory.  Looking at
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsButtonBoxFrame.cpp#119, it looks like key up for space will only work if the control is in "active | hover" state.  Maybe we don't allow that state, perhaps due to how we handle the mouse pointer?

Other relevant code: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1812

Comment 4

[Ben Combee (:bcombee)]

debugged into the C++ code, we're only getting keypress events; the code wants keyup to toggle state.  Not sure why keyup isn't being sent to browser.

Comment 5

[Ben Combee (:bcombee)]

OK, I'm seeing WM_KEYDOWN come through, but no WM_KEYUP events.  This is really odd.

Comment 6

[Ben Combee (:bcombee)]

last comment was wrong, that's a user error.

Comment 7

[Ben Combee (:bcombee)]

OK, I actually see the keyup generate the call into the DOM and modify the checkbox state now.  I don't see this updated on the screen, and even if I pan or refresh after it, I don't see it change.  This is bizarre.

Comment 8

[Antonio Gomes (tonikitoo)]

(In reply to comment #7)
> OK, I actually see the keyup generate the call into the DOM and modify the
> checkbox state now.  I don't see this updated on the screen, and even if I pan
> or refresh after it, I don't see it change.  This is bizarre.

combee should a mozafterpaint event get fired here ?

Comment 9

[Ben Combee (:bcombee)]

it's actually two clicks being generated.  We don't repaint immediately, so you don't see the check get set then unset.

Comment 10

[Ben Combee (:bcombee)]

18:17 < bcombee> ah, I think I figured out the checkbox thing
18:17 < bcombee> the code for key presses turns the space bar press into a mouse
                 click
18:18 < bcombee> since we don't expect that click, we end up cancelling it after
                 it's changed the value
18:18 < bcombee> that causes the second call to reverse what happened before
18:19 < bcombee> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1710
18:20 < bcombee> I think we need to avoid cancelling mouse clicks not sent to
                 canvas or chrome we care about
18:20 < bcombee> this probably needs the input handler rewrite to fix

Comment 11

[Ben Combee (:bcombee)]

Attachment: Rework click supression to be opt-in. Verified that checkboxes work now, also verified that chrome scrolling doesn't send unwanted clicks.

Comment 12

[Ben Combee (:bcombee)]

Comment on attachment 386341 [details] [diff] [review]
Rework click supression to be opt-in. Verified that checkboxes work now, also verified that chrome scrolling doesn't send unwanted clicks.

Cancelling review -- this patch regresses bug 496879

Comment 13

[Ben Combee (:bcombee)]

Attachment: Try #2 - this one doesn't regress other fixes.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 386352 [details] [diff] [review]
Try #2 - this one doesn't regress other fixes.

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js
>   _onMouseUp: function _onMouseUp(aEvent) {
>     // only process if original mousedown was on a scrollable element
>     if (!this._targetScrollbox) {
>-      this._owner.allowNextClick();
>       return;
>     }

Drop the { } ?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/b8dc31a6f725

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in my own OS X build from 8/11/2009.