381426 - Can't be activated Input Method in the Bookmark Properties.

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Hideo Oshima]

Steps to reroduce:
1. Open Bookmarks Manager.
2. Select any bookmark.
3. Open Bookmarks Proerties dialog from context menu.
4. Type a key to be activated Input Method.

Actual result:
Can't be activated IM.

Expected result:
Can be activate IM.

More Information:
If you open Bookmark Propertiese dialog by Properties button
on the toolbar of Bookmarks Manager, there is no problem.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070521 Minefield/3.0a5pre

Comment 1

[Hideo Oshima]

I made a mistake.

Correct steps to rerpoduce:
1. Open Bookmarks Manager.
2. Select any bookmark.
3. Open Bookmarks Proerties dialog from context menu.
4. Click "Name" field.
5. Type a key to be activated Input Method.

Comment 2

[baffclan]

Reproduce on Firefox-trunk/WinXP

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070521 Minefield/3.0a5pre

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

probably, this is a regression of bug 378752.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0

I think that we need to notify to finish the menu loop during the menus hiding.
Because if a modal dialog is created at menu executing, the RemoveKeyboardNavigator is called after the modal loop.

Note that we used the same hacking for nsMenuDismissalListener...

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we should call nsContentUtils::NotifyInstalledMenuKeyboardListener from nsMenuDismissalListener::Shutdown()?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I think that you are right excepted 1st of |nsMenuBarFrame::Escape| (I think that this code is not correct, we should remove it).

For semantics, should we rename |NotifyInstalledMenuKeyboardListener|? The actual kb listener is not removed.

Or, should we call |nsIMenuParent::RemoveKeyboardListener| in |nsMenuDismissalListener::Shutdown()|??

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> I think that you are right excepted 1st of |nsMenuBarFrame::Escape| (I think
> that this code is not correct, we should remove it).

I agree.

> For semantics, should we rename |NotifyInstalledMenuKeyboardListener|? The
> actual kb listener is not removed.

OK ... maybe we don't need to track menu status via NotifyInstalledMenuKeyboardListener? Can we just query to see if nsMenuDismissalListener is active?

> Or, should we call |nsIMenuParent::RemoveKeyboardListener| in
> |nsMenuDismissalListener::Shutdown()|??

Let's just do this for now.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv2.0

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #7)
> > For semantics, should we rename |NotifyInstalledMenuKeyboardListener|? The
> > actual kb listener is not removed.
> 
> OK ... maybe we don't need to track menu status via
> NotifyInstalledMenuKeyboardListener? Can we just query to see if
> nsMenuDismissalListener is active?

No. nsMenuDismissalListener is not only for 'Menu'. It is used for autocomplete too.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I got a feedback, the patch doesn't work fine on sub folder's items of bookmarks toolbar. But it works fine on Bookmarks menu of menubar.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Additional Patch rv1.0

The additional patch.

The kb navigator installed menu parent doesn't related the context menu on submenu items on bookmark toolbar. I think that nsMenuDismissalListener should close *all* menus.

So, this patch is 'adhoc', we need to clean-up nsMenuDismissalListener...

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

see also bug 381689.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is really Neil's department...

Comment 15

[Neil Deakin]

nsMenuDismissalListener will be removed by bug 279703 so any code cleanup shouldn't be done. The patch there fixes the open menu chain to be handled properly.
  

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Neil, thank you for the comment.

We should take the current patch for 1.9a5, bug 279703 is not landed before a5, it's critical for us.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In the patch, what happens if mFirstParent is destroyed while we're in the other menu? We're left with a dangling frame reference, it seems

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

when the first parent frame is destroying, should recreate the nsMenuDismissalListener?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Additional patch rv2.0

more adhoc but safety patch. I'm testing...

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 265829 [details] [diff] [review]
Additional patch rv2.0

let's land this.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in.