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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jepler, Assigned: aaronlev)
References
()
Details
(Keywords: access, html4, testcase)
Attachments
(2 files, 4 obsolete files)
627 bytes,
text/html
|
Details | |
1.28 KB,
patch
|
aaronlev
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Confirming new, 20031012 PC/WinXP Will leave a testcase attached.
Comment 2•21 years ago
|
||
Comment 4•20 years ago
|
||
*** Bug 215899 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•20 years ago
|
Priority: -- → P5
Comment 5•20 years ago
|
||
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?
Comment 6•20 years ago
|
||
seems work for the test case. any possible to break other case?
Assignee | ||
Comment 7•20 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.
Comment 13•20 years ago
|
||
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)
Updated•20 years ago
|
Attachment #150793 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #150905 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 14•20 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-
Comment 15•20 years ago
|
||
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.
Comment 16•20 years ago
|
||
variable declaration modification accroding Aaron's suggesiton
Attachment #150905 -
Attachment is obsolete: true
Assignee | ||
Comment 17•20 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-
Updated•20 years ago
|
Attachment #151328 -
Flags: review?(aaronleventhal)
Comment 18•20 years ago
|
||
shoot, forgot to delete the old declaration
Attachment #151328 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #151331 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 19•20 years ago
|
||
Comment on attachment 151331 [details] [diff] [review] patch v4 Great. Thanks Neo.
Attachment #151331 -
Flags: review?(aaronleventhal) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #151328 -
Flags: review?(aaronleventhal)
Updated•20 years ago
|
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-
Comment 22•20 years ago
|
||
new patch according to dbaron's suggestion
Attachment #151331 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #151660 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 23•20 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.
Attachment #151660 -
Flags: superreview+
Assignee | ||
Updated•20 years ago
|
Attachment #151660 -
Flags: review?(aaronleventhal) → review+
Comment 25•20 years ago
|
||
checked in trunk
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•