461449 - Escape should cancel Tabsposé

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

STR:

1) Start Tabsposé in a window with 100+ tabs
2) Scream
3) Try hitting Esc

ER: Tabsposé stops
AR: Esc is impotent

Comment 1

[Chris Lawson (gone)]

To be fair, Esc doesn't cancel normal Exposé in the OS, either :-p (It does seem like it ought to, though.)

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Were Chris testing Exposé and not Dashboard, he'd not have made comment 1 ;)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Possible fix

Worse, pressing Esc (or Cmd-.) while while Tabsposé is loading *and* the current tab's page is loading cancels the current pageload rather than cancelling Tabsposé. :P  And it annoyed me tonight while working on docs such that I got distracted :P

We're hitting (at least) BrowserWindow's |cancel:|, and either we need to make sure that some other view handles Esc first, or we need to check right there to see if the TabThumbnailGridView is present (e.g., as BWC calls [mContentView tabThumbnailGridViewIsVisible]) and if so toggle the TTGV off (BWC does [mContentView toggleTabThumbnailGridView]), else do |stop:| like we currently do.

This fix adds some logic to BrowserWindow's |cancel:| to check to see if we're in Tabsposé mode (a new public BWC method |tabThumbnailViewIsVisible|, since I couldn't figure out the gymnastics to call [mContentView tabThumbnailGridViewIsVisible] directly like BWC does), and, if so, then calls BWC's |toggleTabThumbnailView:| to toggle us out of Tabsposé.

This appears to fix both the issue in comment 0 and the issue at the beginning of this comment.  However, I have no idea if this is the best way to fix this, or if this is even correct, or if the sender should be |nil| or something else, or anything (one day I'll have to really learn Cocoa), so… :P

Comment 4

[Stuart Morgan]

Comment on attachment 572258 [details] [diff] [review]
Possible fix

Sadness... more API for BWC :( We really need a better interaction between these two classes (like delegating abstract events to BWC instead of BW having to call everything manually) but that's out of scope; for the way we currently do things, this seems like the right fix. We could handle esc in the tabspose view, but that would only work if focus was in that view, which seems likely to be not what we want.

>+  if ([windowController tabThumbnailViewIsVisible]) {
>+   [windowController toggleTabThumbnailView:nil];
>+  }
>+  else {
>+    [windowController stop:nil];
>+  }

No {} on one-line if/else's. Also you are missing a space of indent in the if block.

sr=smorgan with that change though! nil is indeed the standard choice when calling IBAction methods programatically.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #4)

> Sadness... more API for BWC :( We really need a better interaction between
> these two classes (like delegating abstract events to BWC instead of BW
> having to call everything manually) but that's out of scope

Should we get a bug filed on this general problem?

> We could handle esc in
> the tabspose view, but that would only work if focus was in that view, which
> seems likely to be not what we want.

Right; pretty sure it'd fail in both cases I've complained about here, though I haven't checked explicitly.

> No {} on one-line if/else's.

Oops; I keep forgetting when I take out the NSLog that I need to fix the braces, too ;)

http://hg.mozilla.org/camino/rev/58d6a8771d67 with the two style items corrected.