Closed Bug 222297 Opened 21 years ago Closed 20 years ago

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

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jepler, Assigned: aaronlev)

References

()

Details

(Keywords: access, html4, testcase)

Attachments

(2 files, 4 obsolete files)

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)
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
Attached file testcase
OS->All
OS: Linux → All
*** Bug 215899 has been marked as a duplicate of this bug. ***
Blocks: atfmeta
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?
Attached patch suggested patch (obsolete) — Splinter Review
seems work for the test case. any possible to break other case?
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.
Attached patch patch v2 (obsolete) — Splinter Review
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)
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.
Attached patch patch v3 (obsolete) — Splinter Review
variable declaration modification accroding Aaron's suggesiton
Attachment #150905 - Attachment is obsolete: true
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)
Attached patch patch v4 (obsolete) — Splinter Review
shoot, forgot to delete the old declaration
Attachment #151328 - Attachment is obsolete: true
Attachment #151331 - Flags: review?(aaronleventhal)
Comment on attachment 151331 [details] [diff] [review]
patch v4

Great. Thanks Neo.
Attachment #151331 - Flags: review?(aaronleventhal) → review+
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-
Attached patch final patchSplinter Review
new patch according to dbaron's suggestion
Attachment #151331 - Attachment is obsolete: true
Attachment #151660 - Flags: review?(aaronleventhal)
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.
Attachment #151660 - Flags: review?(aaronleventhal) → review+
checked in trunk
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: