Closed Bug 412954 Opened 12 years ago Closed 12 years ago

menus should have Menu, PopupMenu or DropdownMenu window type

Categories

(Core :: Widget: Gtk, defect, trivial)

x86
Linux
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: u294409, Assigned: ventnor.bugzilla)

References

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011704 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011704 Minefield/3.0b3pre

when menu is opened, compiz animates it, if Animation plugin enabled.
menus can have other animations than normal windows, but all menus in firefox are animated as normal.

what does this mean? firefox doesn't set appropriate window type for menus.

Reproducible: Always

Steps to Reproduce:
1. run compiz
2. enable animations
3. select some effects for normal windows and menus
4. open some menu in firefox
Actual Results:  
we can see wrong animation (wrong window type)

Expected Results:  
Menu, PopupMenu or DropdownMenu window type for menus

Menu type is used for menus opening from menubar or combobox/such button

PopupMenus are opened with right mouse button

DropdownMenus are opened from other opened menu (like Send To in Windows, etc
There's workaround in Compiz, but it should be fixed here.
Agreed. Additionally the tooltip hint should be set for tooltips. Gtk already has some framework for setting the window type hint (gtk_window_set_type_hint).

Our workaround in Compiz Fusion unfortunately can't distinguish between Firefox' menus and tooltips, so properly setting the type hint would be much appreciated.
Unfortunately there is no way for us to tell between the different types of menus (to Mozilla, a menu is a menu is a menu) but I can add hints for tooltips, menus and utility popups.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #3)
> Unfortunately there is no way for us to tell between the different types of
> menus (to Mozilla, a menu is a menu is a menu) but I can add hints for
> tooltips, menus and utility popups.

As discussed on IRC: This sounds great and would be sufficient for probably 99% of all Compiz users. :)

Attached patch PatchSplinter Review
I was told on IRC how much Compiz would appreciate this to get rid of their Firefox-specific hacks. Its a small patch just to let us set the hint of the popup.

nsXulPopupManager depends on nsIWidget, so to avoid a circular dependency and compile errors I had to move the nsPopupType enum.
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #305729 - Flags: superreview?(roc)
Attachment #305729 - Flags: review?(roc)
Version: unspecified → Trunk
This patch works absolutely fine here.
Attachment #305729 - Flags: superreview?(roc)
Attachment #305729 - Flags: superreview+
Attachment #305729 - Flags: review?(roc)
Attachment #305729 - Flags: review+
Attachment #305729 - Flags: approval1.9?
Comment on attachment 305729 [details] [diff] [review]
Patch

Would love to get this into b4, as it fixes a long-standing problem with window managers not being able to differentiate the types of windows Firefox uses (menus, popup menus, etc.). This will allow window managers such as compiz to better theme the various types of windows.
Attachment #305729 - Flags: approval1.9b4?
Attachment #305729 - Flags: approval1.9b4?
Comment on attachment 305729 [details] [diff] [review]
Patch

a1.9=beltzner
Attachment #305729 - Flags: approval1.9? → approval1.9+
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.261; previous revision: 1.260
done
Checking in widget/public/nsIWidget.h;
/cvsroot/mozilla/widget/public/nsIWidget.h,v  <--  nsIWidget.h
new revision: 3.169; previous revision: 3.168
done
Checking in layout/xul/base/public/nsXULPopupManager.h;
/cvsroot/mozilla/layout/xul/base/public/nsXULPopupManager.h,v  <--  nsXULPopupManager.h
new revision: 1.29; previous revision: 1.28
done
Checking in layout/xul/base/src/nsMenuPopupFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp,v  <--  nsMenuPopupFrame.cpp
new revision: 1.338; previous revision: 1.337
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Comment on attachment 305729 [details] [diff] [review]
Patch

>-            // treat popups with a parent as top level windows
>-            if (mParent) {
>-                mShell = gtk_window_new(GTK_WINDOW_TOPLEVEL);
>-                gtk_window_set_wmclass(GTK_WINDOW(mShell), "Toplevel", cBrand.get());
>-                gtk_window_set_decorated(GTK_WINDOW(mShell), FALSE);
>-            }
>-            else {
>-                mShell = gtk_window_new(GTK_WINDOW_POPUP);
>-                gtk_window_set_wmclass(GTK_WINDOW(mShell), "Popup", cBrand.get());

Why did you intentionally remove code for bug 395334 and regress that bug on Linux?

Specifically, <panel noautohide="true"> should make a floating panel above its parent window, but not other windows. Now, it floats above all windows.
(In reply to comment #10)
> (From update of attachment 305729 [details] [diff] [review])
> >-            // treat popups with a parent as top level windows
> >-            if (mParent) {
> >-                mShell = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> >-                gtk_window_set_wmclass(GTK_WINDOW(mShell), "Toplevel", cBrand.get());
> >-                gtk_window_set_decorated(GTK_WINDOW(mShell), FALSE);
> >-            }
> >-            else {
> >-                mShell = gtk_window_new(GTK_WINDOW_POPUP);
> >-                gtk_window_set_wmclass(GTK_WINDOW(mShell), "Popup", cBrand.get());
> 
> Why did you intentionally remove code for bug 395334 and regress that bug on
> Linux?
> 
> Specifically, <panel noautohide="true"> should make a floating panel above its
> parent window, but not other windows. Now, it floats above all windows.

I think the comment explaining this code part wasn't that good ;)

From my POV, though, the mentioned code doesn't harm this bug (setting window types), so I think it can be added back (with a better comment perhaps?) without needing reopening this bug.
Oops sorry. Michael, please fix.
File a new bug for the patch, please?
Depends on: 422531
On beta 5 the list that opens below the location bar and lists of saved entries on form fields seems to behave as normal windows, and compiz applies them the default animation for window open/close.

Menus and context menu are working properly. Beta5 has changed anything about this from beta4?
To be more precise a beta 5 pre worked well:

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b5pre) Gecko/2008030704 Minefield/3.0b5pre

But the final beta is not:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5

Any clue? Should I open a diferent bug or reopen?
Guille: my compiz is acting strange as well, but only after I fiddled with the animation settings. Now the location bar list also animates in firefox 3 beta 3... some breakage in compiz? what compiz version are you running btw?
0.7.2
(In reply to comment #14)
> On beta 5 the list that opens below the location bar and lists of saved entries
> on form fields seems to behave as normal windows, and compiz applies them the
> default animation for window open/close.

That's only because the animation in pre-0.7.2 versions grouped the Utility window type used by Firefox for non-menu, non-tooltip popup windows together with Normal window animations.
Just assign a different animation for utility windows and you're done. Personally, I find Horizontal Folds is a nice effect for that.

I was told that unfortunately it's not possible within Firefox to distinguish between e.g. web site combo boxes and panel type popups, so Utility window type is used for all non-menu, non-tooltip popups, which probably is the best we can get from this situation.
But something changed in firefox between: Gecko/2008030704
Minefield/3.0b5pre and Gecko/2008032619 Firefox/3.0b5

On the same computer, same compiz, those versions behave differently...
(In reply to comment #19)
> But something changed in firefox between: Gecko/2008030704
> Minefield/3.0b5pre and Gecko/2008032619 Firefox/3.0b5
> 
> On the same computer, same compiz, those versions behave differently...

What exactly changed for you? A copy of Beta 5 I downloaded today behaves exactly as expected here: menus are of MENU type, tooltips are TOOLTIP type, all other windows have UTILITY type.

On Gecko/2008030704 Minefield/3.0b5pre the list of the (address bar/autocompletion in form fields) opens normaly, with the same compiz effect as normal menus.

On Gecko/2008032619 Firefox/3.0b5 the list of the (address bar/autocompletion in form fields) opens with effect compiz has for open/close windows (beam/fire in my case).

How could I check the window type assigned to the lists? I you tell me I could see whats the difference between the two firefox versions.
(In reply to comment #21)
> On Gecko/2008030704 Minefield/3.0b5pre the list of the (address
> bar/autocompletion in form fields) opens normaly, with the same compiz effect
> as normal menus.

That's probably the "Firefox menu fix" of the workarounds Compiz plugin. That one should be unchecked with FF3, and the default will be changed to "off" once FF3 is released.

> On Gecko/2008032619 Firefox/3.0b5 the list of the (address bar/autocompletion
> in form fields) opens with effect compiz has for open/close windows (beam/fire
> in my case).
> 
> How could I check the window type assigned to the lists? I you tell me I could
> see whats the difference between the two firefox versions.

Just check the animation plugin options and make sure that a type=Utility rule is there with the animation of your choice.
This is a Compiz configuration issue though, thus I suggest to continue either at the Compiz forums (http://forum.compiz-fusion.org) or on IRC (#compiz-fusion on Freenode).
Hmm... strange. With the (old) Firefox build from Ubuntu Hardy, which is

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008031317 Firefox/3.0b4

the location bar and normal menus all zoom in. The only zoom rule I have is this:

(type=Normal | Dialog | ModalDialog | Unknown) & !(name=gnome-screensaver)

I'm running compiz fusion git... but I'm quite sure even this old build worked fine until I tried to fix my animation settings yesterday. I was getting a waning about wrong open/close animation settings (turned out the config had somehow managed to append 0; to the config lines) and after this the problems started. But perhaps it's just that the matches where never done correctly with this error present (which could be for a long time)?
(In reply to comment #23)
> Hmm... strange. With the (old) Firefox build from Ubuntu Hardy, which is
> 
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008031317
> Firefox/3.0b4
> 
> the location bar and normal menus all zoom in. The only zoom rule I have is
> this:
> 
> (type=Normal | Dialog | ModalDialog | Unknown) & !(name=gnome-screensaver)

This is what this bug is about ;)
Previously, FF didn't set the window types, so all windows were of type NORMAL.
From Beta 5 on, window types are set as decribed above, with the result that type=Normal no longer matches them, which is correct behaviour.
(In reply to comment #22)
> Just check the animation plugin options and make sure that a type=Utility rule
> is there with the animation of your choice.

Removing type=Utility from the beam/fire effect I avoid this problem.

I still have to see if "utility" is used by other apps for which I want bean/fire. Utility is not supposed to be a normal window, is it?

Thanks!

> This is a Compiz configuration issue though, thus I suggest to continue either
> at the Compiz forums (http://forum.compiz-fusion.org) or on IRC (#compiz-fusion
> on Freenode).

I know, but I wanted to comment it here because it was a change of "type" between two releases, and I wasn't sure it was on purpose. :)
(In reply to comment #25)
> (In reply to comment #22)
> > Just check the animation plugin options and make sure that a type=Utility rule
> > is there with the animation of your choice.
> 
> Removing type=Utility from the beam/fire effect I avoid this problem.
> 
> I still have to see if "utility" is used by other apps for which I want
> bean/fire. Utility is not supposed to be a normal window, is it?
> 
> Thanks!
> 
> > This is a Compiz configuration issue though, thus I suggest to continue either
> > at the Compiz forums (http://forum.compiz-fusion.org) or on IRC (#compiz-fusion
> > on Freenode).
> 
> I know, but I wanted to comment it here because it was a change of "type"
> between two releases, and I wasn't sure it was on purpose. :)

As said above, this change is what this bug is about ;)
If any other app also uses the Utility window type for a different purpose, you can still distinguish those by class name.

Depends on: 426661
Apparently, Ubuntu Gutsy and Hardy have Compiz default settings that will make autocomplete popups open and close with the animation used for new windows. That default behavior can be pretty annoying.

Ubuntu Gutsy uses Compiz 0.5.2 and they patch the default animation settings (this is from http://archive.ubuntu.com/ubuntu/pool/main/c/compiz-fusion-plugins-main/compiz-fusion-plugins-main_0.5.2+git20070928-0ubuntu2.diff.gz):

-	      <value>(type=Normal | Dialog | ModalDialog | Utility | Unknown) &amp; !(name=gnome-screensaver)</value>
-	      <value>(type=Menu | PopupMenu | DropdownMenu)</value>
+	      <value>((type=Normal | Utility | Unknown) | name=sun-awt-X11-XFramePeer | name=sun-awt-X11-XDialogPeer) &amp; !(role=toolTipTip | role=qtooltip_label) &amp; !(type=Normal &amp; override_redirect=1) &amp; !(name=gnome-screensaver)</value>
+	      <value>(type=Menu | PopupMenu | DropdownMenu | Dialog | ModalDialog | Normal)</value>


For Ubuntu Hardy, Compiz 0.7.2 is used. They do something similar (from http://archive.ubuntu.com/ubuntu/pool/main/c/compiz-fusion-plugins-main/compiz-fusion-plugins-main_0.7.2-0ubuntu1.diff.gz):

-	      <value>(type=Normal | Dialog | ModalDialog | Unknown) &amp; !(name=gnome-screensaver)</value>
-	      <value>(type=Menu | PopupMenu | DropdownMenu)</value>
+	      <value>((type=Normal | Utility | Unknown) | name=sun-awt-X11-XFramePeer | name=sun-awt-X11-XDialogPeer) &amp; !(role=toolTipTip | role=qtooltip_label) &amp; !(type=Normal &amp; override_redirect=1) &amp; !(name=gnome-screensaver)</value>
+	      <value>(type=Menu | PopupMenu | DropdownMenu | Dialog | ModalDialog | Normal)</value>

I see that Compiz 0.7.2 no longer uses "Utility" in the match string for open/close zoom animation, but Ubuntu/Debian is still adding it in their patch. Maybe this is a bug in Ubuntu's patch?
You need to log in before you can comment on or make changes to this bug.