Last Comment Bug 312998 - fix gtkmozembed's EmbedWindow::GetVisibility
: fix gtkmozembed's EmbedWindow::GetVisibility
Status: RESOLVED FIXED
: fixed1.8.0.8, verified1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: Embedding: GTK Widget (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: ---
Assigned To: Christian Persch (GNOME) (away; not receiving bug mail)
: Stuart Parmenter
Mentors:
http://www.gnome.org/~chpe/testcases/
: 335349 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-19 05:25 PDT by Christian Persch (GNOME) (away; not receiving bug mail)
Modified: 2012-04-05 00:46 PDT (History)
13 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (1.10 KB, patch)
2005-10-19 05:25 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
chpe: review-
Details | Diff | Splinter Review
updated fix (1.13 KB, patch)
2006-04-26 04:07 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
caillon: review+
roc: superreview+
dveditz: approval1.8.0.8+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review
updated patch (636 bytes, patch)
2006-09-20 11:36 PDT, Matthias Clasen
no flags Details | Diff | Splinter Review

Description Christian Persch (GNOME) (away; not receiving bug mail) 2005-10-19 05:25:04 PDT
gtkmozembed's EmbedWindow::GetVisibility function doesn't work right: when the
internal state is 'visible' but the widget is not mapped (for example because
it's in a GtkNotebook and not the current tab), it still returns true. That
means that the focus checking code incorrectly allows setting the focus to a
textarea widget in this tab, messing up the focus in the active tab.

Steps to reproduce:
0) Start Epiphany
1) Load http://www.gnome.org/~chpe/testcases/test-othertab.html in tab 0
2) Load http://www.gnome.org/~chpe/testcases/test-textarea.html in tab 1
3) Move mouse over the textarea in tab 1
4) Switch back to tab 0
5) Try to type something in the textarea or input field

Result:
The background tab continously makes the foreground tab lose input focus.

The fix is simple: check if the widget is mapped!
+  *aVisibility = mVisibility &&
+                 mOwner->mOwningWidget &&
+                 GTK_WIDGET_MAPPED(mOwner->mOwningWidget);

I didn't know whether ::GetVisibility might be called after the widget is
destroyed, so I added the mOwner->mOwningWidget safetty check.

This is the same thing for gtkmozembed as bug 306245 was for camino.
This patch does NOT make my patch from bug 303730 obsolete, since there can be
more than one mapped gtkmozembed in the toplevel window (e.g. the main tab, and
the sidebar embed).
Comment 1 Christian Persch (GNOME) (away; not receiving bug mail) 2005-10-19 05:25:37 PDT
Created attachment 200071 [details] [diff] [review]
fix
Comment 2 Christian Persch (GNOME) (away; not receiving bug mail) 2005-11-16 13:38:57 PST
Comment on attachment 200071 [details] [diff] [review]
fix

This breaks opening of chrome URLs in new windows
Comment 3 Jean-François Rameau 2005-11-16 13:44:31 PST
Other problem with EmbedWindow::GetVisibility.

0) start gtkmozembed with about:blank
1) resize the window

The embed part is borked due to some visibility problem. 

2) Click on embed part: the blank page is rightly rendered.
Comment 4 Daniel Holbach 2006-04-20 07:59:10 PDT
This breaks devhelp as mentioned over here: https://launchpad.net/distros/ubuntu/+source/devhelp/+bug/40320/
Comment 5 Christian Persch (GNOME) (away; not receiving bug mail) 2006-04-26 04:07:09 PDT
Created attachment 219861 [details] [diff] [review]
updated fix

This fixes the bug and doesn't break chrome.
Comment 6 Christian Persch (GNOME) (away; not receiving bug mail) 2006-05-11 03:34:58 PDT
*** Bug 335349 has been marked as a duplicate of this bug. ***
Comment 7 Christopher Blizzard (:blizzard) 2006-08-28 12:19:25 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

I'm not doing reviews for the time being, you're better off asking someone else like roc for a review.
Comment 8 Mike Hommey [:glandium] 2006-08-28 12:34:30 PDT
what's his mail ?
Comment 9 Matthias Clasen 2006-09-15 12:55:40 PDT
Robert, can you review this patch ?

It fixes very visible and embarrassing bugs in gtkembedmoz-using applications.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-18 15:59:02 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

I don't understand. && binds tighter than || so when mVisibility is true, this will return true regardless of the mapping state, so I don't know how this fixes the bug. At least parenthesize so it's clear what's going on...
Comment 11 Christian Persch (GNOME) (away; not receiving bug mail) 2006-09-18 16:19:12 PDT
(In reply to comment #10)
> (From update of attachment 219861 [details] [diff] [review] [edit])
> I don't understand. && binds tighter than || so when mVisibility is true, this
> will return true regardless of the mapping state, so I don't know how this
> fixes the bug. At least parenthesize so it's clear what's going on...

I know. mVisibility is never set back to PR_FALSE after it's become PR_TRUE once. The problem is just that sometimes the window is already visible even though mVisibility isn't true yet. This patch is just a work-around: it returns true whenever it did previously but fixes the corner-case. 
A real fix would investigate why chrome breaks when we just always return the widget's mapped state here.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-18 19:18:11 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

OK, please include a comment to that effect, and add the parens. Thanks!
Comment 13 Matthias Clasen 2006-09-20 11:36:45 PDT
Created attachment 239384 [details] [diff] [review]
updated patch
Comment 14 Christopher Aillon (sabbatical, not receiving bugmail) 2006-09-26 17:46:06 PDT
cvs commit: Examining .
Checking in EmbedWindow.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedWindow.cpp,v  <--  EmbedWindow.cpp
new revision: 1.32; previous revision: 1.31
done
Comment 15 Christopher Aillon (sabbatical, not receiving bugmail) 2006-09-26 17:50:08 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

Low risk, embedding only patch that fixes a slew of dependent applications.  See e.g. comment 4 and all the things mentioned in that bug report.
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2006-09-27 10:36:16 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

a=beltzner on behalf of drivers
Comment 17 Daniel Veditz [:dveditz] 2006-09-29 11:44:53 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 18 Daniel Veditz [:dveditz] 2006-10-06 13:27:27 PDT
fix checked into MOZILLA_1_8_0_BRANCH. Looks like it's not on the 1.8 branch yet but I think that tree is pretty locked down at the moment.
Comment 19 Jory A. Pratt 2007-01-02 20:41:55 PST
(In reply to comment #18)
> fix checked into MOZILLA_1_8_0_BRANCH. Looks like it's not on the 1.8 branch
> yet but I think that tree is pretty locked down at the moment.
> 

When shall we see this on 1.8 branch. It still has not been commited.
Comment 20 timeless 2007-01-03 07:49:47 PST
That's not how we do business. please visit irc and ask someone to explain it.
Comment 21 Alexander Sack 2007-05-02 16:59:28 PDT
Looks like this has been forgotten on 1.8 branch.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-05-02 17:29:32 PDT
You should just ask for approval on the patch, it's unlikely this is going to actually block any 1.8.1.x release.
Comment 23 Alexander Sack 2007-05-03 02:35:44 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

Right, but it already has approval for 1.8.1. Re-requesting approval.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-05-03 08:14:23 PDT
Approvals tend to expire once something has missed the release, so re-requesting approval for the next desired milestone is the right thing to do.
Comment 25 Daniel Veditz [:dveditz] 2007-05-04 19:55:02 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

This missed 1.8.1, clearing old approval to avoid confusion.

We had release candidate builds but might have to respin. Could you find someone to land this today or tomorrow (5/5)?
Comment 26 Daniel Veditz [:dveditz] 2007-05-07 10:25:02 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

Unfortunately this request is way too late to squeeze into 1.8.1.4.
Comment 27 Daniel Veditz [:dveditz] 2007-05-07 10:29:16 PDT
Comment on attachment 219861 [details] [diff] [review]
updated fix

Chris Aillon says he'll land this, approved for 1.8.1.4, a=dveditz for release-drivers
Comment 28 Christopher Aillon (sabbatical, not receiving bugmail) 2007-05-07 10:43:09 PDT
Actually, this landed already in this cycle but I forgot to mark the bug.

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/embedding/browser/gtk/src/EmbedWindow.cpp&rev=MOZILLA_1_8_BRANCH&mark=1.31.12.1

fixed1.8.1.4
Comment 29 Tracy Walker [:tracy] 2007-05-15 11:13:21 PDT
verified with linux 2.0.0.4 rc2

Note You need to log in before you can comment on or make changes to this bug.