61782 - focus problems with gtk based plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

There are some problems with plugins that use a gtkmozbox as a shim.  They are
described to me by Neil Hodgson:

"The other troublesome behaviour is what looks to be a focus problem. When
showing the demonstration page and a menu is pulled down, then clicking in
the text input element causes the menu to disappear as expected. However
clicking in the GtkEntry does not cause the menu to disappear. So either a
mouse event should be forwarded to the toplevel window or focus should be
monitored so the menu can be dismissed when focus is gained by another
window."

and

"The major symptom is that when the HTML text entry area has focus and is
accepting character input, accelerators such as Ctrl+O cause commands to be
executed. When the demonstration plugin is accepting character input,
accelerators are ignored. Other widgets including scintilla have similar
behaviour."

I have a patch for the first problem which I will upload in a bit.  I still have
to make a test case for the second.  However, I'm wondering in the accelerator
that he's talking about is the gtk accelerator for a menu or the text widget?
What accelerator is being executed?

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 2

[Neil Hodgson]

The accelerators refer to those attached to Mozilla's menus. Ctrl+O normally 
shows an Open File dialog. When focus is on a plugin, this does not occur.

Comment 3

[Christopher Blizzard (:blizzard)]

So, I went to look at the accelerators and found some problems.  Then I found
some other focus problems.  Then I fixed those and found some other key related
problems.  So, I ended up hacking a lot of the event handling and focus handling
code.  It's a _lot_ more solid than it was before.  I had one of those "I'm
amazed it ever worked" moments.

With this patch I'm about to upload focus works properly with embedded gtk
widgets inside of mozilla.  Plugins also work when mozilla is embedded as a gtk
widget inside of another gtk application with a gtk widget as a plugin.  It also
lets accelerators pass up to mozilla if the key event isn't handled in the
plugin widget.

There are some problems with keyboard accelerators conflicting with keys in
widgets, though.  For example, in the gtk widget Alt-F does something in the
entry so you can't open the file menu directly.  Alt-E works though.  There's
nothing I can do about that, though.

These changes remove some long standing hacks to the focus code.  We don't have
to listen for key or focus events on the toplevel shell window anymore which is
a big improvement.  The reason why we had to do that was because the mozarea
wasn't grabbing focus when the first nsIWidget::SetFocus() was called when the
window was created.  Key and focus events would be sent up to the toplevel
window and instead of being sent to the mozarea where they would be handled they
were being dropped.  Having to listen for those events also causing all kinds of
problems with multiple focus and activate/deactivate events being set.  The new
code is a lot more sane in this respect.

There were also a lot of cases where we weren't passing key events around
properly.  I fixed a lot of those cases.

Saari, I really need you to look at this code if you can and do some good
testing.  I haven't found any problems yet but there still might be some lurking
in there.  Same with you Akkana.  You two always find focus bugs that I miss.

Fixing this code sucked up my entire weekend.  David, you owe me.

Comment 4

[Christopher Blizzard (:blizzard)]

Attachment: patch 2

Comment 5

[Christopher Blizzard (:blizzard)]

Oh, on another note I had to fix problems with the sample plugin that I was
sent.  The api changed after M18 for Unixy systems.  Yes, it sucks but it really
was needed.  When you ask for a plugin port you get a native X Window.  As in
the "Window" type in X.

I'll attach what I had to change in my tip build in a sec.

Comment 6

[Christopher Blizzard (:blizzard)]

Attachment: plugin changes

Comment 7

[saari (gone)]

Firing up my linux build to take a look... sounds like a yummy patch.

Comment 8

[Christopher Blizzard (:blizzard)]

Have fun, chris. :)

Comment 9

[Christopher Blizzard (:blizzard)]

saari has tested this code and hasn't found any problems.

Comment 10

[Christopher Blizzard (:blizzard)]

Waiting on pav to review this.

Comment 11

[Brendan Eich [:brendan]]

+
  if (!gdk_window)
+
    fprintf(stderr, "NO WINDOW!!!\n");

followed by a use of gdk_window seems bad -- instead, NS_ASSERTION and an early
return?

/be

Comment 12

[Christopher Blizzard (:blizzard)]

Brendan, I think that you looked at the wrong patch.  :)  Ignore the 20133
patch, that was just for some test code that isn't anywhere near mozilla.

You want to look at the 20132 patch.

Comment 13

[Akkana Peck]

The code looks good to me as far as I can see; I'm not up on all the details of
the focus code in nsWindow.cpp, but I don't see anything that looks like it
should cause problems, and I haven't seen any problems in the limited testing
I've done so far.  (There's a probable typo in a comment in nsWindow.cpp,
"Figure out of" instead of "Figure out if".)  r=akkana.

Comment 14

[saari (gone)]

So
  PRBool sendActivate = gJustGotActivate;
followed by
   if (sendActivate) {
     DispatchActivateEvent();
   }

Reads as "if I just got an activate, send it again". Is that right or should one
of the variable names change?

Comment 15

[Brendan Eich [:brendan]]

Cc'ing self, waiting for r=....

/be

Comment 16

[Christopher Blizzard (:blizzard)]

The DispatchActivateEvent() is only called once, right after the sendEvent
check.  The reason that I make it a local variable is that it has to be sent
after all of the SetFocus() calls are made.  As part of the first focus in event
the global activate flag might be reset to false and the activate event might
never be sent.  Setting it as a local means that that information makes it to
the end of the function.

Other than that can I consider this an r=?

Comment 17

[Brian Ryner (not reading)]

I've been running with this patch in my tree for some time and I don't see any
focus regressions at all.  r=bryner.

Comment 18

[Christopher Blizzard (:blizzard)]

brendan?  It's up to you now.

Comment 19

[saari (gone)]

Okay, r=saari

Comment 20

[Scott Collins]

the xpcom rule is that interfaces are reference counted, not implentations; so
you typically must |AddRef| and |Release| through an |nsI...*|.  |nsCOMPtr|s
sort of enforce this rule by not usually working (compiling) when applied to
concrete objects.  You might want to fix |GetGrabbingWindow| to either

  (a) Not return an |AddRef|ed object, if lifetime issues allow this solution
  (b) Let it instead return an interface pointer, and hold that in an |nsCOMPtr|
  (c) If it _must_ return a concrete object, _also_ return the corresponding
      interface pointer through which you can manage ownership
  (d) Since |GetGrabbingWindow| isn't a regular xpcom `getter' don't |AddRef|,
      but immediately manage ownership in the caller like so

        nsWindow* grabbingWindow = GetGrabbingWindow();
        nsCOMPtr<nsISupports> grabbingWindowGuard(grabbingWindow);

In any case, comments would be good since this use is outside the norm.

Comment 21

[Scott Collins]

I understand the change to nsWidget.cpp in patch 2 is not to be applied.

Comment 22

[Scott Collins]

you know I hate those old-style casts :-)  You're sprinkling |(void *)| around
in |printf| argument lists.  Prefer |NS_STATIC_CAST| for this kind of conversion.

Comment 23

[Scott Collins]

+    GdkWindow *window;
+    window = (GdkWindow *)sFocusWindow->GetNativeData(NS_NATIVE_WINDOW);

initialize in the declaration, and prefer the macros for casts, e.g.,

     GdkWindow *window = NS_STATIC_CAST(GdkWindow*,
sFocusWindow->GetNativeData(NS_NATIVE_WINDOW));

or else, |NS_REINTERPRET_CAST| if the types are unrelated.

Comment 24

[Scott Collins]

consider similar options (to |GetGrabbingWindow| discussion above) for the
|isChild| block, e.g.,

  if (isChild)
    {
      nsWidget* focusWidget = sFocusWindow;
      nsCOMPtr<nsISupports> focusWidgetGuard(focusWidget);

      focusWidget->...
      ...
    }

Comment 25

[Scott Collins]

why a |while| loop instead of the normal iteration form?  (Here using |w|
instead of your |tmpWindow|)

  for ( GdkWindow* w=mozbox->parent_window; w; w=gdk_window_get_parent(w) )
  {
    gpointer data = NULL;
    gdk_window_get_user_data(w, &data);
    if ( data && GTK_IS_WINDOW(data) )
      return GTK_WINDOW(data);
  }

Comment 26

[Christopher Blizzard (:blizzard)]

Some comments about scc's review...

o I've changed the GrabbingWindow code to use the 3rd form listed:

Since |GetGrabbingWindow| isn't a regular xpcom `getter' don't |AddRef|,
      but immediately manage ownership in the caller like so

        nsWindow* grabbingWindow = GetGrabbingWindow();
        nsCOMPtr<nsISupports> grabbingWindowGuard(grabbingWindow);

o Same for the (isChild) code.  In both places it's clean and easy to understand.

o I've changed the void *'s in printfs to use NS_STATIC_CAST()

As for using a while() instead of a for() loop I much prefer the while() for
that kind of iteration.  If it were an array I would use a for() loop but it
isn't.  It's a linked list.

Also, that's how it is done in a lot of gtk code so it's stylistically familiar
to me when working with gtk objects.

Thanks!

Comment 27

[Christopher Blizzard (:blizzard)]

Attachment: final patch

Comment 28

[Brendan Eich [:brendan]]

Wow, scc is on the case.  Hey scott, didn't you ok the use of an nsCOMPtr for a
concrete type that singly inherits from nsISupports?  I've recommended that in a
few places over the last six months.

I'm looking at the patch now that domain-buddies have r='d it, FWIW.  I'll give
an sr= soon with the proviso that scc's comments get addressed, somehow (the for
over while loop is a choice I would make too, but _de gustibus..._).

/be

Comment 29

[Christopher Blizzard (:blizzard)]

I just checked this in.  I didn't see your comment.  brendan, do you want me to
back it out?

Comment 30

[Brendan Eich [:brendan]]

sr=[e-mail], a bit rubber-stampy but the r='s mean a lot to me.
Thanks for getting them.

/be

Comment 31

[shrirang khanzode]

verified (rubberstamp)