DOMMenuItemActive is not consistent for menulist items

RESOLVED FIXED in mozilla9

Status

()

Core
XP Toolkit/Widgets: Menus
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla9
All
Windows 7
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.11 KB, patch
Neil Deakin (not available until Aug 9)
: review+
surkov
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 559074 [details] [diff] [review]
Possible patch

Does this help?
Attachment #559074 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 2

6 years ago
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.)
Attachment #559075 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 3

6 years ago
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.
Attachment #559074 - Flags: feedback?(surkov.alexander) → feedback+
(Reporter)

Comment 4

6 years ago
Comment on attachment 559075 [details] [diff] [review]
Alternate approach

this works too (both menulist and editable menulist)
Attachment #559075 - Flags: feedback?(surkov.alexander) → feedback+
(Reporter)

Comment 5

6 years ago
(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".
(Assignee)

Comment 6

6 years ago
Comment on attachment 559075 [details] [diff] [review]
Alternate approach

I guess it's down to Enn as to which one he likes best.
Attachment #559075 - Flags: review?(enndeakin)
(Assignee)

Updated

6 years ago
Assignee: nobody → neil
Status: NEW → ASSIGNED
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?
Attachment #559075 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 559536 [details] [diff] [review]
Updated for review comments
Attachment #559074 - Attachment is obsolete: true
Attachment #559075 - Attachment is obsolete: true
Attachment #559536 - Flags: review?(enndeakin)
Attachment #559536 - Flags: feedback?(surkov.alexander)
Attachment #559536 - Flags: review?(enndeakin) → review+
(Reporter)

Comment 10

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

f=me, thank you!
Attachment #559536 - Flags: feedback?(surkov.alexander) → feedback+
(Reporter)

Comment 11

6 years ago
try server - https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=564adf750436
(Reporter)

Comment 12

6 years ago
landed on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/01256254361b
http://hg.mozilla.org/mozilla-central/rev/01256254361b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

6 years ago
Summary: DOMMenuItemActive is not consistent menulist items → DOMMenuItemActive is not consistent for menulist items
You need to log in before you can comment on or make changes to this bug.