545429 - location bar blocks file-open dialog

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Dietrich Ayala (:dietrich)]

STR:

1. type into the location bar such that the autocomplete popup is shown
2. type ctrl+o

Expected: autocomplete popup closes, file-open dialog is fully visible and focused

Actual: autocomplete popup is open and *on top* of the file-open dialog, but the file-open dialog has focus!

Comment 1

[Dietrich Ayala (:dietrich)]

copying Neil. i don't know if this is a location bar fix or a widget/focus fix.

Comment 2

[Dietrich Ayala (:dietrich)]

confirmed not an issue in 3.6 on Mac.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

WFM on Win7
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100209 Minefield/3.7a2pre

Comment 4

[Neil Deakin]

There's another bug on this I can't find. Linux should be rolling up popups when a window is lowered, minimized or closed, as other platforms do.

Comment 5

[Michael Monreal [:micmon]]

You probably mean the annoying bug #522956

Comment 6

[Neil Deakin]

(In reply to comment #5)
> You probably mean the annoying bug #522956

I don't mean that bug no, but it is caused by the same issue.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

This is due to GrabKeyboard in nsWindow::CaptureRollupEvents.
The autocomplete popup window keeps to grab keyboard focus so that the file dialog window can not grab keyboard focus then the autocomplete popup window does not receive blur event.

I think the GrabKeyboard is an overkill for the fix of bug 179078. Without the GrabKeyboard there are no need of workarounds for bug 522956 and bug 618248.  But there is another issue that an assertion happens in nsWindow::OnContainerFocusInEvent while running browser/base/content/test/browser_bug553455.js test.

I do not exactly understand the issue of bug 526941 (The assertion has been inserted for the bug fix), but I am guessing the fix for bug 526941 is not need any more too.

I would like to hear karlt's view.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #7)
> This is due to GrabKeyboard in nsWindow::CaptureRollupEvents.
> The autocomplete popup window keeps to grab keyboard focus so that the file
> dialog window can not grab keyboard focus then the autocomplete popup window
> does not receive blur event.

Hmm.  I see now the fix for bug 522956 works because a configure event is received on the new window.  I had assumed it was the focus-out that triggered the check for rollup, but, as you say, the keyboard grab means that GDK doesn't tell us about the focus change.  The file open dialog is not our window (it is GTK's) so we don't notice its configure events.

> I think the GrabKeyboard is an overkill for the fix of bug 179078.

GrabKeyboard was added to the first implementation of CaptureRollupEvents
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.18#664
even before bug 179078 was involved.

I don't know whether or not the GrabKeyboard is necessary.  Now that we check for rollup on focus out, we would have already rolled up by the time another window is getting key events, if we were expecting key events.  The key thing to check would be that the Esc key rolls up menus and drop-drowns appropriately.

> Without the
> GrabKeyboard there are no need of workarounds for bug 522956 and bug 618248. 

I expect we still need something like bug 522956 to effect the roll-up.

> But there is another issue that an assertion happens in
> nsWindow::OnContainerFocusInEvent while running
> browser/base/content/test/browser_bug553455.js test.
> 
> I do not exactly understand the issue of bug 526941 (The assertion has been
> inserted for the bug fix), but I am guessing the fix for bug 526941 is not need
> any more too.

I don't know why that assertion is firing, but AFAIK, that's not related.  noautohide panels are quite different.

Comment 9

[Karl Tomlinson (:karlt)]

The browser_bug553455.js assertion may be bug 625149.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #8)
> (In reply to comment #7)
> Hmm.  I see now the fix for bug 522956 works because a configure event is
> received on the new window.  I had assumed it was the focus-out that triggered
> the check for rollup, but, as you say, the keyboard grab means that GDK doesn't
> tell us about the focus change.  The file open dialog is not our window (it is
> GTK's) so we don't notice its configure events.

Yes, exactly.

> > I think the GrabKeyboard is an overkill for the fix of bug 179078.
> 
> GrabKeyboard was added to the first implementation of CaptureRollupEvents
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.18#664
> even before bug 179078 was involved.

Hmm, I can not understand why the the GrabKeyboard was needed from the commit log.

> > Without the
> > GrabKeyboard there are no need of workarounds for bug 522956 and bug 618248. 
> 
> I expect we still need something like bug 522956 to effect the roll-up.

In which case? Is there any test cases for that?

(In reply to comment #9)
> The browser_bug553455.js assertion may be bug 625149.

As reading the comments in bug 625149, I think that bug is caused by GrabKeyboard too.

By the way, for browser_bug553455.js, I attached the patch for bug 623242 to wipe the assertion out.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to comment #10)
> > I expect we still need something like bug 522956 to effect the roll-up.
> 
> In which case? Is there any test cases for that?

The steps to reproduce for bug 522956 and this bug are suitable test cases.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Just remove GrabKeyboard in CaptureRollupEvents().

I'd suggest to remove GrabKeyboard in CaptureRollupEvents.
I had a run on try server with this patch, and there was no suspicious orange. (Actually there were two oranges, bug 452706 and bug 510915 but I think those are not related this change.)

So there are two possibilities:
a) This patch is no harmful effect.
b) There is no test case covering this change.

I'd bet a).

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 507813 [details] [diff] [review]
Just remove GrabKeyboard in CaptureRollupEvents().

This makes sense to me, but can you also removed the now unused mRetryKeyboardGrab and GrabKeyboard() as well as the corresponding gdk_keyboard_ungrab(), please?

Comment 14

[Neil Deakin]

(In reply to comment #12)
> So there are two possibilities:
> a) This patch is no harmful effect.
> b) There is no test case covering this change.
> 
> I'd bet a).

Note though that the correct answer is b.

Comment 15

[Karl Tomlinson (:karlt)]

I think what Neil meant is that, because (b) is true, it is not possible to make any conclusions about (a) based on the tests.

However, the cocoa and "windows" ports do not grab the keyboard, so it seems reasonable that the gtk2 port doesn't need to either.

Comment 16

[Neil Deakin]

I couldn't find any issues after testing either. I suspect that removing child widgets and hiding popup on focus changes have removed any issues that might have come up.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 507813 [details] [diff] [review]
Just remove GrabKeyboard in CaptureRollupEvents().

I won't claim this is risk-free but it prevents us getting in awkward situations such as this bug and bug 540474, and would generally improve Alt-TAB behavior (bug 626616 and probably other situations).

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: remove the unused GrabKeyboard() and related

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: remove the unused GrabKeyboard() and related

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 507813 [details] [diff] [review]
Just remove GrabKeyboard in CaptureRollupEvents().

There are two patches here - not sure which we're meant to approve. Please give us a single approval request with a clear risk/reward, thanks!

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: concatenated patches for single approval request

There are two patches here.  One removes the call that causes the problem.  The other removes the unused code that is no longer called.

Reward is described in comment 17.  Bug 540474 is probably the most common and glaring symptom.
Risk is hard to estimate.  We can't think of any reason for this code to be there but perhaps we've forgotten something.  If we have forgotten something it would involve use of a key combination to perform an action on a popup when we don't have focus (but we shouldn't be grabbing the keyboard if we don't have focus anyway).  Mouse actions are unaffected by these changes.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 512605 [details] [diff] [review]
concatenated patches for single approval request

linux only, has all the right people looking at it, conforms to the 2 other top tier platforms, a=me.

Comment 23

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/d41d5082e0e3
http://hg.mozilla.org/mozilla-central/rev/1f49cd65c244

Comment 24

[Teodosia Pop]

Verified using Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12.