721498 - Should use remote timestamp when raising windows

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[otaylor]

Attachment: Patch against Firefox 9

In the gtk2 backend nsWindow::raise does gtk_window_present() which uses a timestamp of CurrentTime==0. This is the case even if we're raising after being asked to open an URL in a new tab by the remote protocol.

A proper "event timestamp" is passed over the remote protocol, but is thrown away because we also have a "desktop startup ID". The "desktop startup ID" is useful when presenting a new window and includes the timestamp information, but isn't something we can use when we're raising an existing window.

The net effect of this is that if in your desktop you click on a link in (say) a terminal or mail client, the effect will be:

 - If Firefox *is not* already open, the Firefox is opened using startup ID, and pops up on top of the current window.

 - If Firefox *is* already open, then Firefox asks to raise itself without a timestamp, gets "focus stealing prevented", causing the desktop to flash it in the taskbar or show a notification. The user has to make a second action to get to the URL they wanted to go to.

The attached patch uses the timestamp from the remote protocol for window raising. There's obviously some concern that some users will be attached to the old behavior of opening in the background without stealing the focus, but it doesn't make any sense to me that depends on whether Firefox is running already or not. 

At least in GNOME we want opening a web browser link to be just like clicking on an application launcher -  but if the result happens immediately before the user does anything else, then the user shouldn't have to *also* switch focus. (https://bugzilla.gnome.org/show_bug.cgi?id=641532) [If the result happens some time in the future, it shouldn't steal the focus - that's what the passed timestamp is used for]

I'll attach patches against Firefox 9 and Firefox 10 that implement what I think is the correct timestamp handling.

Comment 1

[otaylor]

Attachment: Patch against Firefox 10

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 591905 [details] [diff] [review]
Patch against Firefox 10

Karl, can you check it please?

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 591905 [details] [diff] [review]
Patch against Firefox 10

Thanks.  BTW, I filed bug 726479 for updating this code to use GTK instead of libstartupnotification.

Comment 5

[Karl Tomlinson (:karlt)]

Thinking about this more, I don't whether we can be sure that a remote request that calls SetDesktopStartupIDOrTimestamp will end up calling SetFocus or Show.

Using the most recent of the remote timestamp and gdk_x11_display_get_user_time would protect against using the old remote timestamp after a subsequent button press.

Comment 6

[otaylor]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Thinking about this more, I don't whether we can be sure that a remote
> request that calls SetDesktopStartupIDOrTimestamp will end up calling
> SetFocus or Show.
> 
> Using the most recent of the remote timestamp and
> gdk_x11_display_get_user_time would protect against using the old remote
> timestamp after a subsequent button press.

I'm not really sure about the details of how the remote protocol interacts with this code, so if you say that it might not end up with SetFocus or Show, I have to believe you. :-) (but of course, that problem would have been there previously for Show independent of this patch)

 * I don't think it's appropriate to use gdk_x11_display_get_user_time() - that means that activity from one browser window leaks onto another browser window - so any SetFocus or Show that triggered because of network activity or JS activity could steal focus away from the browser window that the user is working in. There is no gdk_x11_window_get_user_time(), though GTK+ tracks that internally - you could record a timestamp yourself for the window from button presses and key presses. Or if you are getting the events through GTK+ event delivery (forget how it works if I ever knew), you could use gtk_get_current_event_time() to check for the timestamp of a *current* event, though that then counts on SetFocus/Show being done immediately in response to an event, and not in a timeout or other delayed callback.

 * Since X timestamps are 32-bit millisecond values that wrap every 40 days, it's non-trivial to compare for newer. Probably the thing to do is to check whether the saved timestamp is within last_even_time to last_event_time + 1 day, and use it if so, otherwise not.

Comment 7

[Karl Tomlinson (:karlt)]

Thanks for your comments.  The problem is hard to solve in all situations until we fix Gecko to pass the timestamp through the focus request code, and that could involve major changes.

Using an client-wide timestamp is perhaps not always the best.  It is kind of relying on the application to do its own focus-stealing prevention.  There is some of that in Gecko, but I'm not confident it always gets it right.
Often enough though user input in one window should cause another window to receive focus.  Interestingly _NET_ACTIVE_WINDOW documentation says 
"The timestamp is Client's last user activity timestamp" rather than a window-specific timestamp, though the timestamp of the event the initiated the focus request would seem better.
http://standards.freedesktop.org/wm-spec/wm-spec-1.4.html#id2559547

I'm not keen on the 1 day timeframe as I think the conflict is more likely to happen before a day expires and so that is not much of a win.

gdk_x11_display_get_user_time() is what gtk_window_present_with_time() uses when the passed GDK_CURRENT_TIME, so using the newer of that and the remote timestamp would be merely using the remote timestamp when newer than what gtk_window_present would use.

However, I can see that using the newer timestamp wouldn't give the best result in the following situation.

1. User initiates remote request (from another client)
2. While that is being sent/processed, user focuses another browser window and starts interacting.
3. Browser finishes processing the remote request but Gecko focus code is not clever enough to know that this was an old request and so SetFocus is called.

In this situation, using the old remote timestamp might let the window manager save us from an out of date request.

As you say, the problem in comment 5 already existed in Show, so it may not be too much worse to extend it to SetFocus, and if it is a problem we would need to fix it in both places.

Let's go with the patch as is, thanks.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: patch for m-c

Comment 9

[otaylor]

Hmm, my knowledge of what gtk_window_present_with_time() does was out of date since 2005 when it was changed to use gdk_x11_display_get_user_time()...

I think the wm-spec language is just a bit imprecise there, and doesn't really take into account a complex app where there might be different parts of the application that act independently, but there's a strong implication of the algorithm:

 Compare the timestamp of the activate request with the _NET_WM_USER_TIME of the currently active window. If older, don't take focus. If newer, take focus.

The timestamp for the activate request should be the timestamp (as best as we can determine) of the user event that triggered the new window. That is actually unlikely to be the newest timestamp in the window itself, since if the window is being interacted with already, why would it try to take focus.

In the web browser context, I guess ideally timestamps should be constrained by the source of the request - the timestamp of a request to show or activate a window should be the timestamp of the last event visible to the set of JS code that is making the request to activate. One browser tab should not be able to steal focus once the user is switched to a different tab. Indeed, toplevel windows probably aren't very relevant.

The one-day window was about comparing timestamps - if you naively compare two timestamps, then an old stale timestamp that doesn't get updated suddenly starts looking like a newer timestamp, and messing things up. So, the point is that if the focus timestamp is a few hours newer than the user time, it's probably a newer timestamp, but if it's 20 days newer than the user time, it's probably old - making the assumption that the user time is much more frequently updated than interactions with the remote protocol.

But anyways, if you are comfortable with the simple patch, I think that's a good way to go - the only downside is if there is some remote protocol codepath that doesn't end up triggering a SetFocus or Show call, then a single subsequent SetFocus could get an old timestamp and be focus-stealing denied.

Comment 10

[Karl Tomlinson (:karlt)]

This passed our debug tests
https://tbpl.mozilla.org/?tree=Try&rev=a3cd868d8d0c

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/69cb70c3a1e9

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/69cb70c3a1e9

Comment 13

[rnovacek]

I personally don't like this "fix". Fortunately, the old behavior can be simulated by setting "browser.tabs.loadDivertedInBackground" to true in about:config.