Restart after closing app from fullscreen mode does not display fullscreen UI

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
In 3.6.4, this bug manifests normal full sized window on the desktop. In the recent titlebar work we did, we fixed this so we actually go into full screen mode on restart. Widget sends a size mode event, but for some reason the browser down't pick up on it, so the UI is the normal browser.

STR:

1) open
2) F11, go to fullscreen
3) close
4) restart

To get out of this weird normal browser/fullscreen mode, hit F11 again.
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Comment 1

7 years ago
Created attachment 455079 [details] [diff] [review]
widget patch
Assignee: nobody → jmathies
(Assignee)

Comment 2

7 years ago
Created attachment 455080 [details] [diff] [review]
appshell patch
(Assignee)

Comment 3

7 years ago
Created attachment 455081 [details] [diff] [review]
dom patch
(Assignee)

Comment 4

7 years ago
Created attachment 455082 [details] [diff] [review]
browser patch
(Assignee)

Comment 5

7 years ago
These patches fix a few bugs and improve one issue related to full screen.

Xul windows persist size modes, so when the user is in full screen mode and opens a new window, or if they shut down in fs and relaunch, the new window should be in fs mode. In 3.6 this doesn't work. On windows what you will get is a normal window that is sized to the size of the desktop. This same problem *was* in m-c builds until a patch for the new titlebar landed this weekend fixing the "big window" problem.

This didn't fix things entirely though - widget currently isn't sending a size mode event when we enter full screen mode [bug #1, widget patch]. nsXULWindow also doesn't propagate fs size mode events to the dom via calls to global window [bug #2, xpfe + dom patches]. So there's no connection on startup between widget's size mode changes and browser.js. Also, browser.js isn't hooked up the "fullscreen" event soon enough [bug #3, browser patch] to receive it on initial startup.

The last change, which is in the xpfe patch, makes it so browser windows with persisted fs mode opened on top of existing full screen windows open normally, which is a standard windows convention.
(Assignee)

Updated

7 years ago
Attachment #455079 - Flags: review?(vladimir)
(Assignee)

Updated

7 years ago
Attachment #455080 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #455081 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Attachment #455082 - Flags: review?(gavin.sharp)

Comment 6

7 years ago
Comment on attachment 455080 [details] [diff] [review]
appshell patch

>+#ifdef XP_WIN
Any reason this is Windows-specific?

>+  // If the parent is currently fullscreen, tell the child to ignore persisted
>+  // full screen states. This way new browser windows open on top of fullscreen
>+  // windows normally.
What if some other window opens a browser window?

>+      reinterpret_cast<nsXULWindow*>(window.get())->IgnoreXULSizeMode(PR_TRUE);
Doesn't nsWebShellWindow derive from nsXULWindow?

Comment 7

7 years ago
Comment on attachment 455081 [details] [diff] [review]
dom patch

This looks like something which should happen in widget level.
MakeFullScreen should just do the right thing, no?
Attachment #455081 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 8

7 years ago
(In reply to comment #6)
> (From update of attachment 455080 [details] [diff] [review])
> >+#ifdef XP_WIN
> Any reason this is Windows-specific?
> 
> >+  // If the parent is currently fullscreen, tell the child to ignore persisted
> >+  // full screen states. This way new browser windows open on top of fullscreen
> >+  // windows normally.
> What if some other window opens a browser window?

It works great, if you have a full screen browser and open a new with ctrl-n, it opens normal. Prior to this patch it would open full screen on top of the first full screen window.
(Assignee)

Comment 9

7 years ago
(In reply to comment #7)
> (From update of attachment 455081 [details] [diff] [review])
> This looks like something which should happen in widget level.
> MakeFullScreen should just do the right thing, no?

There's circular call risk here, I put this in to protect against that on other platforms. win32 widget handles this. So if we're sure

 widget -> send size mode to web shell 
 web shell -> global window -> fullscreen
 global window -> widget -> makefullscreen

Is safe, we don't need it.
(Assignee)

Comment 10

7 years ago
Created attachment 455179 [details] [diff] [review]
appshell patch v.2

- nix over zealous castings
Attachment #455080 - Attachment is obsolete: true
Attachment #455179 - Flags: review?(neil)
Attachment #455080 - Flags: review?(neil)
(In reply to comment #8)
>(In reply to comment #6)
>>(From update of attachment 455080 [details] [diff] [review] [details])
>>>+#ifdef XP_WIN
>>Any reason this is Windows-specific?
No answer to this question?

>>>+  // If the parent is currently fullscreen, tell the child to ignore persisted
>>>+  // full screen states. This way new browser windows open on top of fullscreen
>>>+  // windows normally.
>>What if some other window opens a browser window?
>It works great, if you have a full screen browser and open a new with ctrl-n
No, I said if some *other* window opens a browser window...
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> (In reply to comment #8)
> >(In reply to comment #6)
> >>(From update of attachment 455080 [details] [diff] [review] [details] [details])
> >>>+#ifdef XP_WIN
> >>Any reason this is Windows-specific?
> No answer to this question?

Sorry, missed that. Actually I was kind of hoping that would get fleshed out through reviews or by asking around to see what the convention is on other platforms. 

> 
> >>>+  // If the parent is currently fullscreen, tell the child to ignore persisted
> >>>+  // full screen states. This way new browser windows open on top of fullscreen
> >>>+  // windows normally.
> >>What if some other window opens a browser window?
> >It works great, if you have a full screen browser and open a new with ctrl-n
> No, I said if some *other* window opens a browser window...

Hmm, good question. Bookmarks comes to mind. I'll test and see. Maybe I should  walk across a list of all open top level windows looking for one that is full screen as a test.
(Assignee)

Comment 13

7 years ago
Comment on attachment 455179 [details] [diff] [review]
appshell patch v.2

I can improve on this slightly, but it'll likely be after the summit before I post a new rev.
Attachment #455179 - Flags: review?(neil) → review-
(Assignee)

Updated

7 years ago
Attachment #455081 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Created attachment 457505 [details] [diff] [review]
appshell patch v.3
Attachment #455179 - Attachment is obsolete: true
(Assignee)

Comment 15

7 years ago
Comment on attachment 457505 [details] [diff] [review]
appshell patch v.3

Updated to walk the list of open windows.
Attachment #457505 - Flags: review?(neil)
Attachment #455079 - Flags: review?(vladimir) → review+
blocking2.0: ? → -
I didn't realize the resulting window was unusable, we need to at least restore with a usable window.
blocking2.0: - → final+
Comment on attachment 457505 [details] [diff] [review]
appshell patch v.3

>+  PRBool more;
>+  do {
>+    windowList->HasMoreElements(&more);
>+    if (!more)
>+      return PR_FALSE;
This ...

>+      nsIWidget* widget = nsnull;
nsCOMPtr should work on nsIWidget, no? Saves manually releasing.

>+  } while (more);
... means that this is always true. So you can just write
for (;;) {
  PRBool more = PR_FALSE;
etc.

>+    window.get()->IgnoreXULSizeMode(PR_TRUE);
Nit: .get() shouldn't be necessary

I should be able to give this a whirl soon now that I've got a working build again.
(Assignee)

Updated

7 years ago
Blocks: 580599

Updated

7 years ago
Attachment #457505 - Flags: review?(neil) → review+
Comment on attachment 455082 [details] [diff] [review]
browser patch

The event callback needs the height of the toolbox, which can change during delayedStartup.
Attachment #455082 - Flags: review?(gavin.sharp) → review-
Created attachment 460219 [details] [diff] [review]
browser patch
Attachment #455082 - Attachment is obsolete: true
Attachment #460219 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

7 years ago
gavin, ping?
Comment on attachment 460219 [details] [diff] [review]
browser patch

enterFS is a bit confusing, maybe use "inFullscreen"? I know it's not exactly accurate in the transition case, but I don't really think that's a problem.
Attachment #460219 - Flags: review?(gavin.sharp) → review+
(In reply to comment #21)
> Comment on attachment 460219 [details] [diff] [review]
> browser patch
> 
> enterFS is a bit confusing, maybe use "inFullscreen"? I know it's not exactly
> accurate in the transition case, but I don't really think that's a problem.

What's confusing, "enter" or "FS"? I actually care about the accuracy, as the vague semantics of just "fullScreen" confused me when I wrote the patch.
"enterFS" seems to describe the transition as opposed to the end-state that the controls are being set up for, which I thought was kind of odd. But I don't feel strongly - just leave it as is if you want.
State names are traditionally gerunds or participles/adjectives (consider TCP closing and closed), not imperative-mood verbs like "enter". Sorry to be a nosy bystander but I'm wondering whether entering or entered is best. What's really going on here?

For the ending, -FullScreen instead of -FS seems winning, but maybe the whole name is then too long.

/be
The function runs both before the transition (in the normal "toggle full screen for a given window" case) or after (in the case where a window is restored as a  fullscreen window on startup), so entered/entering can both be wrong for a given invocation. That's why I suggested "inFullScreen" - describes the final state that we're preparing the UI for.
The function runs in two different states. Does the distinction really not matter?

/be
I'm not sure what you're asking. It either updates the UI to match the current state in the post-transition invocation, or updates the UI to match the future state in the pre-transition invocation. In both cases the state it's using to update the UI is reflected by the boolean.
current state : post-transition invocation :: future state : pre-transition

It all makes sense but it's hard to name something that has two "tenses" :-|.

/be

Updated

7 years ago
Duplicate of this bug: 583794
Blocks: 517379
(Assignee)

Comment 30

7 years ago
http://hg.mozilla.org/mozilla-central/rev/bb9d266da12c
http://hg.mozilla.org/mozilla-central/rev/0b6d45b6c2e6
http://hg.mozilla.org/mozilla-central/rev/a179e301dbfd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 591408
No longer blocks: 517379
Duplicate of this bug: 517379
OS: Windows 7 → All
Depends on: 613292
You need to log in before you can comment on or make changes to this bug.