451015 - <panel> element should not be topmost window

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

spun off from bug 433340.

The auto-hide panel creates topmost window. However, if the <panel> has one or more input fields, many IMEs of Windows are not usable (see this screenshot: http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3771 ). Therefore, mainly, for the add-on developers who are not using IME, the auto-hide panel should not be normal window rather than topmost window.

But some <panel> should be topmost window. E.g., the popup window for tab switching of Fx3.1. Therefore, we need to create "topmost" attribute for them.

# in bug 433340, the bookmark dialog is not topmost window now by the adhoc approach, see bug 433340 comment 100. And the patch of bug 433340 should be backed out from trunk.

Comment 1

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

Oops, I forgot an important issue. On Linux, when the non-topmost window closed, the focus doesn't return to the parent window (I confirmed on Fedora9). It's too bad. That is a cause of the mochitest failure in bug 433340 comment 45 and bug 433340 comment 47. We need to fix the issue, first.

Comment 2

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

I gave up to fix this bug with first idea. Because the focus problem of GTK2 cannot be fixed by us, probably. It should be a problem of some Window Managers. On GTK2 reference document, there are only two window types.
1. Normal toplevel window. This is used for normal windows.
2. Popup window. This is used for any popup windows, e.g., menus etc.
http://library.gnome.org/devel/gtk/stable/gtk-Standard-Enumerations.html#GtkWindowType

On GTK2, we are using the normal toplevel window for non-topmost panel, otherwise, i.e., for topmost panel, we are using The popup window. So, we *don't* specify the window level on GTK2 widget code. Probably, that means we cannot specify the window level without the types.

By this reason, we need different thinking on Win/Mac and Linux. On Windows and Mac, the instance of a panel element of both window levels are same normal window. However, on Linux, the instance of topmost panel and the instance of non-topmost panel are different behavior window.

So, you should note this point. I'm testing "Metacity window manager" (2.22.0-3.fc9) on Fedora9, it is default window manager of Fedora9. I can find the different behavior of Metacity for each window types. E.g., the normal window get focus at showing it. And the focus doesn't return to its parent at hiding. But the popup window doesn't get focus (is not activated), and by the behavior, the issue at hiding is gone.

The reason that we need to fix this bug is:
1. On Windows, the text editors should not be on topmost panels for IME users. (bug 448927)
2. On Mac, we *had* same reason, but the bug was fixed (bug 447635), however, we also have bug 404131.

So, on Win and Mac, we recommend that the default setting panel elements should be normal window level window.

However, bug 433340 was not reproduced on Linux. Probably, IME related windows should be popup window type for it don't steal the focus from client applications. So, by this reason, we cannot find bug 433340 on Linux.

So, we *don't* need to change the default panel element behavior on Linux.

Therefore, I think we should use following way for fix this bug:

1. Add a new attribute that is named as "windowlevel".
2. If "windowlevel" is "topmost", the panel element should be topmost window level on all platforms.
3. If "windowlevel" is "normal", the panel element should be normal window level on all platforms.
4. Otherwise (including windowlevel attribute is not specified), the panel element should be the recommended window level that doesn't have the IME accessibility problems and other problems. So, on Win and Mac, it's normal window level. On Linux, it's topmost window level.

I'll post the patch with this approach.

Comment 3

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

Attachment: Patch v1.0

See comment 2 for the detail of this patch.

Comment 4

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

Attachment: Patch v1.0 for comm-central

Comment 5

[Neil Deakin]

Comment on attachment 335280 [details] [diff] [review]
Patch v1.0

>+  if (sGlobalVariablesInitialized)
>+    return;
>+  sDefaultWindowLevelIsTopmost = nsContentUtils::GetBoolPref(
>+    "accessibility.xul_panel.default_window_level_is_topmost", PR_FALSE);
>+  sGlobalVariablesInitialized = 1;

Would it be clearer to use a PRInt8 here and have three values rather than two separate fields?


>+PRBool
>+nsMenuPopupFrame::IsTopMost()
>+{
>+  // If this panel is not a panel, this is always a top-most popup
>+  if (mPopupType != ePopupTypePanel)
>+    return PR_TRUE;
>+
>+  // If this panel is a noautohide panel, it should appear just above the parent
>+  // window.
>+  // XXX Web pages should not be able to create a topmost panel. However, if we
>+  // stop to support that, some mochitests failed on Linux.
>+  if (IsNoAutoHide())
>+    return PR_FALSE;
>+
>+  // Otherwise, check the topmost attribute.
>+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::windowlevel,
>+                            nsGkAtoms::topmost, eIgnoreCase))
>+    return PR_TRUE;
>+
>+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::windowlevel,
>+                            nsGkAtoms::normal, eIgnoreCase))

I think just 'level' is a better name as it's only a window OS-wise, not from a XUL perspective.

'parent' is a better name, as 'normal' implies the default value.
Using 'top' rather than 'topmost' would eliminate the need for an extra atom.
  
>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>+// On GTK2 platform, we should use topmost window level for the default window
>+// level of <panel> element of XUL. GTK2 has only two window types. One is
>+// normal top level window, other is popup window. The popup window is always
>+// topmost window level, therefore, we are using normal top level window for
>+// non-topmost panel, but it is pretty hacky. On some Window Managers, we have
>+// 2 problems:
>+// 1. The non-topmost panel steals from its parent window at showing.

What does 'steals' mean here?

>+// 2. The parent of non-topmost panel is not activated at the panel being
>+//    hidden.

at the panel being hidden -> when the panel is hidden

>+// So, we have no reasones we should use non-toplevel window for popup.

reasones -> reasons

>+pref("accessibility.xul_panel.default_window_level_is_topmost", true);
>+#else
>+// Win32: See bug 448927, on topmost panel, some IMEs are not usable.
>+// Others: Should use non-topmost for IMEs (We don't confirm the IME problem,
>+//         however, it is too serious if there is. So, this is safer setting.)
>+pref("accessibility.xul_panel.default_window_level_is_topmost", false);
>+#endif
>+#else
>+// MacOSX: See bug 404131, topmost panel wins to Dashboard.
>+pref("accessibility.xul_panel.default_window_level_is_topmost", false);
>+#endif

Which value do we want to use on other platforms? false or true? 'true' is the old default.
Is it possible to arrange the ifdefs so that you only need the false line once.
 
"ui.panel.default_level_parent" is a better name for the preference.

Comment 6

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

Attachment: Patch v2.0

Thank you, Deakin.

(In reply to comment #5)
> >diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
> >+// 1. The non-topmost panel steals from its parent window at showing.
> 
> What does 'steals' mean here?

Oops, I meant: "The non-topmost panel steals _focus_ from ...".

> Which value do we want to use on other platforms? false or true? 'true' is the
> old default.

I think that it should be false. But I'm not sure. There are two widget code for the "Others" which are OS/2 and BeOS. BeOS cannot be built now, maybe. OS/2 can be built. I'm not familiar to OS/2 behavior. But looks like that OS/2 behavior is similar to Win32. If the window system is also similar to Win32's, some OS/2 IMEs may have the bug. So, I think they should be false in this bug, and if we will get the bug report of this change, we should think whether we should change the value to true.

> Is it possible to arrange the ifdefs so that you only need the false line once.

I think that it's impossible. |#if defined()| cannot be used here. And some other points uses such ugly #ifdefs. However, now I move the settings to each platform's parts in all.js. I think that this is better for "other" platforms.

Comment 7

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

Attachment: Patch v2.0 for comm-central

Comment 8

[Neil Deakin]

Comment on attachment 335405 [details] [diff] [review]
Patch v2.0

>+  // If this panel is a noautohide panel, it should appear just above the parent
>+  // window.
>+  // XXX Web pages should not be able to create a topmost panel. However, if we
>+  // stop to support that, some mochitests failed on Linux.
>+  if (IsNoAutoHide())
>+    return PR_FALSE;

Not clear how the XXX comment here relates to this code.

>+  // Otherwise, the result depends on the platform.
>+  // XXX Should we reserve "default" or "auto" for this value?

No, we don't need to, so remove this comment.

Comment 9

[Neil Deakin]

Comment on attachment 335406 [details] [diff] [review]
Patch v2.0 for comm-central

I'm not actually a suite reviewer, but it looks ok.

Comment 10

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

Attachment: Patch v2.0.1

Thank you, Deakin. I also request additional review to browser and toolkit reviewer.

Comment 11

[Alfred Kayser]

Requesting blocking 1.9.0.3, as this very probably fixes the 'hang/blocker' bug 452385.

Comment 12

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

This patch should not be landed to any branches.

Comment 13

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

Gavin:

Would you review the patch?

Comment 14

[[e-mail]]

Comment on attachment 335406 [details] [diff] [review]
Patch v2.0 for comm-central

>-  <panel type="autocomplete" id="PopupAutoComplete"/>
>+  <panel type="autocomplete" level="top" id="PopupAutoComplete"/>
You added the level="top" to the end of all the other panels ;-)

Comment 15

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

checked-in.

Comment 16

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

oops.

-> FIXED

Comment 17

[Dão Gottwald [:dao]]

Firefox hangs when opening the panel from bug 436304, which lacks level="top" so far. Is this level of breakage expected?

Comment 18

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

(In reply to comment #17)
> Firefox hangs when opening the panel from bug 436304, which lacks level="top"
> so far. Is this level of breakage expected?

If the hanging is reproduced only on Win32, it may be bug 452385.

Comment 19

[aja+bugzilla]

(In reply to comment #18)
> (In reply to comment #17)
> > Firefox hangs when opening the panel from bug 436304, which lacks level="top"
> > so far. Is this level of breakage expected?
> 
> If the hanging is reproduced only on Win32, it may be bug 452385.

Bug 454404 is win-only, as far as I can tell...WFM on Ubuntu Hardy, hangs on XP/SP3.

Comment 20

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

Oh, when "ui.panel.default_level_parent" is true, a panel is topmost. Otherwise, it is parent. The meaning is inverted??

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/18a3ad252c7b
<panel> element should not be topmost window r=deakin+gavin, sr=neil