Closed Bug 501135 Opened 13 years ago Closed 13 years ago

pressing space in a checkbox should toggle it

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: timeless, Assigned: bcombee)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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
that could be also the case of radiobuttons and normal buttons ... i.e. overall html input elements
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.
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
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.
OK, I'm seeing WM_KEYDOWN come through, but no WM_KEYUP events.  This is really odd.
last comment was wrong, that's a user error.
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.
(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 ?
it's actually two clicks being generated.  We don't repaint immediately, so you don't see the check get set then unset.
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
Assignee: nobody → combee
Status: NEW → ASSIGNED
Attachment #386341 - Flags: review?(mark.finkle)
Attachment #386341 - Flags: review?(mark.finkle) → review+
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
Attachment #386341 - Flags: review+
Attachment #386341 - Attachment is obsolete: true
Attachment #386352 - Flags: review?(mark.finkle)
Attachment #386352 - Flags: review?(mark.finkle) → review+
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 { } ?
http://hg.mozilla.org/mobile-browser/rev/b8dc31a6f725
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Verified fixed in my own OS X build from 8/11/2009.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.