STR: * Navigate to YouTube video * Click on Fullscreen on video * Close window with ctrl-F4 * Start Firefox again Expected results: Firefox returns to maximised or windowed mode Actual results:

Firefox is in F11 fullscreen mode.

Xidorn - is this issue on your radar?

No. Thanks for reminding me. I think we have various issues when a page is closed in fullscreen state.

On Mac, the window state is also confused if Firefox is exited via Cmd-Q when on video fullscreen.

Jet - can I vote for this to happen sooner rather than later? It's actually quite annoying - especially if Firefox crashes when you're watching a video in full screen. A lot of people wouldn't know to press F11 to fix the issue.

Actually the STR is not that precise. It may need time between entering fullscreen and closing the window to reproduce this issue.

This patch tries to reset the "persist" attribute on the topmost <window> element for DOM Fullscreen to avoid persisting the window state, and restore that when exiting fullscreen. On Windows, the window would still open maximized in case Firefox unexpectedly exits in DOM Fullscreen. It may need further investigation, but has been far less annoying.

Comment on attachment 8647979 [details] [diff] [review] patch Review of attachment 8647979 [details] [diff] [review]: ----------------------------------------------------------------- Some comments may not apply if you rework the patch, especially if you eliminate the backup attribute. ::: dom/base/nsGlobalWindow.cpp @@ +6548,5 @@ > + } > + // We don't want to persist the window state if we enter fullscreen > + // for Fullscreen API, because we should not, and actually we cannot > + // properly restore to that state. For this reason, we need disable > + // persistence and restore it accordingly. This comment should probably be merged with the comment above. @@ +6549,5 @@ > + // We don't want to persist the window state if we enter fullscreen > + // for Fullscreen API, because we should not, and actually we cannot > + // properly restore to that state. For this reason, we need disable > + // persistence and restore it accordingly. > + if (!aFullScreen) { You are checking the same variable twice. Actually it's a bit hard to understand this logic, one piece of the logic lives here and the rest in nsXULWindow. @@ +6550,5 @@ > + // for Fullscreen API, because we should not, and actually we cannot > + // properly restore to that state. For this reason, we need disable > + // persistence and restore it accordingly. > + if (!aFullScreen) { > + xulWin->ToggleAttributePersistence(true); I would at least modify this to |/* aAllowPersist */ true| here and below. Also, don't you need to check |aReason == eForFullscreenAP)| here too ? ::: xpfe/appshell/nsXULWindow.cpp @@ +1540,5 @@ > +MoveElementAttributeValue(Element* aElement, const nsAString& aFromAttr, > + const nsAString& aToAttr) > +{ > + ErrorResult rv; > + nsAutoString value; No need for nsAutoString AFAIK. You don't need to modify that string. @@ +1547,5 @@ > + aElement->SetAttribute(aToAttr, value, rv); > +} > + > +NS_IMETHODIMP > +nsXULWindow::ToggleAttributePersistence(bool aAllowPersist) Why do we need to backup the "persist" attribute. Could we instead set a flag that we don't want to persist attributes if we are in dom full screen mode ? @@ +1550,5 @@ > +NS_IMETHODIMP > +nsXULWindow::ToggleAttributePersistence(bool aAllowPersist) > +{ > + nsCOMPtr<dom::Element> element = GetWindowDOMElement(); > + if (!element) if () { } @@ +1553,5 @@ > + nsCOMPtr<dom::Element> element = GetWindowDOMElement(); > + if (!element) > + return NS_ERROR_FAILURE; > + > + static NS_NAMED_LITERAL_STRING(kPersistBackupAttribute, "_persist"); Maybe move (and rename) this to the list of other attributes (right after #include section) ?

Comment on attachment 8649684 [details] [diff] [review] patch Review of attachment 8649684 [details] [diff] [review]: ----------------------------------------------------------------- Much much better, thanks!

We probably shouldn't persist fullscreen state at all, no matter it is for DOM fullscreen or fullscreen mode, because we always apply that state to the next window we create, which could be confusing and problematic.

Bug 1137009 part 1 - Do not persist xul window attributes when they change.

Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

Jan, could you please review these new patches?

Comment on attachment 8650792 [details] MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen. https://reviewboard.mozilla.org/r/16691/#review15999 ::: dom/xul/XULDocument.cpp:1002 (Diff revision 1) > + if (!aElement->IsXULElement(nsGkAtoms::window)) { So here you are just avoiding to persist attributes for all window elements and the other patch checks the full screen state. Is that correct ?

Comment on attachment 8650793 [details] MozReview Request: Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen. https://reviewboard.mozilla.org/r/16693/#review15991 ::: xpfe/appshell/nsXULWindow.cpp:1438 (Diff revision 1) > + if (nsPIDOMWindow* domWindow = mDocShell->GetWindow()) { This entire blog could be moved before GetWindowDOMElement() call I think. Also, instead of calling |mDocShell->GetWindow()| you could use GetWindowDOMWindow() which is used elsewhere.

s/blog/block

(In reply to Jan Varga [:janv] from comment #15) > Comment on attachment 8650792 [details] > MozReview Request: Bug 1137009 part 1 - Do not persist xul window attributes > when they change. > > https://reviewboard.mozilla.org/r/16691/#review15999 > > ::: dom/xul/XULDocument.cpp:1002 > (Diff revision 1) > > + if (!aElement->IsXULElement(nsGkAtoms::window)) { > > So here you are just avoiding to persist attributes for all window elements > and the other patch checks the full screen state. Is that correct ? Yes, exactly. So is there any concern about this patch?

Comment on attachment 8650792 [details] MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen. Bug 1137009 part 1 - Do not persist xul window attributes when they change.

Comment on attachment 8650793 [details] MozReview Request: Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen. Bug 1137009 part 2 - Do not persist xul window attributes when in fullscreen.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #18) > (In reply to Jan Varga [:janv] from comment #15) > > Comment on attachment 8650792 [details] > > MozReview Request: Bug 1137009 part 1 - Do not persist xul window attributes > > when they change. > > > > https://reviewboard.mozilla.org/r/16691/#review15999 > > > > ::: dom/xul/XULDocument.cpp:1002 > > (Diff revision 1) > > > + if (!aElement->IsXULElement(nsGkAtoms::window)) { > > > > So here you are just avoiding to persist attributes for all window elements > > and the other patch checks the full screen state. Is that correct ? > > Yes, exactly. So is there any concern about this patch? So, if I understand it correctly, you could have just one check in XULDocument::Persist() to avoid persisting attributes if we are in full screen mode. You have access to nsPIDOMWindow through mDocumentContainer, see XULDocument::GetWindowRoot(). However, it seems you decided to do it earlier. In that case, nsXULWindow::SavePersistentAttributes() changes look good, but I don't understand why you only check |aElement->IsXULElement(nsGkAtoms::window| in XULDocument::AttributeChanged(). You should check if we are in full screen mode, no ? Otherwise, one would expect that attributes are correctly persisted when the "persist" attribute changes on the "window" element.

(In reply to Jan Varga [:janv] from comment #21) > Otherwise, one would expect that attributes are correctly persisted when the > "persist" attribute changes on the "window" element. Hmm, that probably makes sense if the attributes may be changed in ways other than the related methods of nsXULWindow. OK, I'll update the patch. It should be in one single patch then.

Comment on attachment 8650792 [details] MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen. Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.

Comment on attachment 8650792 [details] MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen. https://reviewboard.mozilla.org/r/16691/#review16541 r=me

Comment on attachment 8650792 [details] MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen. https://reviewboard.mozilla.org/r/16691/#review16547

Backout for bc test failure https://hg.mozilla.org/integration/mozilla-inbound/rev/76826fe33431 I'm pretty confused what is the "persist" attribute supposed to do. Do we ever change that attribute on <window>? Do we ever want the window not to set the attributes when the window state changes?

Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. The attributes should eventually be saved via the AttributeChanged callback in XULDocument if one is specified in the "persist" attribute.

Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Sorry for requesting review again. This patchset now works fine on the try server.

(In reply to Xidorn Quan from comment #27) > I'm pretty confused what is the "persist" attribute supposed to do. It's basically a convenient storage system for attribute values. > Do we ever change that attribute on <window>? It's rare that we change the "persist" attribute's value itself. > Do we ever want the window not to set the attributes when the window state changes? It was probably originally done as an optimisation for windows which don't persist.

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. https://reviewboard.mozilla.org/r/18645/#review17207 I think someone else should take a look at this.

Comment on attachment 8658551 [details] MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen. https://reviewboard.mozilla.org/r/18647/#review17209 I think someone else should take a look at this.

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. The attributes should eventually be saved via the AttributeChanged callback in XULDocument if one is specified in the "persist" attribute.

Comment on attachment 8658551 [details] MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen. Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen.

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. Seems ok, but why do you need to remove the optimization?

(In reply to Neil Deakin from comment #37) > Comment on attachment 8658550 [details] > MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes > but do not persist them in nsXULWindow::SavePersistentAttributes() directly. > > Seems ok, but why do you need to remove the optimization? Do you mean the early return when "persist" attribute is empty? This removal is probably not necessary, but I think removing that matches the new name. I hope to handle all persistence in one place (in XULDocument::AttributeChanged()), so nsXULWindow::SavePersistentAttributes() no longer handles persistence directly, and thus it is weird to check "persist" attribute there. And is it common to have that attribute clear for optimization? It also doesn't seem correct to me that an attribute is not set at all merely because it is not listed in "persist".

I meant you removed the checks that don't assign the attributes when they don't exist in the persist list. The attributes are only used for the benefit of being persisted, so this patch adds these attributes unconditionally, including zorder which really isn't used at all.

I don't think the attributes are only used for being persisted. At least some tests check sizemode attribute for the current window state. See the failures here: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e9cb31373ec2 for the previous patchset, which shows what would happen if I do not set the attribute properly. If zorder is never used, and the attributes are only for being persisted, is there any reason why we shouldn't just remove that attribute?

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. > If zorder is never used, and the attributes are only for being persisted, is > there any reason why we shouldn't just remove that attribute? It looks like the zorder is only used for the help window in Seamonkey, but I don't think it is really needed, so I think we could just remove it. With that change, I think this patch is probably ok then. Would an alternative option be to have ShouldPersistAttribute explicitly look for the width/height/screenX/screenY attributes and not the other attributes and not have this patch?

(In reply to Neil Deakin from comment #41) > Comment on attachment 8658550 [details] > MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes > but do not persist them in nsXULWindow::SavePersistentAttributes() directly. > > > If zorder is never used, and the attributes are only for being persisted, is > > there any reason why we shouldn't just remove that attribute? > > It looks like the zorder is only used for the help window in Seamonkey, but > I don't think it is really needed, so I think we could just remove it. > > With that change, I think this patch is probably ok then. Would an > alternative option be to have ShouldPersistAttribute explicitly look for the > width/height/screenX/screenY attributes and not the other attributes and not > have this patch? The main motivation for this patch is to remove the explicit persisting in SavePersistentAttributes(), because XULDocument::AttributeChanged() will do that anyway. I don't want to duplicate the same logic in two places, especially given the logic in SavePersistentAttributes() is not seems correct to me. Thus I don't think doing any change in ShouldPersistAttribute() could remove the necessity of this patch. We have to do something in SavePersistentAttributes() anyway. Also, I feel that removing zlevel attribute is not directly related to this bug. Would you mind me landing this patchset as-is, and do the removal in a separate bug?

s/is not seems/does not seem

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #42) > The main motivation for this patch is to remove the explicit persisting in > SavePersistentAttributes(), because XULDocument::AttributeChanged() will do > that anyway. I don't want to duplicate the same logic in two places, > especially given the logic in SavePersistentAttributes() is not seems > correct to me. Yes, that part is good. > Also, I feel that removing zlevel attribute is not directly related to this > bug. Would you mind me landing this patchset as-is, and do the removal in a > separate bug? OK.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5322e5fa07efb3f62c6383c4a0bfa5ef588363 Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. r=enndeakin https://hg.mozilla.org/integration/mozilla-inbound/rev/2c77e4edc2a55facff5faa192180aa7b6d887129 Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen. r=enndeakin

(In reply to Neil Deakin from comment #44) > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #42) > > Also, I feel that removing zlevel attribute is not directly related to this > > bug. Would you mind me landing this patchset as-is, and do the removal in a > > separate bug? > > OK. Filed bug 1207883.

https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07 https://hg.mozilla.org/mozilla-central/rev/2c77e4edc2a5

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. Approval Request Comment [Feature/regressing bug #]: probably not a regression [User impact if declined]: meet various issues when using Fullscreen Mode or Fullscreen API, e.g. this bug, bug 946768, bug 1197416, and bug 1204332 [Describe test coverage new/current, TreeHerder]: not aware of any [Risks and why]: have moderate risk that window may not restore to the proper state after restart the browser if there is any case I did not catch [String/UUID change made/needed]: n/a

Actually for bug 946768, it is a regression since bug 1173930. For other bugs, I'm not aware of any bug caused them.

Is it normal after this bugfix that Nightly fails to restart in FS mode if it has been closed in FS mode previously?

(In reply to Loic from comment #51) > Is it normal after this bugfix that Nightly fails to restart in FS mode if > it has been closed in FS mode previously? Yes, it is intended, because starting in fullscreen mode causes issues on at least OS X 10.7+ (e.g. bug 1207674) and Windows 10 (bug 1204332).

Is it a permanent change?

(In reply to Loic from comment #53) > Is it a permanent change? Likely. At least I have no plan to reverse this change. If you think we should support restroing to fs mode, you may file a bug. But I don't think that's important to fix. AFAICS, currently only IE restores to fs mode, and that isn't even true for Edge which doesn't seem to have fs mode at all.

[Tracking Requested - why for this release]: see comment 48.

Comment on attachment 8658550 [details] MozReview Request: Bug 1137009 part 1 - Always save dirty window attributes but do not persist them in nsXULWindow::SavePersistentAttributes() directly. OK, let's uplift this, and keep an eye open for fullscreen or window attribute issues in nightly and aurora.

do we want to uplift both parts or just part 1 ?

Part 1 on its own doesn't fix this issue. We should uplift both parts.

Comment on attachment 8658551 [details] MozReview Request: Bug 1137009 part 2 - Do not persist attributes of xul window if the window is in fullscreen. Let's not forget part 2 for aurora uplift

Sorry, I was in transit and got interrupted :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e06450a934c https://hg.mozilla.org/releases/mozilla-aurora/rev/735ebc504963

Would you like to back this out since it's causing regressions, see bug 1211344? (Also see: Bug 1211344 comment #10). We don't know which other XUL panels are affected, maybe in some add-ons. I think the rule is: If it causes regressions, it gets backed out, at least from Aurora. https://hg.mozilla.org/releases/mozilla-aurora/rev/7e06450a934c https://hg.mozilla.org/releases/mozilla-aurora/rev/735ebc504963 https://hg.mozilla.org/mozilla-central/rev/7a5322e5fa07 https://hg.mozilla.org/mozilla-central/rev/2c77e4edc2a5

Before we actually back out this bug, we should not reopen it. Making this bug depends on the regression bug is enough for tracking.

OK, so I gather you want to leave this on aurora and fix the regression in bug 1211344. Is this a better approach for this particular set of changes? Or is it something you could fix by backing out the patches here and reworking them?

I'll submit a patch in bug 1211344 later today which I believe should be upliftable to aurora.

Please backout this changeset in central and aurora. Also note that, backing out this bug would need backing out bug 1211344 first.

Both patches backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/726a356eef02 https://hg.mozilla.org/releases/mozilla-aurora/rev/38cffa3439e4 Unsure exactly what the flags should be set back to, hopefully this is right. Leaving 1211344 closed since it'll be dealt with here.

Bug 1137009 - Do not persist xul window attributes when in fullscreen.

It should be less risky this time...

Comment on attachment 8674614 [details] MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen. > if (!aElement->IsXULElement(nsGkAtoms::window) && !persist.IsEmpty()) { What if someone wants to persist an attribute that isn't one that XULWindow handles?

That attribute would never be persisted. But do we really have code does that? If that really causes any regression later, I can extend the condition here to exclude only the attributes we handle in XULWindow. If you do think we should be careful about this now, I can do that in this bug directly as well.

Comment on attachment 8674614 [details] MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen. Bug 1137009 - Do not persist xul window attributes when in fullscreen.

OK, now I do exclude only the attributes we do handle in XULWindow.

Comment on attachment 8674614 [details] MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen. I don't know if there's any other attributes that get used for window, but it would good to be safe considering that this bug has had some regressions already.

Yeah, I agree.

https://hg.mozilla.org/integration/mozilla-inbound/rev/437b3c3ef56c48b25e432f8fd61afdad2c605b86 Bug 1137009 - Do not persist xul window attributes when in fullscreen. r=enndeakin

Comment on attachment 8674614 [details] MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen. Approval Request Comment [Feature/regressing bug #]: probably not a regression [User impact if declined]: meet various issues when using Fullscreen Mode or Fullscreen API, e.g. this bug, bug 946768, bug 1197416, and bug 1204332 [Describe test coverage new/current, TreeHerder]: not aware of any [Risks and why]: relatively low risk, because we tried harder to avoid changing unrelated behavior in this new patch [String/UUID change made/needed]: n/a

FWIW: The issue reported in bug 1211344 is *not* being observed with this new patch, so it's all good. I trust you tested bug 1214064 (which will also receive a test).

Comment on attachment 8674614 [details] MozReview Request: Bug 1137009 - Do not persist xul window attributes when in fullscreen. OK, let's try uplift one more time. But, if this doesn't work, during the last week of aurora (next week) we might want to just keep this to ride on 44.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #83) > Comment on attachment 8674614 [details] > MozReview Request: Bug 1137009 - Do not persist xul window attributes when > in fullscreen. > > OK, let's try uplift one more time. But, if this doesn't work, during the > last week of aurora (next week) we might want to just keep this to ride on > 44. Thanks.

I have successfully reproduced this bug on Firefox nightly version 33.0a1 according to (2015-02-25) It's fixed and verified on Firefox Beta and Latest Nightly 45.0a1 Firefox Beta Build ID 20151112144305 User Agent Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0 Tested OS-32bitWindows 8.1

It's fixed and verified ?On Latest Developer Edition (In reply to Saheda Reza [:Antora] from comment #86) > I have successfully reproduced this bug on Firefox nightly version 33.0a1 > according to (2015-02-25) > > It's fixed and verified on Firefox Beta and Latest Developer Edition > Firefox Beta > Build ID 20151112144305 > User Agent Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0 > Latest Developer Edition Build ID 20151113004115 User Agent Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0 > Tested OS-32bitWindows 8.1