Video fullscreen confuses itself with F11 fullscreen state

RESOLVED FIXED in Firefox 43

Status

()

Core
Widget
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: xidorn)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ fixed, firefox44+ fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 6 obsolete attachments)

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?
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 3

3 years ago
No. Thanks for reminding me.

I think we have various issues when a page is closed in fullscreen state.
Blocks: 1121280
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 4

3 years ago
On Mac, the window state is also confused if Firefox is exited via Cmd-Q when on video fullscreen.
Component: Layout → Widget
OS: Windows 7 → All
Hardware: x86_64 → All
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 6

3 years ago
Actually the STR is not that precise. It may need time between entering fullscreen and closing the window to reproduce this issue.
(Assignee)

Comment 7

3 years ago
Created attachment 8647979 [details] [diff] [review]
patch

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.
Assignee: nobody → quanxunzhen
Attachment #8647979 - Flags: review?(bugs)
Attachment #8647979 - Flags: review?(Jan.Varga)

Updated

3 years ago
Flags: needinfo?(bugs)

Comment 8

3 years ago
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) ?
Attachment #8647979 - Flags: review?(Jan.Varga)
(Assignee)

Updated

3 years ago
Blocks: 1180574
(Assignee)

Comment 9

3 years ago
Created attachment 8649684 [details] [diff] [review]
patch
Attachment #8647979 - Attachment is obsolete: true
Attachment #8647979 - Flags: review?(bugs)
Attachment #8649684 - Flags: review?(bugs)
Attachment #8649684 - Flags: review?(Jan.Varga)

Comment 10

3 years ago
Comment on attachment 8649684 [details] [diff] [review]
patch

Review of attachment 8649684 [details] [diff] [review]:
-----------------------------------------------------------------

Much much better, thanks!
Attachment #8649684 - Flags: review?(Jan.Varga) → review+
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8649684 - Attachment is obsolete: true
Attachment #8649684 - Flags: review?(bugs)
(Assignee)

Comment 12

3 years ago
Created 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.
Attachment #8650792 - Flags: review?(Jan.Varga)
(Assignee)

Comment 13

3 years ago
Created 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.
Attachment #8650793 - Flags: review?(Jan.Varga)
(Assignee)

Updated

3 years ago
Blocks: 1197416
(Assignee)

Comment 14

3 years ago
Jan, could you please review these new patches?
Flags: needinfo?(Jan.Varga)
(Assignee)

Updated

3 years ago
Blocks: 946768

Updated

3 years ago
Attachment #8650792 - Flags: review?(Jan.Varga)

Comment 15

3 years ago
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 16

3 years ago
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.
Attachment #8650793 - Flags: review?(Jan.Varga)

Comment 17

3 years ago
s/blog/block
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 18

3 years ago
(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?
(Assignee)

Updated

3 years ago
Attachment #8650792 - Flags: review?(Jan.Varga)
(Assignee)

Comment 19

3 years ago
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.
(Assignee)

Comment 20

3 years ago
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.
Attachment #8650793 - Flags: review?(Jan.Varga)

Comment 21

3 years ago
(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.

Updated

3 years ago
Attachment #8650792 - Flags: review?(Jan.Varga)

Updated

3 years ago
Attachment #8650793 - Flags: review?(Jan.Varga)
(Assignee)

Comment 22

3 years ago
(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.
(Assignee)

Comment 23

3 years ago
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.
Attachment #8650792 - Attachment description: MozReview Request: Bug 1137009 part 1 - Do not persist xul window attributes when they change. → MozReview Request: Bug 1137009 - Do not persist attributes of xul window if the window is in fullscreen.
Attachment #8650792 - Flags: review?(Jan.Varga)
(Assignee)

Updated

3 years ago
Attachment #8650793 - Attachment is obsolete: true

Comment 24

3 years ago
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
Attachment #8650792 - Flags: review?(Jan.Varga)

Comment 25

3 years ago
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
Attachment #8650792 - Flags: review+
(Assignee)

Comment 27

3 years ago
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?
(Assignee)

Updated

3 years ago
Attachment #8650792 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created 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.
Attachment #8658550 - Flags: review?(Jan.Varga)
(Assignee)

Comment 30

3 years ago
Created 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.
Attachment #8658551 - Flags: review?(Jan.Varga)
(Assignee)

Comment 31

3 years ago
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 33

3 years ago
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.
Attachment #8658550 - Flags: review?(Jan.Varga)

Comment 34

3 years ago
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.
Attachment #8658551 - Flags: review?(Jan.Varga)
(Assignee)

Comment 35

3 years ago
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.
(Assignee)

Comment 36

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8658550 - Flags: review?(enndeakin)
(Assignee)

Updated

3 years ago
Attachment #8658551 - Flags: review?(enndeakin)
(Assignee)

Updated

3 years ago
Blocks: 1204332

Updated

3 years ago
Attachment #8658551 - Flags: review?(enndeakin) → review+

Comment 37

3 years ago
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?
(Assignee)

Comment 38

3 years ago
(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".

Comment 39

3 years ago
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.
(Assignee)

Comment 40

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(enndeakin)

Comment 41

3 years ago
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?
Flags: needinfo?(enndeakin)
Attachment #8658550 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 42

3 years ago
(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?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 43

3 years ago
s/is not seems/does not seem

Comment 44

3 years ago
(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.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 45

3 years ago
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
(Assignee)

Comment 46

3 years ago
(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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Comment 48

3 years ago
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
Attachment #8658550 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8658551 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 49

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

Updated

3 years ago
Blocks: 1207674
(Assignee)

Updated

3 years ago
Blocks: 1197642

Updated

3 years ago
Duplicate of this bug: 1205433

Comment 51

3 years ago
Is it normal after this bugfix that Nightly fails to restart in FS mode if it has been closed in FS mode previously?
(Assignee)

Comment 52

3 years ago
(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).

Comment 53

3 years ago
Is it a permanent change?
(Assignee)

Comment 54

3 years ago
(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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1203775

Comment 56

3 years ago
[Tracking Requested - why for this release]: see comment 48.
status-firefox43: --- → affected
tracking-firefox43: --- → ?
(Assignee)

Updated

3 years ago
Blocks: 1209831
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.
Attachment #8658550 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox43: ? → +
tracking-firefox44: --- → +
do we want to uplift both parts or just part 1 ?
Flags: needinfo?(lhenry)
(Assignee)

Comment 59

3 years ago
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
Flags: needinfo?(lhenry)
Attachment #8658551 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Sorry, I was in transit and got interrupted :)
Duplicate of this bug: 1207674
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
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(enndeakin)
Flags: needinfo?(Jan.Varga)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 65

3 years ago
Before we actually back out this bug, we should not reopen it.

Making this bug depends on the regression bug is enough for tracking.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Depends on: 1211344
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(enndeakin)
Flags: needinfo?(Jan.Varga)
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?
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 67

3 years ago
I'll submit a patch in bug 1211344 later today which I believe should be upliftable to aurora.
Flags: needinfo?(quanxunzhen)

Updated

3 years ago
Blocks: 1174518
(Assignee)

Comment 68

3 years ago
Please backout this changeset in central and aurora. Also note that, backing out this bug would need backing out bug 1211344 first.
Flags: needinfo?(wkocher)
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.
Status: RESOLVED → REOPENED
status-firefox43: fixed → affected
status-firefox44: fixed → affected
Flags: needinfo?(wkocher)
Resolution: FIXED → ---
(Assignee)

Updated

3 years ago
Attachment #8658550 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8658551 - Attachment is obsolete: true
(Assignee)

Comment 70

3 years ago
Created 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.
Attachment #8674614 - Flags: review?(enndeakin)
(Assignee)

Comment 71

3 years ago
It should be less risky this time...
(Assignee)

Updated

3 years ago
Blocks: 1211529

Comment 73

3 years ago
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?
(Assignee)

Comment 74

3 years ago
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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(enndeakin)
(Assignee)

Comment 75

3 years ago
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.
(Assignee)

Comment 76

3 years ago
OK, now I do exclude only the attributes we do handle in XULWindow.
Flags: needinfo?(enndeakin)

Comment 77

3 years ago
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.
Attachment #8674614 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 78

3 years ago
Yeah, I agree.
(Assignee)

Comment 79

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/437b3c3ef56c48b25e432f8fd61afdad2c605b86
Bug 1137009 - Do not persist xul window attributes when in fullscreen. r=enndeakin
https://hg.mozilla.org/mozilla-central/rev/437b3c3ef56c
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 81

3 years ago
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
Attachment #8674614 - Flags: approval-mozilla-aurora?
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.
Attachment #8674614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 84

3 years ago
(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
QA Whiteboard: [testday-20151113]
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

Updated

2 years ago
Depends on: 1264968

Updated

2 years ago
Depends on: 1271324
(Assignee)

Updated

2 years ago
Duplicate of this bug: 909439
You need to log in before you can comment on or make changes to this bug.