Closed Bug 1168028 Opened 5 years ago Closed 5 years ago

Revert fullscreen state after window finish changing if it causes that

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files, 1 obsolete file)

This is the second half of delaying fullscreen state change when the window needs to resize. The first half is bug 1161802 which is for delaying the state change for entering.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Attachment #8612683 - Flags: review?(dao)
Attachment #8612683 - Flags: review?(bugs)
Blocks: 1170369
Comment on attachment 8612683 [details] [diff] [review]
patch

>+  if (exitingFullscreen) {
>+    // If we are fully exiting fullscreen, don't touch anything here,
>+    // just wait for the window to get out from fullscreen first.
>+    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+      (new AsyncEventDispatcher(
>+        this, NS_LITERAL_STRING("MozDOMFullscreen:Exit"),
>+        /* Bubbles */ true, /* ChromeOnly */ true))->PostDOMEvent();
>+    } else {
>+      SetWindowFullScreen(this, false);
>+    }
(I still wish we'd find some way to handle this without explicit process type checks)
Attachment #8612683 - Flags: review?(bugs) → review+
A potential solution would be to copy the code in nsGlobalWindow::SetFullScreenInternal() which checks whether the top level window is a chrome window.
Attachment #8612683 - Flags: review?(dao) → review+
I thought this change should be trivial and shouldn't break anything, but it proves I was wrong. I should have pushed a try build before pushing to m-i.

Anyway, this is the prerequisite patch which ensures even if we are exiting to fullscreen mode, we still exit DOM fullscreen properly.

Sorry for new review request, smaug.
Flags: needinfo?(quanxunzhen)
Attachment #8620806 - Flags: review?(bugs)
The only change to this patch is that three ok() in file_fullscreen-api.html are removed, since they are no longer true.

This change is for fixing M2 failure in this try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2f5e2633e47
Attachment #8612683 - Attachment is obsolete: true
Attachment #8620809 - Flags: review?(bugs)
Attachment #8620806 - Flags: review?(bugs) → review+
Comment on attachment 8620809 [details] [diff] [review]
patch 2 - exit fullscreen state after window changed, r=dao

Hmm, so mozFullScreen reflects the actual value. Fine, but this may break pages, though apparently mozFullScreen is a gecko-ism which we should get rid of at some point.
Attachment #8620809 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #8)
> Comment on attachment 8620809 [details] [diff] [review]
> patch 2 - exit fullscreen state after window changed, r=dao
> 
> Hmm, so mozFullScreen reflects the actual value. Fine, but this may break
> pages, though apparently mozFullScreen is a gecko-ism which we should get
> rid of at some point.

It may, but I think that meets what the spec says, and pages should generally listen to fullscreenchange event instead of check the property synchronously.

Anyway, I don't have any data, but if bug 1161802 doesn't break pages, I believe this bug shouldn't either.
No longer blocks: 1174323
Depends on: 1174323
May I close this bug with fixed?

I really have no idea about that intermittent, but that shouldn't affect the fact that this bug has been fixed, should it?
Flags: needinfo?(wkocher)
Yes. Not entirely sure why this didn't automatically get marked fixed when those commits got merged to m-c...
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(wkocher)
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.