1254448 - Ensure window size is correct on Linux when fullscreenchange event dispatches for Fullscreen API calls

Status: NEW

(Core :: Widget: Gtk, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

It seems on Linux the window size is not changed when fullscreenchange event is dispatched, which should be fixed.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

From bug 1240643 comment 15 it seems this problem only occurs on e10s.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

No, this actually occurs on non-e10s as well. I have had patches for this, but need to investigate some failures tomorrow.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

fullscreenchange events triggered by exiting fullscreen unexpectedly may need further work to ensure this, and that may have relatively lower priority than this one.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1254448 part 1 - Invoke fullscreen change callback after size gets changed.

Review commit: https://reviewboard.mozilla.org/r/39419/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39419/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1254448 part 2 - Add new test for checking window dimension.

Review commit: https://reviewboard.mozilla.org/r/39421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39421/

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03adcd21166&filter-classifiedState=unclassified
(Hidden jobs are just empty jobs due to using tag to restrict tests running)

Mostly fine. Even better, it seems the two intermittents bug 1239932 and bug 931445 also go away here (but this only runs a small set of the tests, so it may not reflect the real case if previous tests could affect the state of those tests).

There is one "Assertion count 2 is greater than expected range 0-0 assertions" failure on Linux debug M(c2), which I'll continue investigating a bit. It doesn't seem to be a big issue, though.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

It also seems bug 888164 goes away as well per https://treeherder.mozilla.org/#/jobs?repo=try&revision=2923bf1cc720

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #7)
> It also seems bug 888164 goes away as well per
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2923bf1cc720

The assertions disappear in this try push... hmmmm... Anyway, let's do a full mochitest try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86708d8b23ef

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8729399 [details]
MozReview Request: Bug 1254448 part 2 - Add new test for checking window dimension.

https://reviewboard.mozilla.org/r/39421/#review36135

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/39419/#review36267

::: widget/gtk/nsWindow.h:432
(Diff revision 1)
> +    // Fullscreen change has four steps:
> +    // 1. the change is started via MakeFullscreen;
> +    // 2. the window state gets changed in OnWindowStateEvent;
> +    // 3. the position of the window gets changed in OnConfigureEvent;
> +    // 4. the size of the window gets changed in OnSizeAllocate.
> +    // We want to continue the fullscreen change only after all of them
> +    // finish. However, the order of step 2 and 3 is not consistent
> +    // across machines. Some trigger step 3 first, others do step 2
> +    // first instead. It seems OnSizeAllocate is always the last signal
> +    // in these steps, and thus we invoke the callback there. However,
> +    // actually this is not documented anywhere, so this assumption
> +    // could be broken in some cases as well.

Errrr... it seems step 2, 3, 4 can happen in completely arbitrary order... I have a new idea then...

Comment 11

[Karl Tomlinson (:karlt)]

I'm uncomfortable with trying to enforce this.
If all three events are received, then I'd expect OnSizeAllocate() to be called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at any time wrt those.
However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent() will be called.

Is this causing real problems, or is it just that some tests are not waiting for the size change?

Size changes need to be handled anyway regardless of window state.

GDK delays requesting a draw until there are no events remaining in the queue, so drawing in mid-transition would usually be rare.  OTMC likely defeats that logic, but doesn't the fullscreen animation usually cover the window during transition?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> I'm uncomfortable with trying to enforce this.
> If all three events are received, then I'd expect OnSizeAllocate() to be
> called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at
> any time wrt those.
> However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent()
> will be called.
> 
> Is this causing real problems, or is it just that some tests are not waiting
> for the size change?

It is a real issue, since we want to ensure our size being stable after we dispatch fullscreenchange event, so that the page can rely on the size at that moment and we do a big reflow rather than doing two reflows for resize and fullscreen separately.

> Size changes need to be handled anyway regardless of window state.

Yes, but authors may want to use the size of elements and viewport in fullscreenchange event handler to do something. Getting any size there would make an additional reflow.

> GDK delays requesting a draw until there are no events remaining in the
> queue, so drawing in mid-transition would usually be rare.  OTMC likely
> defeats that logic, but doesn't the fullscreen animation usually cover the
> window during transition?

Yes, but resize reflow + fullscreen reflow could be slow. It could take several hundreds of ms for each full reflow, and we cannot cover things for that long.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> I'm uncomfortable with trying to enforce this.
> If all three events are received, then I'd expect OnSizeAllocate() to be
> called after OnConfigureEvent(), but the OnWindowStateEvent() may arrive at
> any time wrt those.
> However, there is no guarantee that OnSizeAllocate() or OnConfigureEvent()
> will be called.

In which case those two will not be called? Probably the window can occupy the whole screen without being in fullscreen state? I guess comparing bounds against screen rect and original bounds is even more unreliable... I have no idea how can I fix this then, if those signals are not reliable... Any suggestion?

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

If GDK requests a draw only until there is no event in the queue, can we probably handle that at the very beginning of the first OnExposeEvent after the window state changes?

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #13)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> In which case those two will not be called? Probably the window can occupy
> the whole screen without being in fullscreen state?

Yes, a maximized window may be fullscreen under some window managers.
Conversely, some window managers may not make the window cover the whole screen when switching to fullscreen mode.

... I have no
> idea how can I fix this then, if those signals are not reliable... Any
> suggestion?

I don't have any good ideas, which is why it seems easier to accept possible
double reflow.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #14)
> If GDK requests a draw only until there is no event in the queue, can we
> probably handle that at the very beginning of the first OnExposeEvent after
> the window state changes?

That's essentially how things used to work.  OnExposeEvent would force the
needed reflow before paint.  That had some problems, but we usually survived.
JS would run in response to the expose signal, which could run other events.
If there were too many input events, the expose event would never arrive
because we stayed busy.

With OMTC, I guess we now have the refresh driver forcing reflow before
OnExposeEvent requests a redraw?  If there are pending resizes when that
reflow is forced, then we have the same problem with performing an unnecessary
reflow.  The issue doesn't seem specific to fullscreen changes.

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #12)
> It is a real issue, since we want to ensure our size being stable after we
> dispatch fullscreenchange event, so that the page can rely on the size at
> that moment and we do a big reflow rather than doing two reflows for resize
> and fullscreen separately.

What forces a reflow?

Is there a reason why handling the size change before the fullscreen change is
better than the reverse?

The page may reorganize in response to a size change before a fullscreen change.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

The order of resize and fullscreenchange events is not particularly important, but any intermittent state of the page not being observable inside those event handlers is important. If we expose the intermittent state, we would likely waste a reflow, because functions like Element.getClientBoundingRect(), or even Window.innerWidth need a flush.

We currently have code to freeze the refresh driver and block any resize request during fullscreen change. This flag is set before calling MakeFullscreen, and unset inside FullscreenChanged callback, which means resize happens before fullscreen change finishes would not trigger any resize reflow itself, so that the intermittent state is not observable.

I want this to be consistent on GTK+ like on other platforms.

Other platforms ensure this actually because they call Resize() to enter fullscreen, which change mBounds directly. So they have the correct geometry synchronously. But I guess it is not quite doable with GTK+, and we would need to play with the signals anyway...

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

No... even OnExposeEvent doesn't work... I have no idea what can I do then...

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

I want to disable all fullscreen tests which have intermittent on Linux then... I totally have no idea how can I fix it...

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

From this try run result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d22718c9fd8d&filter-classifiedState=unclassified&selectedJob=18017107

It seems:
1. expose-event could happen before size-allocate after window-state-event [Linux x64 asan M-e10s(2)]
2. expose-event may not happen at all after window-state-event [M(bc7) M-e10s(bc7)]

I have no idea what can I do then... karlt, suggestions?

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

It seems expose-event not happening after window-state-even happens on other tests as well, like M(2) on Linux opt/debug and Linux64 ASAN.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #19)
> From this try run result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d22718c9fd8d&filter-
> classifiedState=unclassified&selectedJob=18017107
> 
> It seems:
> 1. expose-event could happen before size-allocate after window-state-event
> [Linux x64 asan M-e10s(2)]
> 2. expose-event may not happen at all after window-state-event [M(bc7)
> M-e10s(bc7)]
> 
> I have no idea what can I do then... karlt, suggestions?

My best idea is to use gdk_frame_clock_request_phase() [1] on receipt of window state change and suppress refresh driver ticks until that phase is received.

That way, we know that a signal will be received (as long as the window is not destroyed) and I'm assuming we can pick a phase that will happen after the size change, if there is one pending.  If the window manager is performing an animation, then it may take an unknown length of time for the size change to be received, and so the size change could still be after the frame clock phase signal.

The idea needs some investigation to see whether it is sane, and it is only available from gtk 3.8.  Tests run against 3.4, which does not have this function.

I'm sceptical re whether it is worth the effort.

https://developer.gnome.org/gdk3/stable/GdkFrameClock.html#gdk-frame-clock-request-phase

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

It's worth because we want to ensure the consistency across platforms, and we want to fix all linux-specific intermittents related to fullscreen. Disabling them means we lose coverage of all pointerlock tests and a large part of fullscreen tests for Linux.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Filed a bug for GTK+ to see whether their developers have any suggestions.

Comment 24

[daxpedda]

I'm not entirely sure if I just stumbled on this bug or if it's unrelated, but when using Element.requestFullscreen() and having the developer tools open, the reported window size, e.g. Window.innerHeight is incorrect.

I made a minimal example demonstrating this:

<!DOCTYPE html>
<html>
<head>
	<script type="module">
		let box = document.getElementById('box');
		box.addEventListener('fullscreenchange', () => {
			if (document.fullscreenElement == box) {
				let height = window.innerHeight;
				setTimeout(() => {
					if (height != window.innerHeight) {
						console.log(
							'`fullscreenchange` reported the wrong size: ',
							height,
							' (incorrect) - ',
							window.innerHeight,
							' (correct)',
						);
					}
				}, 250);
			}
		});

		window.addEventListener('keydown', (e) => {
			if (e.code == 'KeyF') {
				if (document.fullscreenElement == box) {
					document.exitFullscreen();
				} else {
					box.requestFullscreen();
				}
			}
		});
	</script>
	<style>
		#box {
			width: 100px;
			height: 100px;
			background-color: red;
		}
	</style>
</head>
<body>
	<div id="box"></div>
</body>
</html>

To test this you will have to have your developer tools embedded at the bottom or top of your browser window. Now if you press "F", the red box will be made fullscreen. But even though this will only partly cover your screen, because the developer cover the other part of the screen, it will report the full height in Window.innerHeight during fullscreenchange. But just 250ms Window.innerHeight will report the correct size.

This is done on Linux X11. I also tried this in Firefox Nightly, same behavior.

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

(In reply to daxpedda from comment #24)

To test this you will have to have your developer tools embedded at the bottom or top of your browser window. Now if you press "F", the red box will be made fullscreen. But even though this will only partly cover your screen, because the developer cover the other part of the screen, it will report the full height in Window.innerHeight during fullscreenchange. But just 250ms Window.innerHeight will report the correct size.

This is a separate thing.

We optimize fullscreen change by aggressively reporting the full screen dimensions in advance so that layout change can start early and doesn't need to kick in multiple times, even if the window hasn't really finished resizing. It was done in https://hg.mozilla.org/mozilla-central/rev/ef599e365a7b.

This is, however, based on the assumption that we are actually going to occupy the full screen, which is the most common cases. However, in your case, the content is not occupying the whole screen, and thus the assumption is wrong, and the correct dimensions come after we unblock the resize when the fullscreen state is settled.

I don't think we need to handle that case, as it's not something that matters to a normal user of fullscreen.