387981 - No focus events when exiting XUL submenus effective 5th July trunk build

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Accerciser and either Thunderbird or Firefox
2. Turn event monitoring on in Accerciser
3. In Thunderbird or Firefox:
    a. Press Alt V for the View menu (focus should be on Toolbars)
    b. Press Right Arrow to open the Toolbars menu
    c. Press Escape once to close the Toolbars menu

Expected results:  A focus: event and a state-changed:focused event would each be issued for the Toolbars menu.

Actual results:  Neither of these events is issued.

This seems to be a regression introduced in the 5th July trunk build.  I'm not sure if this a dup of bug 387496 or just another similar regression, so I'm filing it separately "just in case."

Comment 1

[Ginn Chen]

Joanie, I could not reproduce this bug with latest trunk.

Feel free to reopen if it still occurs on your machine.

Comment 2

[Joanmarie Diggs]

The problem still exists for me with the latest trunk on 3 different machines (2 "Gutsy", 1 "Feisty").  Are you getting new focus events *after step 3c*?  After step 3b we should see a focus: event and an object:state-changed:focused event for the first item in Toolbars like this:

focus(0, 0, None)
	source: [check menu item | Navigation Toolbar]
	application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [check menu item | Navigation Toolbar]
	application: [application | Minefield]

Those I get.  After step 3c we should see something similar to indicate that focus has been returned to the Toolbars menu just like we got after step 3a, like:

focus(0, 0, None)
	source: [menu item | Toolbars]
	application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [menu item | Toolbars]
	application: [application | Minefield]

Well, ideally it would be "menu" and not "menu item".  That's another bug I need to file. :-)  Regardless.... I get those events after step 3a (as expected), but NOT after step 3c (as needed).  As far as ATs are concerned, we're still on the Navigation Toolbar check menu item.

Reopening.

Comment 3

[Ginn Chen]

reproduced.

sorry, I misunderstood the description.

Comment 4

[Ginn Chen]

It's caused by the rewrite the menu popup.
File: nsMenuFrame.cpp

I don't fully understand this file, here's my understanding.

When PopupOpened() being called, the current meunitem in nsMenuParent is set.
So when PopupClosed(), it doesn't need to do anything for nsMenuParent unless we're closing all the menu.

From a11y's view, we want a DOMMenuItemActive, if we're closing the popup and go back to parent menu.
We could send such an event in PopupClosed(PR_FALSE), but we don't know if the parent menu is also closing. 
e.g. Alt+V to View, Right arrow to Navigation Toolbar, and press Right arrow again.
We don't like a focus event to "View" menu, then a focus event to "History".
And we didn't do that before 4th July.

I don't know how to do it yet.

Comment 5

[Ginn Chen]

Attachment: patch

Move nsMenuActivateEvent before nsMenuFrame, so we can use it in nsMenuFrame::PopupClosed()

Add an else-clause to PopupClosed(), I'm not sure if it is correct.
It works for me.

Comment 6

[Aaron Leventhal]

Comment on attachment 273567 [details] [diff] [review]
patch

Sorry, I'm busy for the next few days. Surkov, can you review?

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 273567 [details] [diff] [review]
patch

I'm not sure how much this is correct but since it fixes ally issue of the bug then my r+.

I have one suggestion. Probably it;s better to move PopupClosed() instead nsMenuActivateEvent. It will bring less cvs changes.

I think Enn should review this approach.

Comment 8

[Ginn Chen]

surkov, I agree it will bring less cvs change.
Actually, my first approach in my local space is to move PopupClosed()

But I don't like the idea of put "class nsMenuActivateEvent" between nsMenuFrame methods.
So I decide to move nsMenuActivateEvent.

Anyway, I'd like to change if it's recommended.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> surkov, I agree it will bring less cvs change.
> Actually, my first approach in my local space is to move PopupClosed()
> 
> But I don't like the idea of put "class nsMenuActivateEvent" between
> nsMenuFrame methods.
> So I decide to move nsMenuActivateEvent.

Agree but now it is defined between nsMenuFrame::PopupClosed and nsMenuFrame::SelectMenu. So we can save this approach. But it's up to Enn and Neil I guess.

Comment 10

[Neil Deakin]

Comment on attachment 273567 [details] [diff] [review]
patch

Yes, this looks like the right patch to me.

>+      // if we're re-activating the highlight menuitem in parent menu
>+      // send out a DOMMenuItemActive for accessibility

Change the comment to read:

// We are not deselecting the parent menu while closing the popup, so send
// a DOMMenuItemActive event to the menu to indicate that the menu is
// becoming active again.

>+      nsMenuFrame *current = mMenuParent->GetCurrentMenuItem();
>+      if (current) {
>+        nsCOMPtr<nsIRunnable> event =
>+          new nsMenuActivateEvent(current->mContent, PresContext(), PR_TRUE);
>+        NS_DispatchToCurrentThread(event);
>+      }

I think using GetContent() is clearer than mContent since it isn't 'this'.

I generally prefer to have braces around the if-block as well.

Comment 11

[Ginn Chen]

committed, thanks all!