312998 - fix gtkmozembed's EmbedWindow::GetVisibility

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

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)]

Attachment: fix

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 200071 [details] [diff] [review]
fix

This breaks opening of chrome URLs in new windows

Comment 3

[Jean-François Rameau]

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]

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)]

Attachment: updated fix

This fixes the bug and doesn't break chrome.

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

*** Bug 335349 has been marked as a duplicate of this bug. ***

Comment 7

[Christopher Blizzard (:blizzard)]

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]]

what's his mail ?

Comment 9

[Matthias Clasen]

Robert, can you review this patch ?

It fixes very visible and embarrassing bugs in gtkembedmoz-using applications.

Comment 10

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

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)]

(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) (email my personal email if necessary)]

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]

Attachment: updated patch

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

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)]

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]]

Comment on attachment 219861 [details] [diff] [review]
updated fix

a=beltzner on behalf of drivers

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 219861 [details] [diff] [review]
updated fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Daniel Veditz [:dveditz]]

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 [:anarchy]]

(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]

That's not how we do business. please visit irc and ask someone to explain it.

Comment 21

[Alexander Sack]

Looks like this has been forgotten on 1.8 branch.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

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]

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: [e-mail]]]

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]]

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]]

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]]

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)]

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]]

verified with linux 2.0.0.4 rc2