Closed Bug 1056251 Opened 10 years ago Closed 10 years ago

Changing to a Firefox window in a different workspace does not focus automatically

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
firefox32 --- wontfix
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed

People

(Reporter: gcharles, Assigned: smichaud)

References

Details

(4 keywords, Whiteboard: [STR in comment #8])

Attachments

(1 file, 1 obsolete file)

Fix
1.63 KB, patch
masayuki
: review+
lmandel
: approval-mozilla-aurora+
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Reproduce:
1. Open up a Firefox window (window 1)
2. Navigate to a new workspace
3. Open a new Firefox window (window 2)
 - Right click on the Firefox dock icon
 - Click 'New Window'
4. Click on the Firefox dock icon to jump back to window 1

Behaviour:
All Firefox toolbar actions and any keyboard shortcuts apply to window 2.

Expected:
All Firefox toolbar actions and any keyboard shortcuts apply to window 1.
This is bizarre. I don't use spaces, but I'm a little surprised we're this buggy on them. :-\

Markus, I know you have like 1000 things to do, but can you have a look? :-)
Component: General → Widget: Cocoa
Flags: needinfo?(mstange)
Product: Firefox → Core
(where "papercut" is a little bit of an understatement...)
Keywords: papercut, reproducible, ux-mode-error, ux-userfeedback
As best I can tell, this isn't a bug at all -- it's a feature :-)

Exactly the same behavior happens in Safari.

You should add to step 4 that if you keep clicking on the Firefox (or Safari) Dock icon, you keep switching back and forth between window 1 and window 2.
I tested on OS X 10.7.5, with Firefox 31.
(In reply to Steven Michaud from comment #3)
> As best I can tell, this isn't a bug at all -- it's a feature :-)
> 
> Exactly the same behavior happens in Safari.
> 
> You should add to step 4 that if you keep clicking on the Firefox (or
> Safari) Dock icon, you keep switching back and forth between window 1 and
> window 2.

It's a feature that your keyboard input is directed to a different window from what you're looking at, and that if you use "cmd-l" and start typing, the autocomplete box shows in front of the visible window, even though it applies to the hidden one?

Note that when I test with Safari in 10.9, as soon as I change any input (so type in the location bar, or paste), the other space slides back into view so I can see what I'm doing. This doesn't happen for cmd-c or cmd-t (open new tab). I'm not sure how Safari decides to switch space again...
(In reply to :Gijs Kruitbosch from comment #5)
> I'm not sure how Safari decides to switch space again...

(ie, what actions trigger switching back to the window listening to your input, and what actions don't)
The STR is rather poorly written, and I didn't actually notice the part about keyboard shortcuts applying to the wrong window.

But when I tested again I didn't see this.

At some point I'll test again on OS X 10.9.  But I strongly suspect that even on 10.9, this bug happens only under unusual circumstances.

Gijs:  Please test again, as thoroughly as possible.  Then write up very detailed and precise STR.
0. Open a second 'space' from mission control using the big plus button on the top-right edge of the screen
1. Return to your initial space
2. Open Firefox (with a single window, let's call it window 1)
3. Navigate to a new workspace (ctrl-right-arrow by default)
4. Right click on the Firefox dock icon
5. Click 'New Window', and wait for this new window (let's say window 2) to come up.
7. Click on the Firefox dock icon to jump back to window 1
8. Hit cmd-L and type "mozilla.org" and hit enter

Actual results:

The autocomplete popup shows on top of window 1 (if you moved window 2 in the other space, I expect it'll show in the position for that window, so it won't align properly with window 1!), but you cannot see your typing and the location bar in window 1 isn't focused. However, you remain on the first space with window 1, looking at window 1.

Expected results:

Either all Firefox toolbar actions and any keyboard shortcuts apply to window 1, or as soon as you hit a shortcut, switch back to window 2 on the other space, so you can see your typing.


---
Similar odd things happen if you use other keyboard shortcuts - they all run in window 2 instead of window 1.
Egh, I meant to add to the actual results: and mozilla.org loads in window 2, instead of window 1, but you can't see that and don't switch space.

Expected results would be that either it loads in window 1, or we switch space back to the one that has window 2.
Thanks, Gijs, for your detailed STR.

I've finally gotten around to testing it, and am able to reproduce it perfectly on OS X 10.9.

Interestingly, though, something similar (though less obviously incorrect) happens in both Chrome and Opera.  In step 8, the focus doesn't switch to window 2 until you start typing.  Cmd+l doesn't switch the focus on its own.

Not surprisingly, the only browser that handles this entirely correctly is Safari.  In step 8 it switches focus to window 2 when you press Cmd+l.

Probably the solution is (somehow) to switch desktops whenever a window becomes active (which, as far as I can tell, is the same thing as the window acquiring the keyboard focus).
Attached patch Fix (obsolete) — — Splinter Review 
I've figured out this bug and have a patch for it.  It's ultimately an Apple bug -- when a key window in a different space receives keyboard input, the OS should automatically switch to that space.

But Safari has the most effective workaround.  So I reverse engineered that and will now do something similar in Firefox.

Safari's workaround is to call -[NSWindow makeKeyAndOrderFront:] on the appropriate view's window.  This happens (among other places) in the event handler for Cmd+l (-[BrowserDocument openLocation:]).  That's actually overkill, since the "right" window is always already key.  So instead my patch uses -[NSWindow orderWindow:relativeTo;] (which is called from makeKeyAndOrderFront:).

My patch is in -[ChildView keyDown:].  This is always called exactly once when a window receives keyboard input -- even a Cmd+key combination (which also gets sent to -[NSMenu performKeyEquivalent:], sometimes more than once).
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8487520 - Flags: review?(mstange)
Flags: needinfo?(mstange)
Comment on attachment 8487520 [details] [diff] [review]
Fix

Sigh.  I just realized this doesn't work for IME keyboard input (e.g. Japanese Hiragana).

I'll keep trying.
Attachment #8487520 - Flags: review?(mstange)
Comment on attachment 8487520 [details] [diff] [review]
Fix

Oops, it *does* work.  I was testing the wrong build :-(

But after my IME scare, I realize that Masayuki is probably the most appropriate reviewer here.
Attachment #8487520 - Flags: review?(masayuki)
Side note:

At first I considered using a "spaces API" to deal with this bug.  But it turns out Apple doesn't have a documented spaces API.  Of course there's an undocumented spaces API (see https://gist.github.com/puffnfresh/4053980).  But even this is only used extensively by the Dock.app (in /System/Library/CoreServices/).  Even Safari doesn't use it, and instead works around Apple's bug as described in comment #11.
Comment on attachment 8487520 [details] [diff] [review]
Fix

Looks like this patch triggers the following reftest failures:

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/reftests/mathml/munder-mover-align-accent-true.html | image comparison (==), max difference: 255, number of differing pixels: 32053
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/reftests/mathml/menclose-2-top.html | image comparison (==), max difference: 255, number of differing pixels: 840

Tomorrow I'll try to figure out how.
These test failures make *no* sense.

I can't reproduce them locally.  And the code I added isn't even run anytime during any reftest!  So whatever the cause, they're completely unrelated to my patch.

I've started another set of tryserver builds:
https://tbpl.mozilla.org/?tree=Try&rev=188045df26dd

Hopefully the failures will just disappear down whatever hole they came out of.
What's the method name?
> What's the method name?

I don't understand.
(In reply to Steven Michaud from comment #19)
> > What's the method name?
> 
> I don't understand.

Your patch is too small. I don't know where you insert the new lines...
@smichaud: Plaease use -u8 (or corresponding mercurial settings) to generate the diff.
@masayuki: https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm?rev=09d86eb69c01#5685
I have "unified = 10" in the [diff] section of my .hgrc.

But in any case, as you've seen, the method I'm changing is -[ChildView keyDown:] in nsChildView.mm.
showfunc = 1 should do the trick. I seem to remember that this was optional, but I found it to make my patches quite a bit more readable.
Attached patch Fix — — Splinter Review 
But, oops, I generated that patch booted from a different partition, where I have no .hgrc file at all.  Here's a regenerated copy.
Attachment #8487520 - Attachment is obsolete: true
Attachment #8487520 - Flags: review?(masayuki)
Attachment #8488656 - Flags: review?(masayuki)
(Following up comment #17)

> I've started another set of tryserver builds:
> https://tbpl.mozilla.org/?tree=Try&rev=188045df26dd
>
> Hopefully the failures will just disappear down whatever hole they came out of.

The rats have gone back into their holes.  This time there are no unexplained failures.
Comment on attachment 8488656 [details] [diff] [review]
Fix

Looks okay to me.

However, if there is no side effect, please add similar code to flagsChange: *only* when one of modifier key flags is changing because it causes modifer keydown or keyup event which may be handled by web apps.
Attachment #8488656 - Flags: review?(masayuki) → review+
> However, if there is no side effect, please add similar code to
> flagsChange: *only* when one of modifier key flags is changing
> because it causes modifer keydown or keyup event which may be
> handled by web apps.

I doubt we should really do this.

If we did, we'd switch to the "active space" in step 8 (from comment
#8) every time we pressed Cmd, Opt/Alt, Shift, Ctrl or CapsLock
(instead of Cmd+l).  Safari doesn't do this.  Neither do Chrome or
Opera.

If we find a case where we actually need to do this, we should open a
new bug for it.
[Tracking Requested - why for this release]:

We need to decide how serious this bug is, and whether or not we want to uplift it to the 34 and 33 branches.

But before we do any uplifting, let's first let this bake for at least a week on the 35 branch, to see if there are any undesirable side effects.
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
Whiteboard: [STR in comment #8]
I'm tracking for 33 and 34 so that we will remember to make a decision on this. Tracking is not a guarantee that we will uplift the patch.
status-firefox32: --- → wontfix
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
(In reply to Steven Michaud from comment #27)
> > However, if there is no side effect, please add similar code to
> > flagsChange: *only* when one of modifier key flags is changing
> > because it causes modifer keydown or keyup event which may be
> > handled by web apps.
> 
> I doubt we should really do this.
> 
> If we did, we'd switch to the "active space" in step 8 (from comment
> #8) every time we pressed Cmd, Opt/Alt, Shift, Ctrl or CapsLock
> (instead of Cmd+l).  Safari doesn't do this.  Neither do Chrome or
> Opera.
> 
> If we find a case where we actually need to do this, we should open a
> new bug for it.

Sounds reasonable to me. Thanks!
https://hg.mozilla.org/mozilla-central/rev/7185bf064880
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Steven - This has not been on m-c for a week. This looks safe to uplift to Aurora. What do you think about uplifting to Beta?
Flags: needinfo?(smichaud)
> This has not been on m-c for a week.

I presume you mean "now" instead of "not" :-)

> This looks safe to uplift to Aurora.

I agree.

> What do you think about uplifting to Beta?

If there were any serious fallout, we should already have seen it.  So I think it should also be safe to uplift it to Beta.

This bug isn't often encountered.  But it's very disconcerting when it does happen.  The fix is simple and straightforward.  And the fact that Safari does more or less the same makes it even less likely to cause trouble.
Flags: needinfo?(smichaud)
Comment on attachment 8488656 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: Apple bug
[User impact if declined]: Seldom encountered but bad UI problem
[Describe test coverage new/current, TBPL]: One week baking on m-c
[Risks and why]: Low risk. The patch is simple and straightforward, and uses more or less the same workaround as Safari does.
[String/UUID change made/needed]: none
Attachment #8488656 - Flags: approval-mozilla-beta?
Attachment #8488656 - Flags: approval-mozilla-aurora?
(In reply to Steven Michaud from comment #34)
> > This has not been on m-c for a week.
> 
> I presume you mean "now" instead of "not" :-)

Yes. What a great typo. :)
Comment on attachment 8488656 [details] [diff] [review]
Fix

Beta+
Aurora+
Attachment #8488656 - Flags: approval-mozilla-beta?
Attachment #8488656 - Flags: approval-mozilla-beta+
Attachment #8488656 - Flags: approval-mozilla-aurora?
Attachment #8488656 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: