Cannot toggle input checkbox via accesskey without taking focus off of input field first

RESOLVED FIXED

Status

()

Core
Keyboard: Navigation
P5
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Jeff Epler, Assigned: Aaron Leventhal)

Tracking

({access, html4, testcase})

Trunk
x86
All
access, html4, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

In standard applications (eg gtk, win32), repeatedly using the accesskey for a
checkbutton control toggles the control (just like repeatedly clicking the
associated label does).  In Mozilla, the first accesskey press focuses and
toggles, but subsequent presses do nothing until focus is moved away (for
instance, by using the accesskey for another field, tabbing out, or clicking
elsewhere)

Reproducible: Always

Steps to Reproduce:
1.Go to the demonstration URL
2.Press <modifer>-T twice (Alt-T on Linux)

Actual Results:  
The checkbox is in the "on" state

Expected Results:  
The checkbox should have been set to the "off" state after the second press.
(This is the behavior of IE5.5 on Windows)

Comment 1

15 years ago
Confirming new, 20031012 PC/WinXP

Will leave a testcase attached.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: access, html4, testcase
Summary: LABEL ACCESSKEY should always toggle an INPUT TYPE="CHECKBOX" → Cannot toggle input checkbox via accesskey without taking focus off of input field first

Comment 2

15 years ago
Created attachment 133361 [details]
testcase

Comment 3

15 years ago
OS->All
OS: Linux → All

Comment 4

15 years ago
*** Bug 215899 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Blocks: 127812
(Assignee)

Updated

14 years ago
Priority: -- → P5
the root cause is that when nsHTMLLabelElement::HandleDOMEvent is called by
nsEventStateManager::HandleAccessKeyEvent, the condition clause 
   if (content && !EventTargetIn(aPresContext, aEvent, content, this))
returns false which makes event NS_MOUSE_LEFT_CLICK not send to corresponding
element.

according to Blame info, this clause is to "Fix checking of checkboxes inside
their label by not sending the duplicate event if the click was already within
the form control" and submitted by David Baron.

David, could you make an explanation in detail here?
Created attachment 150793 [details] [diff] [review]
suggested patch

seems work for the test case. any possible to break other case?
(Assignee)

Comment 7

14 years ago
Comment on attachment 150793 [details] [diff] [review]
suggested patch

Makes sense to me.
Attachment #150793 - Flags: superreview?(dbaron)
Attachment #150793 - Flags: review+
It doesn't make sense to me.  EventTargetIn should be returning false in this
case (for the event dispatched by nsEventStateManager::HandleAccessKey), since
the event target should be the label, which is not inside or equal to the input.
... of course, the real problem is that
nsEventStateManager::GetEventTargetContent is returning the wrong thing since
HandleAccessKey dispatches the fake click event in a way that doesn't set its
target properly (e.g., doesn't change mCurrentTargetContent or whatever it
should do).  Shouldn't it call DispatchMouseEvent rather than calling
aContent->HandleDOMEvent directly?
Attachment #150793 - Flags: superreview?(dbaron) → superreview-
Comment on attachment 150793 [details] [diff] [review]
suggested patch

(And did you test that this doesn't regress clicking the checkbox in attachment
84596 [details] on bug 96813, which is what the fix you're modifying was fixing?	It sure
seems like this would break that fix.)
(In reply to comment #9)
> should do).  Shouldn't it call DispatchMouseEvent rather than calling
> aContent->HandleDOMEvent directly?

The "it" in this sentence is nsEventStateManager::HandleAccessKey.
Although actually DispatchMouseEvent isn't quite right -- but you want something
similar.
Created attachment 150905 [details] [diff] [review]
patch v2

how about this one? when nsEventStateManager::HandleAccessKey is called, and
content is a label element. we change the content to the element which is
specified by attribute "for". this can avoid
nsHTMLLabelElement::HandleDOMEvent.

test result is OK for both test cases(David's and mine)
Attachment #150793 - Attachment is obsolete: true
Attachment #150905 - Flags: review?(aaronleventhal)
(Assignee)

Comment 14

14 years ago
Comment on attachment 150905 [details] [diff] [review]
patch v2

I see, this is not a bug if the accesskey is definited on the <input> element
-- only if it's on the <label>.

+ nsresult rv;
Mozilla style is to declare a variable right before you use it, unless there's
a good reason not too (such as several things in the same method will use it,
so declare it ahead of time).

You don't need to define this until right when you're going to use it, like:
nsresult rv = label->GetHtmlFor(eledId)

Anyway, you don't really need rv since you're checking eleId.IsEmpty()

+ nsCOMPtr<nsIDOMElement> element;
Again, this declaration can be delayed. You can do:
if (domDocument) {
  nsCOMPtr<nsIDOMElement> element;
  domDocument->GetElementById(...);
  if (element) {
    content = do_QueryInterface(element);
  }
}
Attachment #150905 - Flags: review?(aaronleventhal) → review-
Aaron,

for the case that accesskey is defined in input element, it currently works and
no  need to make any fix on that.

for the variable rv and element, yeah, i agree with you.
Created attachment 151328 [details] [diff] [review]
patch v3

variable declaration modification accroding Aaron's suggesiton
Attachment #150905 - Attachment is obsolete: true
(Assignee)

Comment 17

14 years ago
Comment on attachment 151328 [details] [diff] [review]
patch v3

Looks like 
+	   nsCOMPtr<nsIDOMElement> element;

is still too early. It can be declared right before |if (domDocument)|
Attachment #151328 - Flags: review-
Attachment #151328 - Flags: review?(aaronleventhal)
Created attachment 151331 [details] [diff] [review]
patch v4

shoot, forgot to delete the old declaration
Attachment #151328 - Attachment is obsolete: true
Attachment #151331 - Flags: review?(aaronleventhal)
(Assignee)

Comment 19

14 years ago
Comment on attachment 151331 [details] [diff] [review]
patch v4

Great. Thanks Neo.
Attachment #151331 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

14 years ago
Attachment #151328 - Flags: review?(aaronleventhal)
Attachment #151331 - Flags: superreview?(dbaron)
Comment on attachment 151331 [details] [diff] [review]
patch v4

This is just yet another workaround for the real problem I described in comment
9.  I'd rather not spread label logic all over the tree if we don't have to. 
What I described in comment 9 shouldn't be very hard to fix.
Comment on attachment 151331 [details] [diff] [review]
patch v4

In fact, just saving and restoring mCurrentTargetContent around the
HandleDOMEvent call should be sufficient -- and that would be a considerably
smaller chunk of added code than this is.
Attachment #151331 - Flags: superreview?(dbaron) → superreview-
Created attachment 151660 [details] [diff] [review]
final patch

new patch according to dbaron's suggestion
Attachment #151331 - Attachment is obsolete: true
Attachment #151660 - Flags: review?(aaronleventhal)
(Assignee)

Comment 23

14 years ago
Comment on attachment 151660 [details] [diff] [review]
final patch

Neo, what did you think of David's suggestion?	
> Shouldn't it call DispatchMouseEvent 
> rather than calling
> Content->HandleDOMEvent
> directly?
(In reply to comment #23)
> (From update of attachment 151660 [details] [diff] [review])
> Neo, what did you think of David's suggestion?	
> > Shouldn't it call DispatchMouseEvent 
> > rather than calling
> > Content->HandleDOMEvent
> > directly?

No, it shouldn't.  See comment 12, or, more importantly, DispatchMouseEvent itself.
(Assignee)

Updated

14 years ago
Attachment #151660 - Flags: review?(aaronleventhal) → review+

Comment 25

14 years ago
checked in trunk
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.