When quitting (not saving session) Firefox (this probably applies to other apps as well...), peristing sizemode=fullscreen leads to opening the first window fullscreen. This doesn't seem like the right behavior. So I'm proposing that just like we don't persist sizemode=minimzed, we shouldn't persist sizemode=fullscreen either. We should either not save it (as appears to be what we do with minimized), or we save it as maximized.

This applies to all platforms, but I'm just noticing on OS X as I was testing my work in bug 539601 and bug 639705. After playing with it for a minute, I'm leaning towards option 1 (don't save anything, which means we'll use whatever the previous sizemode was). Using maximized felt wrong, at least on OS X.

This ignores sizemode == nsSizeMode_Fullscreen

So there is a problem here: window.document.documentElement.sizemode doesn't get updated when sizemode=fullscreen. This is in line with what happens when sizemode=minimized. There is window.windowState which can be used to query the actual state and works with fullscreen & minimized. That's what sessionstore uses. Not sure if that's an acceptable change, but window.sizemode is already kinda broken, so...

This also breaks https://hg.mozilla.org/mozilla-central/file/dead9dc86cfa/dom/tests/mochitest/chrome/test_sizemode_attribute.xul because like I mention above, sizemode isn't updated. The test was only being run on Windows before and never checks minimized, so it's not the most useful test anyway.

I updated the sizemode_attribute.xul test to ignore checking sizeMode when expecting fullscreen. I also tried to enable it on OS X but then the test needs more work (in all practicality, it works but the test needs to be written a bit differently). I'm going to leave my partial patch in my queue & if I come back to it, I'll do so in bug 409095.

Backed out since this change broke theme and browser.js code: > So there is a problem here: window.document.documentElement.sizemode doesn't > get updated when sizemode=fullscreen. This is in line with what happens when > sizemode=minimized. https://hg.mozilla.org/mozilla-central/rev/344e07332d52 https://hg.mozilla.org/mozilla-central/rev/c1bfb6203345

(In reply to Dão Gottwald [:dao] from comment #8) > Backed out since this change broke theme and browser.js code: I thought I'd checked, but apparently I didn't check very well... So we're clear, this just breaks https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser-aero.css#145 and https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5445 ? Is there anything else? Can we just change these to use window.windowState?

(In reply to Paul O'Shannessy [:zpao] from comment #9) > So we're clear, this just breaks > https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/ > browser-aero.css#145 Also selectors using [sizemode=normal] or [sizemode=maximized] without wanting to affect full-screen windows. http://mxr.mozilla.org/mozilla-central/search?string=[sizemode%3D > Is there anything else? Add-ons? > Can we just change these to use window.windowState? In CSS? Not directly.

Well, persisting sizemode=fullscreen causes objc exceptions on restoring, so we'll need to figure something out, either here or down in widget. Neil, is there something we can do to keep the attribute up to date, but not persist certain values. I'm guessing the answer is currently "no"...

How about this? One note on the XXX comment: There's another Windows-specific "resize" handler in browser.js where this doesn't seem to be a problem. I tested this patch on Linux. Not sure if this explains it.

Or this? I haven't tested this.

Comment on attachment 609465 [details] [diff] [review] nsXULWindow::SavePersistentAttributes patch Review of attachment 609465 [details] [diff] [review]: ----------------------------------------------------------------- Heh, I had just tried this. It doesn't end up working because we'll still call Persist some other time when not in fullscreen. The persist code just knows what attributes to persist so will still end up persisting sizemode=fullscreen

(In reply to Dão Gottwald [:dao] from comment #12) > Created attachment 609458 [details] [diff] [review] > front-end patch > > How about this? > > One note on the XXX comment: There's another Windows-specific "resize" > handler in browser.js where this doesn't seem to be a problem. I tested this > patch on Linux. Not sure if this explains it. I didn't try yet, but my gut says it'll end up being the same issue - that once we tell an attribute to be persisted, it will regardless of value. I'll give it a try though

Looks like my gut was wrong. I guess your `document.persist` call only does a 1 time update? > One note on the XXX comment: There's another Windows-specific "resize" > handler in browser.js where this doesn't seem to be a problem. I tested this > patch on Linux. Not sure if this explains it. I think this would be ok for now, but we might need to change this whenever bug 625989 happens. The problem I have with this approach is that it doesn't solve the problem in other XUL apps. This is really a problem lower than browser and I feel like it should probably be fixed in XUL/widget. As a perhaps crazy over the top solution, what if from XULDocument::Persist, instead of calling GetAttr, we call GetPersistAttr, which we add. By default it would just forward on and call GetAttr. But it could be implemented and be used to filter values. In this case, if it was trying to get "sizemode", we would return "maximized" even if the actual value was "fullscreen".

(In reply to Dão Gottwald [:dao] from comment #10) > Also selectors using [sizemode=normal] or [sizemode=maximized] without > wanting to affect full-screen windows. > http://mxr.mozilla.org/mozilla-central/search?string=[sizemode%3D I realized we're adding the "inFullScreen" attribute in browser.js when we enter FS, so at least for everything in browser/ we could change the rules to check for that. That would just leave that one rule in toolkit/ to figure out.

I don't think we want to add :not([inFullScreen]) to every selector involving sizemode.

This might be belong in a new bug, but oh well. This patch doesn't stop persisting fullscreen sizemode, but instead will delay making the window fullscreen until it's ready. This works around the objc exception (see bug 639705 comment 68). Markus, this feels hackier than I would like (and windowDidUpdate gets called a lot), but I haven't thought of much else that would work right. I'd considered removing going fullscreen from nsXULWindow::LoadMiscPersistentAttributesFromXUL, but we want that for opening new windows (or we rewrite how that works, I'm also not sure that's the right place to get this anyway)

Neil, do you have any opinions here? Does my crazy idea from comment 16 sound like something we would want? Is it even doable in a sane way? Or should we work around this in some other way?

I'm not seeing an objc exception anymore so perhaps this is no longer an issue…

(to be fair, I still think persisting sidemode=fullscreen is crappy behavior, but the driving force was the exception)

Comment on attachment 611073 [details] [diff] [review] Alt Patch v0.1 - Delay going fullscreen This looks good to me. Sorry for the unbelievable lag here :(

No longer persisting sizemode=fullscreen is annoying for embedded apps. I was getting around this by removing write permission on xulstore.json, but later versions are now overriding even this.

My solution was to change the ownership of xulstore.json to root:root, and chmod 0644.

No, Firefox also replaces a root-owned xulstore.json with its own version without fullscreen sizemode. The only solution to start in full screen seems to be to start Firefox with a script that copies over xulstore.json.