346529 - Pressing accesskey for checkbox should toggle it

Status: RESOLVED FIXED

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

Description

[Timwi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.5) Gecko/20060719 Firefox/1.5.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.5) Gecko/20060719 Firefox/1.5.0.5

See STR.

Reproducible: Always

Steps to Reproduce:
1. View attachment.
2. Press Alt+A.
Actual Results:  
Checkbox is focussed, but nothing else happens.

Expected Results:  
Checkbox should be toggled. That is standard behaviour in all other major UI environments.

Comment 1

[Timwi]

Attachment: Test case

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Changing "Component" from "Form Manager" -> "Keyboard Navigation" as it probably fits better in there.

Comment 3

[Adam Guthrie]

I always thought you used the spacebar to toggle checkboxes.

Comment 4

[Aaron Leventhal]

Accesskey should act like a click unless accessibility.accesskeycausesactivation is false.

Is this a regression?

Comment 5

[Wayne DeAngelo]

I see this reported problem with Bon Echo nightly build 20060731, Aaron is right this is a regression.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

The content access key modifier was changed to Shift+Alt intentionally in bug 340902. If I press Shift+Alt+A in a recent Bon Echo build, I get the same behavior that I get if I use Alt+A in 1.5.0.5 (checkbox gets the focus ring, but isn't checked).

If this bug is about Alt+A not focusing the checkbox, then it's invalid. If it's about Shift+Alt+A not checking the checkbox, then it could be valid, I guess, but it isn't a regression from 1.5.0.x.

Comment 7

[Aaron Leventhal]

It's a valid bug. Shift+Alt+A is focusing the checkbox but not toggling it. The change to shift may have caused the regression.

Comment 8

[Aaron Leventhal]

Actually, I'm not sure that it's a regression though -- the accesskey is not toggling the checkbox in Firefox 1.5 as I would expect.

Another odd thing that I noticed while playing with this: if you shift+click on the checkbox a few times it actually disappears.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Actually, I'm not sure that it's a regression though -- the accesskey is not
> toggling the checkbox in Firefox 1.5 as I would expect.

That's what I said in comment 6... :)

> Another odd thing that I noticed while playing with this: if you shift+click
> on the checkbox a few times it actually disappears.

Shift+Double click is an AdBlock feature to quickly remove unwanted elements from the page. Do you happen to have adblock installed?

Comment 10

[Aaron Leventhal]

Good call on the Adblock, yes :)

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Results, SeaMonkey trunk on Linux:
Shift-Alt-A:
INPUT(tester): focus
LABEL: click
INPUT(tester): click
INPUT(tester): change

left mouse-click on label:
LABEL: click
INPUT(tester): focus
INPUT(tester): click
INPUT(tester): change

left mouse-click on checkbox:
INPUT(tester): focus
INPUT(tester): click
INPUT(tester): change

Comment 12

[Mats Palmgren (inactive)]

This bug is not a recent regression:
works: 2005-01-20-06
broken: 2005-01-24-05 (broken by bug 146066)
broken: 2006-03-07-04
works: 2006-03-07-14 (most likely fixed by bug 234455)

Comment 13

[Mats Palmgren (inactive)]

Attachment: Like so?

With this patch I get the same results as trunk for Testcase #2.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Er...  Labels should be responsing to the system event group here (the default action of the label is to dispatch a click to the control).  Why is that not working?  Is the label never getting a click in the system event group or something?

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #14)
> Is the label never getting a click in the system event group or
> something?

Yes. This patch adds the missing call.

Additionally, I think the order of events is wrong (also on trunk).
With this patch we get what I think is the correct order:
LABEL: click
INPUT(tester): focus
INPUT(tester): click
INPUT(tester): change

This is the same order of events that we get for a mouse left-click
on the label.

If I leave "ChangeFocusWith()" unconditional but include the rest of the
patch, we get the same order of events as we currently have on trunk:
INPUT(tester): focus
LABEL: click
INPUT(tester): click
INPUT(tester): change

By skipping it we instead rely on this code to set the focus instead:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLLabelElement.cpp&rev=1.87.6.1&root=/cvsroot&mark=240-241#211

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 231845 [details] [diff] [review]
Patch rev. 2

Yeah, here we go.  This looks excellent!

Make the focus change on trunk too, right?

Comment 17

[Mats Palmgren (inactive)]

Attachment: ChangeFocusWith() hunk for trunk

Comment 18

[Mats Palmgren (inactive)]

Attachment: Trunk patch, rev. 2

Referring the right method in the comment too...

Comment 19

[Mike Schroepfer]

Comment on attachment 231845 [details] [diff] [review]
Patch rev. 2

a=schrep for drivers.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 231845 [details] [diff] [review]
Patch rev. 2

Checked in to MOZILLA_1_8_BRANCH at 2006-08-05 15:19 PDT

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 231849 [details] [diff] [review]
Trunk patch, rev. 2

Checked in to trunk at 2006-08-05 15:18 PDT.

Comment 22

[Mats Palmgren (inactive)]

-> FIXED