214583 - [gtk2] windows not activating correctly

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Brian Ryner (not reading)]

This seems to be gtk2-only.  If I load a page in a browser window, then open a
mail compose window, then switch back to the browser window, I cannot use
pageup/pagedown to scroll the page - focus memory is not getting restored.
Hitting tab several times does allow you to recover focus in the window.

Comment 1

[Christopher Blizzard (:blizzard)]

Yay for focus problems.

Comment 2

[Mitch]

I *think* this may be a metacity bug. I've upgraded to cvs of metacity and can't
say i see this problem. Reporter do you use metacity as your window damager ?

Comment 3

[Brian Ryner (not reading)]

Ok, I found out what's going on.  When the mail compose window is deactivated,
we update some commands which cause data to be fetched from the clipboard.  This
can cause another iteration of the gtk mainloop, which means that we can get the
focus-in event for the browser window while we're still processing the
deactivate for the compose window.  This causes gFocusWindow to incorrectly end
up set to null.

Still thinking about what the right fix is here -- I can get gFocusWindow to be
correct easily enough, but this is still likely to leave things in a bad state
as we assume the events can be handled atomically (I'm thinking of ESM and focus
controller).  What I think I'd like to do is skip over events (without changing
their order in the event queue) if we're in the middle of a call to
gtk_clipboard_wait_for_*.

Comment 4

[Brian Ryner (not reading)]

... but I don't see a way to make gtk, or X, do that.  Which makes sense, as
being a queue means you can only take the event off the front of it.

Here are a few other ideas:

1. Make Mozilla's clipboard API be asynchronous so that it maps better to GTK's
functionality.

2. Could we could queue up the events ourselves during execution of the nested
mainloop, and re-insert them into the event queue after the clipboard function
returns?  That's getting kind of messy.

3. Make Mozilla's event system able to deal with reentrancy between deactivate
and activate events.

Comment 5

[Christopher Blizzard (:blizzard)]

I'd like to make the clipboard API async, if we can.  It would make things soooo
much easier.

( I'd also like to do the same for the DND api while we're at it, since drop
events are async as well. )

Comment 6

[Brian Ryner (not reading)]

Yet another idea is to reinstate some version of the gtk1
nsClipboard::FindSelectionNotifyEvent, which waits for the data without running
the gtk event loop.  This might require no longer using GtkClipboard, though.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch

Ignore my earlier comment about not being able to manipulate the event queue. 
This patch ports some code from the gtk1 source to wait for the SelectionNotify
event and send it to the clipboard, without letting any other events be
processed.

Comment 8

[Stuart Parmenter]

Making all the callers of the clipboard code, including embedders use async
clipboard APIs I suspect would be far more difficult than making the gtk2 code
sync.  That said, I wish we had made the clipboard APIs async 4 years ago :/

Comment 9

[Brian Ryner (not reading)]

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

let's not do this.  It introduces bug 56219 to the gtk port.  I'll come up with
something better.

Comment 10

[Brian Ryner (not reading)]

You know what, let's just make the clipboard API async.  I'll hook up some
dependent bugs.

Comment 11

[Brian Ryner (not reading)]

Attachment: new patch

I think there's actually a way to do this using my original approach (with a
little tweaking).  I discovered XCheckIfEvent() and can now pull events out of
the queue in the correct order.  I tested it with pasting a very large amount
of text, so I'm sure I didn't introduce bug 56219.

This also fixes an unnecessary temporary string assignment in GetData(), which
should save a string copy.

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 130804 [details] [diff] [review]
new patch

Couple of things: use select() on the X connection (much to the chagrin of VMS)
not sleep.

Also, please declare all functions at the top of the file instead of just
defining them inline.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 130804 [details] [diff] [review]
new patch

> use select() on the X connection (much to the chagrin of VMS) not sleep.

Are you saying it should be ifdef'd for VMS to still use sleep?

Comment 14

[Brian Ryner (not reading)]

Attachment: new patch

Fixed blizzard's comments (I took the VMS fu from gtk2xtbin.c, hope it's
right).  This also fixes it to not wait for the timeout if the request
completed synchronously (i.e. copying from the same Mozilla instance).

Comment 15

[Brian Ryner (not reading)]

Attachment: patch that works on gtk+ 2.0.x

This patch avoids using API's that are not present in GTK+ 2.0.x.

Comment 16

[Christopher Blizzard (:blizzard)]

What, you don't just want to make the APIs async? :)

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

Not right now, no :)

Comment 18

[Christopher Blizzard (:blizzard)]

Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

r=blizzard

My only comment is that WaitForRetrieval is StudlyCaps while other static
functions use_underscores, which I prefer.

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

I'm just going to use blizzard's review since this isn't part of the default
build.

Comment 20

[Brian Ryner (not reading)]

checked in.

Comment 21

[Darin Fisher]

i think this may have caused bug 226405