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

Status: RESOLVED FIXED

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

Description

[Jeff Epler]

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

[Bill Mason]

Confirming new, 20031012 PC/WinXP

Will leave a testcase attached.

Comment 2

[Bill Mason]

Attachment: testcase

Comment 3

[Bill Mason]

OS->All

Comment 4

[brewthatistrue]

*** Bug 215899 has been marked as a duplicate of this bug. ***

Comment 5

[Nian Liu(n/a in a long time)]

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

[Nian Liu(n/a in a long time)]

Attachment: suggested patch

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

Comment 7

[Aaron Leventhal]

Comment on attachment 150793 [details] [diff] [review]
suggested patch

Makes sense to me.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... 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?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although actually DispatchMouseEvent isn't quite right -- but you want something
similar.

Comment 13

[Nian Liu(n/a in a long time)]

Attachment: 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)

Comment 14

[Aaron Leventhal]

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);
  }
}

Comment 15

[Nian Liu(n/a in a long time)]

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

[Nian Liu(n/a in a long time)]

Attachment: patch v3

variable declaration modification accroding Aaron's suggesiton

Comment 17

[Aaron Leventhal]

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)|

Comment 18

[Nian Liu(n/a in a long time)]

Attachment: patch v4

shoot, forgot to delete the old declaration

Comment 19

[Aaron Leventhal]

Comment on attachment 151331 [details] [diff] [review]
patch v4

Great. Thanks Neo.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 22

[Nian Liu(n/a in a long time)]

Attachment: final patch

new patch according to dbaron's suggestion

Comment 23

[Aaron Leventhal]

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?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 25

[Robin Lu]

checked in trunk