330006 - switching tabs and quickly switching virtual desktops makes window move desktops

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've been noticing Firefox windows end up on different virtual desktops quite often the past few weeks, and I had a nagging suspicion it was related to the changes for bug 295447 (which also caused bug 324348).  I finally had a look at the first version of that patch and the discussion there and realized the steps to reproduce, which are something I do reasonably often:
 1. switch tabs using Ctrl-PgUp or Ctrl-PgDn
 2. really quickly, before the switch happens, switch virtual desktops using
    Ctrl-Arrow (that may be a shortcut I configured rather than the default
    GNOME one)
It's pretty easy for me to do this fast enough that I'm on a different virtual desktop before the tab switch happens.

I really don't like the look of the patch in bug 295447, for two reasons:
 * focus is already overloaded enough; if we need a method to do a raise/present, it should be a separate method.  That the DOM 0 backwards compatibility requires that window.focus() do a raise/present in some cases is IMO a quirk that should not be propagated all the way down into the widget code.  If the DOM code requires a raise/present, it should do that on its own.
 * we shouldn't do a raise/present when switching tabs.
Why do we want that patch -- why should it not be backed out?

Note that I haven't actually yet tested that that patch is the cause of my problems, but I'd be quite surprised if it isn't.  I'll try to do that in the morning.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Sorry, the keyboard shortcuts for switching virtual desktops are, for me at least, Ctrl-Shift-Arrows.  And I know that Ctrl-Alt-Shift-Arrows moves the focused window *and* the user between virtual desktops, but I'm confident I don't have the Alt key down; it's involved in neither shortcut.)

Comment 2

[Ginn Chen]

Hi dbaron,

I also dislike the change of bug 295447 since it caused some regressions.

I don't want to back it out because it solved https://bugzilla.mozilla.org/show_bug.cgi?id=260560#c37, which is a security issue. (It seems few people care about it.)

Also raising a window without focus is ridiculous. It doesn't happen with other applications, and doesn't happen on Windows or Mac OS X.

Comment 3

[Ginn Chen]

Confirmed. change of Bug 295447 caused this bug.

Is there a way to call SetFocus(PR_FALSE) when switching tabs?
It is correct?

Comment 4

[Ginn Chen]

Backing out the change of Bug 295447 will cause another problem, e.g.
1) Open Firefox, browse to a site
2) Press Ctrl+I to show Page Info, "Page Info" window shows. Do not close it.
3) Click the browser window to switch focus back.
4) Press Ctrl+I again, "Page Info" window shows but the window is not focused. (I think it's bad.)
5) Click the location bar, try to input something. You can't input although the window title is highlighted. User will be confused.
6) User has to click the "Page Info" window, and click the browser window again to input in the location bar.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> Also raising a window without focus is ridiculous. It doesn't happen with other
> applications, and doesn't happen on Windows or Mac OS X.

It's a common behavior for some window manager preferences on X (focus follows mouse, either sloppy or strict) -- some users like it that way.  Why do you want Mozilla to be the one app that doesn't let them have that behavior?

But in this case the really problematic issue seems to be that you also don't like focusing a window without raising it.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> I don't want to back it out because it solved
> https://bugzilla.mozilla.org/show_bug.cgi?id=260560#c37, which is a security
> issue. (It seems few people care about it.)

That sounds like something that should be solved using modality, not one-off raises.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #4)
> 4) Press Ctrl+I again, "Page Info" window shows but the window is not focused.
> (I think it's bad.)

For some sets of Window manager preferences, it's bad for the app to change focus so that it differs from the mouse; fixing what you think is bad requires causing what I think is bad.

It seems what should happen in a case like that is that the window be raised, and if the window manager focuses raised windows, it should focus it as a side-effect of the raise.

But you shouldn't cause extraneous raises when switching tabs.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)

... then again, the fix you want for that case probably won't break my case either, since my window manager knows better.  But the key point there is that the fix you want for that case should not apply to all focus calls, but only some of them.  In other words, you shouldn't overload focus to do other things that are only wanted some of the time.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given the documentation at:
http://developer.gimp.org/api/2.0/gtk/GtkWindow.html#gtk-window-present
http://developer.gimp.org/api/2.0/gtk/GtkWidget.html#gtk-widget-get-toplevel
I think maybe the solution to get what you want is the following change:

-        if (gRaiseWindows && aRaise && toplevelWidget &&
+        if (gRaiseWindows && aRaise && toplevelWidget == owningWidget &&

since it seems that a significant part of the problem here is that gtk_window_present does things for non-toplevel widgets (e.g., tab switching).  I suppose SetFocus already has an aRaise parameter, but I think that's probably set for all DOM window.focus calls, so it really shouldn't be so important for calling focus() on something that's inside a window.

Or do or toplevel nsWindows have something that GTK doesn't consider toplevel, in which case this would make the whole thing a no-op?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That does seem to fix this problem; I'm not sure if it regresses the things bug 295447 was supposed to be fixing.

Comment 11

[Ginn Chen]

Testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=330006#c4
can be reproduced after -        if (gRaiseWindows && aRaise && toplevelWidget &&
+        if (gRaiseWindows && aRaise && toplevelWidget == owningWidget &&

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, then it sounds like a better way of determining if it's a toplevel window is needed.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any progress on this?

Comment 14

[Ginn Chen]

(In reply to comment #13)
> Any progress on this?
> 

I've no updates for this.

> * we shouldn't do a raise/present when switching tabs.
The focus method is in nsWindow, I think we can't do a check in it.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This needs to be either addressed, or the patch for bug 295447 needs to be backed out.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, the Focus call in which this gtk_window_present is happening is the focus call made by setFocus in updateCurrentBrowser in tabbrowser.xml.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

So this is the only way I could distinguish chrome from content in the widget code.

Here's what I'm trying to fix.  nsGlobalWindow::Focus calls nsWindow::SetFocus(PR_TRUE), for both chrome and content windows.  Ginn Chen wants us to have the gtk_window_present call for the cases where the global window in question is a chrome window.  This bug is caused by also doing that gtk_window_present call when the window in question is a content window.

Our GTK widget and GDK/X window hierarchy is rather mystical to me.  In both cases, owningWindow is this->mParent.  In both cases, owningWindow's drawingarea's clip_window has a gdk_window_get_window_type() of CHILD_WINDOW, as does its parent, and its grandparent has a type of TOPLEVEL_WINDOW.  However, the GDK window / X window hierarchy is different, and that's how I distinguish the cases, since in the chrome case, this's drawingarea's clip_window is a child of owningWindow's drawingarea's inner_window, but in the content case there are other windows in between.  (In all cases, a given drawingarea's inner_window is the child of its clip_window.)

So this checks the first thing I could find that was different.

Comment 18

[Adam Guthrie]

I think bug 348369 may fix the underlying issue here.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably, but either way I don't think we should call gtk_window_present in response to a focus call on something inside a window.

Comment 20

[Ginn Chen]

Thanks, dbaron.

This patch works.

I was about to back my patch out.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244545 [details] [diff] [review]
patch

Yucky, but makes sense.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: attempt at a different approach

I think this patch didn't work only because we're getting the nsWindow::Focus call well after the key event that triggered the code that eventually did a focus has been done being processed.  Fixing that would require huge changes across Mozilla.

(I might try to get some pieces of this in in separate bugs, though.)

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative patch

This works.  It prevents tabbrowser from trying to focus a window that the user could already have moved away from.  And I think any outline panting issues should be long-since fixed.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative patch (diff -U12 -b)

Thanks to gavin for suggesting removing the setTimeout.

This makes sure to do the focus call on the inner window before we've potentially moved away from it, since, if we have, it might raise a window that the user has moved away from or even move that window to a different desktop.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Old fix backed out and new fix checked in to trunk.