685191 - DOMMenuItemActive is fired for submenu parent when other menu is activated

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[alexander :surkov (:asurkov)]

steps to reproduce:

1) alt + v to select View -> Toolbars
DOMMenuItemActive events for view-menu and viewToolbarsMenu (correct)
2) right arrow to select View -> Toolbars -> Menubar
DOMMenuItemActive events for toggle_toolbar-menubar (correct)
3) right arrow to select History -> Back
DOMMenuItemActive events for history-menu, viewToolbarsMenu (incorrect), historyMenuBack

This event is quite confusing since happens between events for 'History' and 'History -> Back'.

DOMMenuItemActive event is used to fire accessible focus event what is announced by screen readers. Since basically DOMMenuItemActive events are asynchronous then this should lead to intermittent noisy screen reader's output.

Comment 1

[alexander :surkov (:asurkov)]

Enn, can you take a look at this issue as well since you're around menu a11y focus problems anyway? :)

These bugs aren't visible normally but they hit screen reader users.

Comment 2

[Neil Deakin]

Attachment: Patch to skip extra DOMMenuItemActive event

Comment 3

[alexander :surkov (:asurkov)]

There's no exess DOMMenuItemActive. However some events are not in sync for example DOMMenu(Item)Inactive are fired after DOMMenuItemActive, some are fired before it. I don't know however whether this makes a problem for AT. See log below for details.

Example:

<menu id="fruit" label="Fruit">
          <menupopup>
            <menuitem id="apple" label="Apple"/>
            <menuitem id="orange" label="Orange" disabled="true"/>
          </menupopup>
        </menu>
        <menu id="vehicle" label="Vehicle">
          <menupopup>
            <menu id="cycle" label="cycle">
              <menupopup>
                <menuitem id="tricycle" label="tricycle"/>
              </menupopup>
            </menu>
            <menuitem id="car" label="Car" disabled="true"/>
          </menupopup>
        </menu>

Test: right key on 'tricycle' menu item.

Event queue:
  invoke:  'vehicle'  'right ' key
TEST-INFO | unknown test url | Invoke the ' 'vehicle'  'right ' key' test { expected 'focus' event; unexpected 'focus' event;  }

A11Y DOMEvents: event 'DOMMenuItemInactive' handled; 24:44.848
  {
    Target: 0C7F7090; role: parent menuitem, name: 'Vehicle';
    Target node: 0A879E38, menu@id='vehicle', idx in parent: 1
  }

A11Y DOMEvents: event 'DOMMenuItemActive' handled; 24:44.848
  {
    Target: 0C7F7000; role: parent menuitem, name: 'Fruit';
    Target node: 0A87A8B8, menu@id='fruit', idx in parent: 0
  }

A11Y FOCUS: active item changed; 24:44.848
  {
    Item: 0C7F7000; role: parent menuitem, name: 'Fruit';
    Item node: 0A87A8B8, menu@id='fruit', idx in parent: 0
  }
  {
    Caused by: DOMMenuItemActive
    Item: 0C7F7000; role: parent menuitem, name: 'Fruit';
    Item node: 0A87A8B8, menu@id='fruit', idx in parent: 0
  }

A11Y DOMEvents: event 'popuphiding' handled; 24:44.863
  {
    Target: 03709AD0; role: menupopup, name: 'cycle';
    Target node: 0A87ABB8, menupopup@id='', idx in parent: 0
  }

A11Y DOMEvents: event 'popuphiding' handled; 24:44.863
  {
    Target: 0AA638C0; role: menupopup, name: 'Vehicle';
    Target node: 0A87A638, menupopup@id='', idx in parent: 0
  }

A11Y DOMEvents: event 'DOMMenuItemInactive' handled; 24:44.957
  {
    Target: 05707A68; role: menuitem, name: 'tricycle';
    Target node: 0A87A6B8, menuitem@id='tricycle', idx in parent: 0
  }

A11Y DOMEvents: event 'DOMMenuInactive' handled; 24:44.957
  {
    Target: 03709AD0; role: menupopup, name: 'cycle';
    Target node: 0A87ABB8, menupopup@id='', idx in parent: 0
  }

Event type: menupopup end. Target: ['menupopup node', address: [object XULElement], role: menupopup, name: 'cycle', address: 0x3709ad0]

A11Y FOCUS: fire focus event; 24:44.973
  {
    Target: 0C7F7000; role: parent menuitem, name: 'Fruit';
    Target node: 0A87A8B8, menu@id='fruit', idx in parent: 0
  }

Event type: focus. Target: ['menu@id="fruit" node', address: [object XULElement], role: parent menuitem, name: 'Fruit', address: 0xc7f7000]. Listeners count: 1

A11Y DOMEvents: event 'DOMMenuItemInactive' handled; 24:44.988
  {
    Target: 05707678; role: parent menuitem, name: 'cycle';
    Target node: 0A87A038, menu@id='cycle', idx in parent: 0
  }

A11Y DOMEvents: event 'DOMMenuInactive' handled; 24:44.988
  {
    Target: 0AA638C0; role: menupopup, name: 'Vehicle';
    Target node: 0A87A638, menupopup@id='', idx in parent: 0
  }

Event type: menupopup end. Target: ['menupopup node', address: [object XULElement], role: menupopup, name: 'Vehicle', address: 0xaa638c0]

A11Y DOMEvents: event 'DOMMenuItemActive' handled; 24:44.988
  {
    Target: 057074C8; role: menuitem, name: 'Apple';
    Target node: 0A87A9B8, menuitem@id='apple', idx in parent: 0
  }

A11Y FOCUS: active item changed; 24:44.988
  {
    Item: 057074C8; role: menuitem, name: 'Apple';
    Item node: 0A87A9B8, menuitem@id='apple', idx in parent: 0
  }
  {
    Caused by: DOMMenuItemActive
    Item: 057074C8; role: menuitem, name: 'Apple';
    Item node: 0A87A9B8, menuitem@id='apple', idx in parent: 0
    Document: 0AC773A8, document node: 07CDEE20
    Document uri: chrome://mochitests/content/a11y/accessible/events/test_focus_menu.xul
  }

A11Y DOMEvents: event 'popupshown' handled; 24:45.035
  {
    Target: 0AA63660; role: menupopup, name: 'Fruit';
    Target node: 0A87A7B8, menupopup@id='', idx in parent: 0
  }

Event type: menupopup start. Target: ['menupopup node', address: [object XULElement], role: menupopup, name: 'Fruit', address: 0xaa63660]

A11Y FOCUS: fire focus event; 24:45.160
  {
    Target: 057074C8; role: menuitem, name: 'Apple';
    Target node: 0A87A9B8, menuitem@id='apple', idx in parent: 0
  }

Event type: focus. Target: ['menuitem@id="apple" node', address: [object XULElement], role: menuitem, name: 'Apple', address: 0x57074c8]. Listeners count: 1

*****
EQ matched: focus

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 675085 [details] [diff] [review]
Patch to skip extra DOMMenuItemActive event

f=me per comment #3. Thank you!

Comment 5

[[e-mail]]

Sorry for the delay, but I wanted to try to understand what was going on. The bit that confuses me is when the menubar navigates from one top-level menu to another. Why doesn't it pass true for aDeactivateMenu when hiding the previous menu's popup?

Comment 6

[[e-mail]]

Sorry for the delay, but I wanted to try to understand what was going on. The bit that confuses me is when the menubar navigates from one top-level menu to another. Why doesn't it pass true for aDeactivateMenu when hiding the previous menu's popup?

Comment 7

[Neil Deakin]

(In reply to [e-mail] from comment #6)
> Sorry for the delay, but I wanted to try to understand what was going on.
> The bit that confuses me is when the menubar navigates from one top-level
> menu to another. Why doesn't it pass true for aDeactivateMenu when hiding
> the previous menu's popup?

nsMenuBarFrame::ChangeMenuItem already deselects the old menu and selects the new one.

Comment 8

[[e-mail]]

(In reply to Neil Deakin from comment #7)
> (In reply to comment #6)
> > Sorry for the delay, but I wanted to try to understand what was going on.
> > The bit that confuses me is when the menubar navigates from one top-level
> > menu to another. Why doesn't it pass true for aDeactivateMenu when hiding
> > the previous menu's popup?
> 
> nsMenuBarFrame::ChangeMenuItem already deselects the old menu and selects
> the new one.

Ah, right, so you would get two DOMMenuItemInactive events instead.

Comment 9

[[e-mail]]

(In reply to from comment #8)
> (In reply to Neil Deakin from comment #7)
> > (In reply to comment #6)
> > > Sorry for the delay, but I wanted to try to understand what was going on.
> > > The bit that confuses me is when the menubar navigates from one top-level
> > > menu to another. Why doesn't it pass true for aDeactivateMenu when hiding
> > > the previous menu's popup?
> > 
> > nsMenuBarFrame::ChangeMenuItem already deselects the old menu and selects
> > the new one.
> 
> Ah, right, so you would get two DOMMenuItemInactive events instead.

Interestingly you already get two DOMMenuItemInactive events from closing a menubar (because MenuClosed() calls SelectMenu(false) too) but that might be a bug in MenuClosed() (the only other caller is the popup manager but only if there is no popup to close).

Comment 10

[[e-mail]]

Comment on attachment 675085 [details] [diff] [review]
Patch to skip extra DOMMenuItemActive event

r=me if you improve the comment by mentioning that nsMenuBarFrame::ChangeMenuItem already deselects the old menu before closing the popup which is why it doesn't bother deselecting the menu again which is why we have to check that we're changing menu item before firing the active event. (If it didn't then none of this would have to happen, but I'm not going to get drawn into a long discussion of why nsMenuBarFrame::ChangeMenuItem feels it needs to deselect the old menu before closing the popup.)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bcb93c79fff