370857 - Fix/enhance fullscreen mode on OS X/Cocoa

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Sylvain Pasche]

Attachment: Full screen patch, v1

First of all, some background information:

When doing fullscreen, we basically want:

1. Hide the dock and/or menu if they are on the screen where we toggle fullscreen:

This can be achieved using the SetSystemUIMode function mentioned in the above document. If was thinking of only hiding the dock/menu bar if we are on the menu bar screen, but there are situations where the dock is not on the same screen as the menu bar. This happens for instance when you have a screen below the menu bar screen: the dock will be on the bottom screen and the menu bar on the top screen. While some algorithm could be used for guessing if we have the dock/menubar visible on the screen where we want to go full screen, the patch hides them in any case. Opera or Joost do the same.

2. Hide the window title bar.

It is unfortunately not possible to hide a window title bar after it is created. The common "hack" to get around this is to move the window a bit offscreen, so that the title bar is above the screen (this is how Opera and Joost do). Another way would be to create a new window with no title bar, and reparent the NSView on it (that's what Shiira do apparently), but I don't know if it's possible or better to do in our case.

For this to work, we need to remove the default Cocoa window constraint which does not allow windows to have their height greater that the screen's height. That's why I introduced a new NSWindow subclass for dealing with this (I couldn't find a way to do it through the delegate).

Some issues/discussions related to the implementation:

- I hide the resize indicator while in full screen, and prevent window resizing through the delegate. However, the resize indicator square will still eat click events (for instance, the scrollbar arrow is not clickable in this region). Opera manages to pass the events through it, so there should be a way to deal with this (ideas?).

- I didn't change type of nsCocoaWindow::mWindow to UnconstrainableWindow, as there are some cases were the window is not created by nsCocoaWindow directly:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsCocoaWindow.mm&rev=1.81&mark=385#385

I coped with such a situation in the fullscreen switch, so that the menubar is visible in case we have a true NSWindow. Not sure in what case this could happen (embedding?)

- nsCocoaWindow::MakeFullScreenInternal is duplicated from nsBaseWidget::MakeFullScreenInternal as we need to do things slightly differently. See comment in patch for proposed fixes. That could result in another bug opened for changing xpwidget.

- member ordering: not sure about the policy.

- I'm getting a warning when calling HideWindowChrome from the destructor. Maybe that's not the right place to call it.

Miscellaneous:
If you want to test this in a multi-screen environment, you should apply patch in bug 370106.
Likewise, if you want to switch to fullscreen when the window is partly off screen, it won't work until bug 370107 is fixed.

You may find this extension useful for testing on Firefox: https://addons.mozilla.org/firefox/810/

Comment 1

[Josh Aas]

I won't be able to review this week, hopefully next week. Thanks for looking in to this!

Comment 2

[Sylvain Pasche]

Note to self: after discussion with Josh, this will need to wait a few month for the new theming, top-level window changes and other things to land.

Don't hesitate to ping me when this can be respinned.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sylvain are there already existing bugs about the upcoming things we have to wait for?

Comment 4

[Sylvain Pasche]

(In reply to comment #3)
> Sylvain are there already existing bugs about the upcoming things we have to
> wait for?

Honestly I don't know if all the prerequisites are materialized in bugzilla. Ideally, these should be added in dependencies, so that we can formally know when to move this forward (catching ff 3.0 would be great).

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #4)
> Honestly I don't know if all the prerequisites are materialized in bugzilla.
> Ideally, these should be added in dependencies, so that we can formally know
> when to move this forward (catching ff 3.0 would be great).

Do you have a URL where I can find a list of needed prerequisites? So I could do a search in Bugzilla to find already existing reports.

Comment 6

[Sylvain Pasche]

(In reply to comment #5)
> Do you have a URL where I can find a list of needed prerequisites? So I could
> do a search in Bugzilla to find already existing reports.

I'm afraid such an URL does not exist. The best way would be to contact Josh directly to ask for more details, we just talked on IRC some times ago. I'm more on Linux these days, so this is not very high priority for me now, but I'd be happy to work on it when prerequisites are fulfilled.

Comment 7

[Josh Aas]

Sylvain - I think the stuff I wanted to get done first is done if you want to take another crack at this.

Comment 8

[Sylvain Pasche]

Attachment: nsIWidget changes

I've split the patch between a xpwidgets and a cocoa part. This is the xpwidgets part. This removes the code duplication I had in the previous version.

Comment 9

[Sylvain Pasche]

Attachment: cocoa part, v2

This is the cocoa part. I renamed the ToolbarWindow class, as it is now used for more than showing the toolbar pill button.

I noticed a small issue with the patch applied. If you enter fullscreen and quit the browser, next time you open it the window will try to use the last fullscreen window size. This means that the window titlebar will be under the top menu (so it can't be moved). I think this should be a follow-up bug, as it involves different parts.

Comment 10

[Colin Barrett [:cbarrett]]

Sylvian, just be aware that you'll probably get/cause conflicts with bug 303110.

Also, I was under the impression the way to do fullscreen mode was to use CGDisplayCapture(kCGDirectMainDisplay), and then [window setLevel:CGShieldingWindowLevel()]; [window setFrame:[[NSScreen mainScreen] frame] display:YES];

Comment 11

[Colin Barrett [:cbarrett]]

Er, disregard that. I just found tn2062.

http://developer.apple.com/technotes/tn2002/tn2062.html

Adding the auto-show menubar option would be cool!

Comment 12

[Sylvain Pasche]

(In reply to comment #10)
> Sylvian, just be aware that you'll probably get/cause conflicts with bug
> 303110.

ok, I'll keep an eye on it.

(In reply to comment #11)
> Adding the auto-show menubar option would be cool!

I tried with it, but that can be annoying if you have some Firefox toolbar autohiding extensions (like fullerscreen). Both Firefox and the native menubar will show up when moving the mouse on the top of screen.

Comment 13

[Josh Aas]

Sorry I'm letting this one sit for a bit, but we need to let Gecko 1.9 M10 development open up and land some patches we have ready before we can get to this. Thanks for keeping the patch up to date!

Comment 14

[Sylvain Pasche]

Attachment: cocoa part, v2.1

Update after bug 303110. The other patch for nsIWidget still applies.

Comment 15

[Dave Townsend [:mossop]]

So the hack of just moving the window's titlebar above the top of the screen really doesn't work so well when you have a multimonitor display with a second screen above the one you are going fullscreen on. It's also of course the cause of comment 9, though the title bar isn't under the menubar, it is above it.

Comment 16

[Sylvain Pasche]

Thanks for your feedback.

(In reply to comment #15)
> So the hack of just moving the window's titlebar above the top of the screen
> really doesn't work so well when you have a multimonitor display with a second
> screen above the one you are going fullscreen on.

Yes, that's really the shortcoming of this hack. It would be great of there was a way in Cocoa to hide the titlebar of a window dynamically.

Note that Opera or Joost use the same trick for fullscreen.

> It's also of course the cause
> of comment 9, though the title bar isn't under the menubar, it is above it.

I'd say that it worsens the real issue there: the application should either:
1) restore in fullscreen mode if it was closed in this state (probably not very intuitive)
2) restore the previous window size before entering fullscreen.

I also noticed this issue on Linux with a window manager that do not constrain the window position: I could get the titlebar under the Gnome menu when I start Firefox after quitting it in fullscreen mode.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> I'd say that it worsens the real issue there: the application should either:
> 1) restore in fullscreen mode if it was closed in this state (probably not very
> intuitive)
> 2) restore the previous window size before entering fullscreen.

Well what is going on is when the window is resized the xul is persisting the width, height, position etc exactly as it should. I guess a cheap, though not ideal hack would be to just not report the new size and position when you go into fullsize.

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> Well what is going on is when the window is resized the xul is persisting the
> width, height, position etc exactly as it should. I guess a cheap, though not
> ideal hack would be to just not report the new size and position when you go
> into fullsize.

Actually don't do that, it's a stupid idea.

Comment 19

[Josh Aas]

If you could update this patch soon we might be able to get to this now. Sorry again for the delays.

Comment 20

[Sylvain Pasche]

Attachment: nsIWidget changes

great to see this moving.

Comment 21

[Sylvain Pasche]

Attachment: cocoa part, v2.2

Comment 22

[Sylvain Pasche]

Attachment: cocoa part, v2.2

Comment 23

[Håkan Waara]

Ok, I've reviewed the patches and the approach. Here are some general open questions (not code-related, I get to those in the next post). I'm CC:ing some UE guys for input especially on #1.

1. I worked in technical support for a swedish broadband company a few years ago and actually had some people call in panicked when they accidently had hit F11 in Internet Explorer (which enters fullscreen mode), and had no idea what was happening or how to fix it. For this reason, I believe persisting fullscreen mode is a bad idea.

I'd propose that we implement a listener for the quit event, and make sure to exit the fullscreen mode there, just before qutting, maybe even restore to the last window size before fullscreen mode, as you suggested in an earlier post. CC:ing some UE guys for their input on this.

2. I think it's desirable in the long run to let fullscreen really just enter fullscreen on the current screen. This means figuring out what the "current" screen is, and also doing the same before hiding the dock and the menubar. 

However, since we cannot hide the titlebar on the fly for a window (without recreating the window or reparenting the content view), I think we should spin off a separate bug for implementing this. Reparenting the contentview is IMO the "right" way to do this, but it's stability-wise it's a risky change (what with widgets caching weak pointers to native windows, etc) so maybe that's even a post-1.9 thing.

Comment 24

[Håkan Waara]

Comment on attachment 299866 [details] [diff] [review]
cocoa part, v2.2

* You're using HideWindowChrome() as a place to enter/exit fullscreen mode, but are you sure that's the only time it's used? I think you should be overriding MakeFullscreen() instead? Also, you probably want to call the nsBaseWidget's impl as well, to avoid any future bugs; both in this method and in |AdjustFullscreenBounds|.

* Apart from the above, calling HideWindowChrome() from ~nsCocoaWindow is potentially dangerous! You're modifying a window which has just been released. The thing that saves you from a crash is that it's been autoreleased... 

To restore the menubar/dock, I suggest you create a separate helper method (maybe in nsCocoaUtils.mm) which basically hides/shows the menubar and dock. Both the dtor and the MakeFullscreenMode(PR_FALSE) could use that safely.

* Instead of implementing two new setters in two different places, setUnconstrained (on ToolbarWindow) and setAllowResize (on WindowDelegate), you can create a new concept of "locking a window" which would mean making it unresizable and immovable. That way you'd only need one method and one instance variable controlling this; both on the WindowDelegate. ToolbarWindow's constrainFrameRect:toScreen: would ask the delegate for the locked state before doing anything.

Comment 25

[Sylvain Pasche]

Thanks for your review Håkan. If I can't find time this week, I'll dive back into this this week-end.

Comment 26

[Sylvain Pasche]

Attachment: cocoa part, v3

(In reply to comment #23)
> 1. I worked in technical support for a swedish broadband company a few years
> ago and actually had some people call in panicked when they accidently had hit
> F11 in Internet Explorer (which enters fullscreen mode), and had no idea what
> was happening or how to fix it. For this reason, I believe persisting
> fullscreen mode is a bad idea.

Yes, not persisting fullscreen seems the best thing to to.

> I'd propose that we implement a listener for the quit event, and make sure to
> exit the fullscreen mode there, just before qutting, maybe even restore to the
> last window size before fullscreen mode, as you suggested in an earlier post.
> CC:ing some UE guys for their input on this.

I couldn't reproduce the issue I saw on comment 9 (If I quit the browser while having a window in fullscreen, it isn't positioned under the Menu/Dock after I restart it).
I'm not sure if this has been fixed in OSX 10.5, or in Gecko. If that's the latter the issue would not be a blocker I think (while it would still be good to have this fixed).


> 2. I think it's desirable in the long run to let fullscreen really just enter
> fullscreen on the current screen. This means figuring out what the "current"
> screen is, and also doing the same before hiding the dock and the menubar.

You mean that the Menu or Dock should only be hidden when they are on the same screen as the window that enters fullscreen?

That would be the best option, however last time I checked I didn't find an easy way of knowing on what screen the Dock was located.

> However, since we cannot hide the titlebar on the fly for a window (without
> recreating the window or reparenting the content view), I think we should spin
> off a separate bug for implementing this. Reparenting the contentview is IMO
> the "right" way to do this, but it's stability-wise it's a risky change (what
> with widgets caching weak pointers to native windows, etc) so maybe that's even
> a post-1.9 thing.

Filed bug 420491

(In reply to comment #24)
> (From update of attachment 299866 [details] [diff] [review])
> * You're using HideWindowChrome() as a place to enter/exit fullscreen mode, but
> are you sure that's the only time it's used? I think you should be overriding
> MakeFullscreen() instead? Also, you probably want to call the nsBaseWidget's
> impl as well, to avoid any future bugs; both in this method and in
> |AdjustFullscreenBounds|.

I'm not sure why I used HideWindowChrome for entering fullscreen at the time. HideWindowChrome should hide borders/titlebars but that is not possible/meaningful on Cocoa.

Now I do that in MakeFullscreen()

> * Apart from the above, calling HideWindowChrome() from ~nsCocoaWindow is
> potentially dangerous! You're modifying a window which has just been released.
> The thing that saves you from a crash is that it's been autoreleased...

Fixed by doing what you propose next

> To restore the menubar/dock, I suggest you create a separate helper method
> (maybe in nsCocoaUtils.mm) which basically hides/shows the menubar and dock.
> Both the dtor and the MakeFullscreenMode(PR_FALSE) could use that safely.

I added a nsCocoaUtils::HideMenuAndDock(PRBool aShouldHide)

> * Instead of implementing two new setters in two different places,
> setUnconstrained (on ToolbarWindow) and setAllowResize (on WindowDelegate), you
> can create a new concept of "locking a window" which would mean making it
> unresizable and immovable. That way you'd only need one method and one instance
> variable controlling this; both on the WindowDelegate. ToolbarWindow's
> constrainFrameRect:toScreen: would ask the delegate for the locked state before
> doing anything.

Good idea. I added a mIsLocked member on the delegate.

Comment 27

[Håkan Waara]

Comment on attachment 306775 [details] [diff] [review]
cocoa part, v3

> > 2. I think it's desirable in the long run to let fullscreen really just enter
> > fullscreen on the current screen. This means figuring out what the "current"
> > screen is, and also doing the same before hiding the dock and the menubar.
> 
> You mean that the Menu or Dock should only be hidden when they are on the same
> screen as the window that enters fullscreen?
> 
> That would be the best option, however last time I checked I didn't find an
> easy way of knowing on what screen the Dock was located.

See http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSScreen_Class/Reference/Reference.html#//apple_ref/occ/clm/NSScreen/mainScreen

Basically, in the [NSScreen screens] array, the first object is the one with the menubar and the Dock. Then you can use NSWindow's |screen| message to find out on which screen the window lives.

+#include "MacApplication.h"

I think you can do #import <Carbon/Carbon.h> instead (MacApplication.h is included in the Carbon framework).

>+  // Keep track of how many hiding requests have been made, so that they can
>+  // be nested.

I'm not sure about this. Is it really useful to be able to nest these calls? I'm worried it's a potential source for bugs if the calls don't line up... Hmm. Can we at least make sure the same window doesn't accidently increase/decrease the count more than once in a row?

+  // A locked window cannot be moved and removes positionning constraints so
+  // that it can be moved offscreen for full screen mode.
+  BOOL mIsLocked;

Fix the typo. ;)

>+NS_IMETHODIMP nsCocoaWindow::AdjustFullScreenBounds(PRInt32* aLeft, PRInt32* aTop,
>+                                                    PRInt32* aWidth, PRInt32* aHeight)
>+{
>+  if (![mWindow isKindOfClass:[ToolbarWindow class]])
>+    return nsBaseWidget::AdjustFullScreenBounds(aLeft, aTop, aWidth, aHeight);

Should we not be invoking this method for all windows? I think it depends on what it will do, of course, but I think it's the best to default to.

>+  if ([[self delegate] isLocked])
>+    return frameRect;

Please make a comment about the "locked" mode here.

Comment 28

[Sylvain Pasche]

Attachment: cocoa part, v4

(In reply to comment #27)
> See
> http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSScreen_Class/Reference/Reference.html#//apple_ref/occ/clm/NSScreen/mainScreen
> 
> Basically, in the [NSScreen screens] array, the first object is the one with
> the menubar and the Dock. Then you can use NSWindow's |screen| message to find
> out on which screen the window lives.

This can be useful for finding the menubar and not hiding it if we are on another screen. (Note for reference: this can be achieved by using |SetSystemUIMode(kUIModeContentHidden, 0);| which will hide the Dock only). Maybe that could be addressed separately? Because the Dock is not always on the same screen (see next paragraph) that makes things tricky to manage.

That's difficult to locate the Dock because it may not always be on the same screen as the menubar. For instance, if you have a second screen under the one with the menubar, the Dock will be on the lowest screen. Another example is if you have two horizontally positioned screens, the menubar on the right screen and your Dock position set to the left: the Dock will appear on the left screen (the one without the menubar).

Because of this, we may be forced to always hide the Dock when entering full screen.

> +#include "MacApplication.h"
> 
> I think you can do #import <Carbon/Carbon.h> instead (MacApplication.h is
> included in the Carbon framework).

you're right, works fine with Carbon.h

> >+  // Keep track of how many hiding requests have been made, so that they can
> >+  // be nested.
> 
> I'm not sure about this. Is it really useful to be able to nest these calls?
> I'm worried it's a potential source for bugs if the calls don't line up... Hmm.

I think this is required for managing multiple windows entering fullscreen at the same time. For instance, if you have two windows A and B:

A enters fullscreen
 -> HideMenuAndDock(true)
  -> sHiddenCount = 1 -> Hiding of Menu/Dock
B enters fullscreen
 -> HideMenuAndDock(true)
  -> sHiddenCount = 2 -> nothing to do
A leaves fullscreen
 -> HideMenuAndDock(false)
  -> sHiddenCount = 1 -> nothing to do
B leaves fullscreen
 -> HideMenuAndDock(false)
  -> sHiddenCount = 0 -> Unhiding of Menu/Dock

> Can we at least make sure the same window doesn't accidently increase/decrease
> the count more than once in a row?

That should be prevented by always calling HideMenuAndDock when mFullScreen instance variable is toggled.
The other constraint is that MakeFullScreen should never be called twice with the same value. This should be ensured in http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&rev=1.999&mark=3685-3690#3685. I've added an assert just to be sure.

> +  // A locked window cannot be moved and removes positioning constraints so
> +  // that it can be moved offscreen for full screen mode.
> +  BOOL mIsLocked;
> 
> Fix the typo. ;)

I want a spellchecker in my editor ;-)

> >+NS_IMETHODIMP nsCocoaWindow::AdjustFullScreenBounds(PRInt32* aLeft, PRInt32* aTop,
> >+                                                    PRInt32* aWidth, PRInt32* aHeight)
> >+{
> >+  if (![mWindow isKindOfClass:[ToolbarWindow class]])
> >+    return nsBaseWidget::AdjustFullScreenBounds(aLeft, aTop, aWidth, aHeight);
> 
> Should we not be invoking this method for all windows? I think it depends on
> what it will do, of course, but I think it's the best to default to.

I'm not sure to understand what you mean here. You mean we shouldn't call the parent's implementation in that case and just return NS_OK?

> >+  if ([[self delegate] isLocked])
> >+    return frameRect;
> 
> Please make a comment about the "locked" mode here.
> 

added

Comment 29

[Bob]

Is this going to happen?  Firefox 3 really needs fullscreen on the Mac.  It is
only partially finished without it!

Now that the code is not frozen, could this be done for Firefox 3.1?

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It looks like that this feature was forgotten. We should nominate for blocking1.9.1.

Håkan, could you please have a look at the patch?

Comment 31

[Håkan Waara]

Comment on attachment 307118 [details] [diff] [review]
cocoa part, v4

With Boris work on bug 113934 there is potential for a much simpler and nicer solution that involves almost no C++. The approach would be this:

1. Create a XPCOM wrapper method that will hide the dock and menubar. Use that, create a new titlebarless window, and then just move all of the tabs into the new window. nsIFullscreen::hide/showOsChrome seems like a good method to implement in this case.

If that is for some reason not possible, we still have two other approaches, similar to this patch, so it's more C++ and not as neat.

2. Make it possible to switch window type "on the fly" for a nsCocoaWindow. 

To do this, you would have to hold on to a nsCocoaWindow's content view (the NSWindow's contentView), recreate the current NSWindow as a titlebarless window, and then finally put back the old content view into the new window.

3. In the worst case, if we can't get #2 or #3 working, I guess we could still go with the approach of this patch: resize the window to be as big as the screen and move it up the height of the titlebar, effectively "hiding" it. People with screens on top of each other will see the titlebar sticking up at the bottom of the upper screen if the lower screen goes into fullscreen mode, so it's not really ideal, but according to Sylvain that's what both Joost and Opera do...

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can the following URL help us here?

http://www.cocoadev.com/index.pl?ImplementingFullScreen

Comment 33

[Håkan Waara]

(In reply to comment #32)
> Can the following URL help us here?
> 
> http://www.cocoadev.com/index.pl?ImplementingFullScreen

The challenge is not resizing our window: that's easy. The challenge is that we're changing window type, from a window with titlebar (a regular browser window) to a borderless window. 

In Mac OS X you cannot change window type on an already-created window, so you need to recreate it (or create a new window) and put your old content there. That's basically what #1 and #2 are about.

Comment 34

[Paweł Kondzior]

VLC is implementing exactly that approach. Code is here:

http://git.videolan.org/gitweb.cgi?p=vlc.git;a=blob;f=modules/gui/macosx/embeddedwindow.m;h=b9f22cd82bca7179f16e47e8a8fb8a195ba3fda0;hb=8f2cd83a36d67d0107c9747be623903f60aaf24a#l219 

Comment 35

[Sylvain Pasche]

What we discussed with Håkan (comment #31) should offer a better solution than what we have now. Unfortunately, I won't have time for this in the near future. If anyone has time to move this forward, please feel free.

Comment 36

[jrblier]

Not enough permission for me here. Can someone review the following bugs and mark them either fix or duplicate?

Bug 211551 is a duplicate.
Bug 126685 is a duplicate.

Is there any plan for FF 3.2 if we miss the 3.1 release?

Comment 37

[Brian Crowder]

Those other bugs are not dupes.  One is a tracking bug which will be resolved by the resolution of this one, and the other is a bug which this solution should ensure it addresses (likely will).

Comment 38

[Eric Kanagy]

(In reply to comment #33)
> (In reply to comment #32)
> > Can the following URL help us here?
> > 
> > http://www.cocoadev.com/index.pl?ImplementingFullScreen
> 
> The challenge is not resizing our window: that's easy. The challenge is that
> we're changing window type, from a window with titlebar (a regular browser
> window) to a borderless window. 
> 
> In Mac OS X you cannot change window type on an already-created window, so you
> need to recreate it (or create a new window) and put your old content there.
> That's basically what #1 and #2 are about.

Any progress on this approach? My company, RedPost, is using Firefox for digital signage...we'd like to be platform independent, which is what Firefox offers, but we need to get rid of the title bar on the Mac. Linux/Windows fullscreen mode works great. We'd also prefer that the fullscreen setting is persistent, but we can work around that, as its prbly better for the generic user that its not...

Comment 39

[Markus Stange [:mstange]]

Taking.

Comment 40

[Markus Stange [:mstange]]

Tryserver build is here: https://build.mozilla.org/tryserver-builds/[e-mail]-try-74eb7751ae2/try-74eb7751ae2-macosx.dmg
The extension https://addons.mozilla.org/firefox/810/ is useful for testing.

The tryserver build still has at least two bugs:
 - Dock and menubar are hidden even if the fullscreen window is on a different
   screen.
 - Restoring a window from fullscreen will position the window on the main
   screen, even if it started out on a different screen.

Also, the transition to fullscreen is all flashy and jumpy, but I think we can live with that for now.

Comment 41

[Markus Stange [:mstange]]

Another bug: On Windows, restoring from fullscreen fails to restore the window chrome.

Comment 42

[Dave Townsend [:mossop]]

(In reply to comment #41)
> Another bug: On Windows, restoring from fullscreen fails to restore the window
> chrome.

How is this OSX patch causing a bug on Windows?

Comment 43

[Markus Stange [:mstange]]

I haven't attached the patch that I used for the tryserver build yet. It affects Windows because it makes changes to nsBaseWidget::MakeFullscreen.

Comment 44

[Alex Faaborg [:faaborg] (Firefox UX)]

We will likely want to do something cool with the icon design when in full screen mode.  A HUD appearance seems like it would match other full screen modes.  I'm tagging this [icon-namoroka] so we can get started on the eye candy after we ship 3.5 (ideally a bit earlier in the release cycle compared to this time around).

Comment 45

[Markus Stange [:mstange]]

New build is here: https://build.mozilla.org/tryserver-builds/[e-mail]-try-c36281d6e79/try-c36281d6e79-macosx.dmg
It fixes all the issues that I mentioned in the previous comments.

It would be great if people could test this. I'm especially interested in any new crashes that I might have introduced.
Since there's no UI to get into full screen mode on OS X, you'll need the extension mentioned in comment 40.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Will you add the gui part at a later stage to the patch or will it be a separate bug?

Comment 47

[philippe (part-time)]

(In reply to comment #45)

Seems to work well with a _test_ profile in a first round of browsing - mostly around what I have on my dev. server. A bit overwhelming on a 2000px wide screen though (me is used to 1200px wide windows maximum).

> ... I'm especially interested in any
> new crashes that I might have introduced.

Any particular situation you have in mind ? Of course you don't know, but I'm just asking in case you fear some weak point somewhere. So far it holds well.


> Since there's no UI to get into full screen mode on OS X, you'll need the
> extension mentioned in comment 40.

Note that you need 'user_pref("extensions.checkCompatibility", false);' for that extension to install.

Comment 48

[Markus Stange [:mstange]]

(In reply to comment #46)
> Will you add the gui part at a later stage to the patch or will it be a
> separate bug?

Separate bug, tracked by bug 307371.


(In reply to comment #47)
Thanks for testing!

> Any particular situation you have in mind ?

Not really. "Normal" browsing should be enough. That's how I hit the crash bug that the patch in bug 495920 fixes (which is included in the tryserver build) - I just middle-clicked a link.

Comment 49

[philippe (part-time)]

No crashes yet; good.

This may need some attention:
STR
1. enter fullscreen mode
2. view any youtube video
3. set to (flash) video to full screen.
4. esc to leave flash full screen.

At this point the window is send behind everything. The menu bar is back in front, as is the Dock. There is no way to bring back the window to the front (short of escaping out of fullscreen mode.) Mouse interaction is still possible, but most keyboard shortcuts don't work.
That is basically bug 452843, just a bit more annoying than usual.

Comment 50

[Markus Stange [:mstange]]

Attachment: wip

Comment 51

[Jack Anderson]

Hi, Sorry I haven't got a clue how to to install this patch. Could you please provide a short guide? I just want the darn the Title Bar hidden, even this addon doesn't work: https://addons.mozilla.org/en-US/firefox/addon/9256

I'll do anything (to the software on the computer).

Please help!

Thank You!!!!!!!!

Comment 52

[Markus Stange [:mstange]]

Attachment: v5

Comment 53

[Markus Stange [:mstange]]

Landed: http://hg.mozilla.org/mozilla-central/rev/d7c1d5b9dcd1

Further UI work (like enabling the UI in the first place) will be tracked in bug 307371.

Also, thanks Sylvain for the original patches! They did help me a lot.

Comment 54

[Sylvain Pasche]

Woohoo! Thank you a lot for making this happen :-)

Comment 55

[Jack Anderson]

How do I use/test/apply/implement/ it? =(

Comment 56

[Dave Townsend [:mossop]]

(In reply to comment #55)
> How do I use/test/apply/implement/ it? =(

Bugzilla is not a support forum, take your questions to one of the development newsgroups or IRC channels. In general you will need to download the Firefox source code, apply the patch and build it. Unless you are fairly comfortable with doing that you'd be better off just waiting for a nightly build with the fix in it.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Markus are automated tests possible? Beside that QA should check existing Litmus tests if they need updates.

Comment 58

[Markus Stange [:mstange]]

There's already a small fullscreen test that was added by bug 484488. But it should probably be extended to check whether the window really covers the whole screen. So yes, automated tests are possible.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Full screen looks perfect now with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090921 Namoroka/3.6a2pre ID:20090921034459

Comment 60

[David Bolter [:davidb] (NeedInfo me for attention)]

How do I invoke the full screen mode via keyboard?

I noticed an access key which is nice, but unfortunately I couldn't get it to work (OS X) even when the video had focus-- it just just seemed to invoke the browser's full screen mode.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-context.inc#106

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, see bug 505699. It's F11 or Cmd+Shift+F.

Comment 62

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #61)
> David, see bug 505699. It's F11 or Cmd+Shift+F.

Yep, that's what I've been trying but I hit your bug 505699 comment 37.

Comment 63

[Markus Stange [:mstange]]

(In reply to comment #60)
> it just just seemed to invoke the browser's full screen mode.

That's what it's supposed to do and what this bug is about :-)
Full screen video is bug 453063, and I don't know how to invoke it via the keyboard. Maybe there are magic keys that open the context menu?

Comment 64

[David Bolter [:davidb] (NeedInfo me for attention)]

Yep, sorry, my bad. (And yeah ctrl+space on mac for FF context menu) I'll comment in the right bug now :)

Comment 66

[SteveC]

how do I figure out what version of FF will include this? I note FF 3.5.3 doesn't include fullscreen as far as I can see, but the bug's marked as fixed, will it be in 3.6?

Comment 67

[Jesse Ruderman]

Firefox 3.6 will include this.  (The "verified1.9.2" keyword means it was fixed and verified for Gecko 1.9.2, which corresponds to Firefox 3.6.)

Comment 68

[SteveC]

ah, thanks!