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 User image 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 User image Christian Persch (GNOME) (away; not receiving bug mail) 2005-10-19 05:25:37 PDT
Created attachment 200071 [details] [diff] [review]
fix
Comment 2 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Mike Hommey [:glandium] 2006-08-28 12:34:30 PDT
what's his mail ?
Comment 9 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image Robert O'Callahan (:roc) (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 User image Matthias Clasen 2006-09-20 11:36:45 PDT
Created attachment 239384 [details] [diff] [review]
updated patch
Comment 14 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Alexander Sack 2007-05-02 16:59:28 PDT
Looks like this has been forgotten on 1.8 branch.
Comment 22 User image :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 User image 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 User image :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 User image 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 User image 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 User image 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 User image 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 User image 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.