685464 - DOMMenuItemActive is not consistent for menulist items

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[alexander :surkov (:asurkov)]

steps to reproduce:

1) tab to menulist
2) down arrow to select second menuitem
3) alt+down arrow to show menupopup
result: DOMMenuItemActive event is not fired for second item

4) escape menupopup
5) alt+down arrow to show menupopup
result: DOMMenuItemActive event is fired for second item

DOMMenuItemActive event should be fired in both cases (that makes it closer to HTML selects).

On menulist binding keypress activeChild is changed (as result of processing by menuBoxObject). On the another hand menulist binding has popupshowing event handler where it sets menuBoxObject.activeChild what causes events if active child is changed. So since activeChild is not changed then no events.

This bug is important for focus handling in accessibility.

Comment 1

[[e-mail]]

Attachment: Possible patch

Does this help?

Comment 2

[[e-mail]]

Attachment: Alternate approach

This version avoids clearing the active child when we don't need to. (Clearing the active child when it is already clear doesn't do anything significant.)

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 559074 [details] [diff] [review]
Possible patch

yes, thank you, Neil! This may cause extra DOMMenuItemInactive event but accessibility doesn't depend on it and if it is then I don't know whether this good or bad.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 559075 [details] [diff] [review]
Alternate approach

this works too (both menulist and editable menulist)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #3)
> Comment on attachment 559074 [details] [diff] [review]
> Possible patch
> 
> yes, thank you, Neil! This may cause extra DOMMenuItemInactive event but
> accessibility doesn't depend on it and if it is then I don't know whether
> this good or bad.

to avoid potential confusion: "and if it is" means "if there's extra DOMMenuItemInactive".

Comment 6

[[e-mail]]

Comment on attachment 559075 [details] [diff] [review]
Alternate approach

I guess it's down to Enn as to which one he likes best.

Comment 7

[Neil Deakin]

Comment on attachment 559075 [details] [diff] [review]
Alternate approach

>diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml
>--- a/toolkit/content/widgets/menulist.xml
>+++ b/toolkit/content/widgets/menulist.xml
>@@ -28,10 +28,11 @@
>       <handler event="command" phase="capturing"
>         action="if (event.target.parentNode.parentNode == this) this.selectedItem = event.target;"/>
> 
>       <handler event="popupshowing">
>         <![CDATA[
>+          this.menuBoxObject.activeChild = null;
>           if (event.target.parentNode == this && this.selectedItem)

Don't you want to put this inside the 'event.target.parentNode == this' check, as you've done with the editable menulist?

Comment 8

[[e-mail]]

(In reply to Neil Deakin from comment #7)
> Don't you want to put this inside the 'event.target.parentNode == this'
> check, as you've done with the editable menulist?
Indeed. I'll update the patch.

Comment 9

[[e-mail]]

Attachment: Updated for review comments

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 559536 [details] [diff] [review]
Updated for review comments

f=me, thank you!

Comment 11

[alexander :surkov (:asurkov)]

try server - https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=564adf750436

Comment 12

[alexander :surkov (:asurkov)]

landed on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/01256254361b

Comment 13

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/01256254361b