fix gtkmozembed's EmbedWindow::GetVisibility

RESOLVED FIXED

Status

Core Graveyard
Embedding: GTK Widget
--
major
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Assigned: Christian Persch (GNOME) (away; not receiving bug mail))

Tracking

({fixed1.8.0.8, verified1.8.1.4})

Trunk
x86
Linux
fixed1.8.0.8, verified1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

1.13 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
636 bytes, patch
Details | Diff | Splinter Review
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).
Created attachment 200071 [details] [diff] [review]
fix
Assignee: mpgritti → chpe
Status: NEW → ASSIGNED
Attachment #200071 - Flags: superreview?(roc)
Attachment #200071 - Flags: review?(mpgritti)
Attachment #200071 - Flags: superreview?(roc) → superreview+
Comment on attachment 200071 [details] [diff] [review]
fix

This breaks opening of chrome URLs in new windows
Attachment #200071 - Flags: superreview+ → superreview-
Attachment #200071 - Flags: superreview-
Attachment #200071 - Flags: review?(mpgritti)
Attachment #200071 - Flags: review-
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

12 years ago
This breaks devhelp as mentioned over here: https://launchpad.net/distros/ubuntu/+source/devhelp/+bug/40320/
Created attachment 219861 [details] [diff] [review]
updated fix

This fixes the bug and doesn't break chrome.
Attachment #200071 - Attachment is obsolete: true
*** Bug 335349 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Attachment #219861 - Flags: review?(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.
Attachment #219861 - Flags: review?(blizzard)
what's his mail ?
Attachment #219861 - Flags: superreview?(roc)
Attachment #219861 - Flags: review+

Comment 9

11 years ago
Robert, can you review this patch ?

It fixes very visible and embarrassing bugs in gtkembedmoz-using applications.
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...
(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 on attachment 219861 [details] [diff] [review]
updated fix

OK, please include a comment to that effect, and add the parens. Thanks!
Attachment #219861 - Flags: superreview?(roc) → superreview+

Comment 13

11 years ago
Created attachment 239384 [details] [diff] [review]
updated patch
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Attachment #219861 - Flags: approval1.8.1?
Attachment #219861 - Flags: approval1.8.0.8?
Comment on attachment 219861 [details] [diff] [review]
updated fix

a=beltzner on behalf of drivers
Attachment #219861 - Flags: approval1.8.1? → approval1.8.1+

Updated

11 years ago
Depends on: 354549
No longer depends on: 354549
Comment on attachment 219861 [details] [diff] [review]
updated fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #219861 - Flags: approval1.8.0.8? → approval1.8.0.8+
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.
Keywords: fixed1.8.0.8

Comment 19

11 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 20

11 years ago
That's not how we do business. please visit irc and ask someone to explain it.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 21

11 years ago
Looks like this has been forgotten on 1.8 branch.
Flags: blocking1.8.1.4?
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

11 years ago
Comment on attachment 219861 [details] [diff] [review]
updated fix

Right, but it already has approval for 1.8.1. Re-requesting approval.
Attachment #219861 - Flags: approval1.8.1.4?
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 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)?
Attachment #219861 - Flags: approval1.8.1+
Comment on attachment 219861 [details] [diff] [review]
updated fix

Unfortunately this request is way too late to squeeze into 1.8.1.4.
Attachment #219861 - Flags: approval1.8.1.4? → approval1.8.1.5?
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
Attachment #219861 - Flags: approval1.8.1.5? → approval1.8.1.4+
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
Keywords: fixed1.8.1.4
verified with linux 2.0.0.4 rc2
Keywords: fixed1.8.1.4 → verified1.8.1.4

Updated

10 years ago
Flags: blocking1.8.1.4?

Updated

7 years ago
Component: Embedding: GTK Widget → Embedding: GTK Widget
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.