Last Comment Bug 685464 - DOMMenuItemActive is not consistent for menulist items
: DOMMenuItemActive is not consistent for menulist items
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: Menus (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla9
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on:
Blocks: focuseventa11y
  Show dependency treegraph
 
Reported: 2011-09-08 01:32 PDT by alexander :surkov
Modified: 2011-09-11 19:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible patch (554 bytes, patch)
2011-09-08 01:37 PDT, neil@parkwaycc.co.uk
surkov.alexander: feedback+
Details | Diff | Splinter Review
Alternate approach (1.37 KB, patch)
2011-09-08 01:42 PDT, neil@parkwaycc.co.uk
enndeakin: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review
Updated for review comments (2.11 KB, patch)
2011-09-09 12:18 PDT, neil@parkwaycc.co.uk
enndeakin: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2011-09-08 01:32:10 PDT
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 neil@parkwaycc.co.uk 2011-09-08 01:37:36 PDT
Created attachment 559074 [details] [diff] [review]
Possible patch

Does this help?
Comment 2 neil@parkwaycc.co.uk 2011-09-08 01:42:06 PDT
Created attachment 559075 [details] [diff] [review]
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 2011-09-08 01:48:15 PDT
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 2011-09-08 01:53:03 PDT
Comment on attachment 559075 [details] [diff] [review]
Alternate approach

this works too (both menulist and editable menulist)
Comment 5 alexander :surkov 2011-09-08 01:54:12 PDT
(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 neil@parkwaycc.co.uk 2011-09-08 02:01:59 PDT
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 (away until Oct 3) 2011-09-09 05:33:52 PDT
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 neil@parkwaycc.co.uk 2011-09-09 06:12:58 PDT
(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 neil@parkwaycc.co.uk 2011-09-09 12:18:44 PDT
Created attachment 559536 [details] [diff] [review]
Updated for review comments
Comment 10 alexander :surkov 2011-09-09 19:14:33 PDT
Comment on attachment 559536 [details] [diff] [review]
Updated for review comments

f=me, thank you!
Comment 11 alexander :surkov 2011-09-09 20:01:35 PDT
try server - https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=564adf750436
Comment 12 alexander :surkov 2011-09-09 22:56:46 PDT
landed on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/01256254361b
Comment 13 Matt Brubeck (:mbrubeck) 2011-09-11 06:38:16 PDT
http://hg.mozilla.org/mozilla-central/rev/01256254361b

Note You need to log in before you can comment on or make changes to this bug.