469414 - Bookmarks inside subfolders in the dock menu don't bring Camino forward

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Mehmet]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.6pre) Gecko/2008121200 Camino/2.0b1pre (like Firefox/3.0.6pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.6pre) Gecko/2008121200 Camino/2.0b1pre (like Firefox/3.0.6pre)

Hello Camino-Team.

this bug https://bugzilla.mozilla.org/show_bug.cgi?id=381846 is fixed, but my problem relates to this.

Single links opens now in the front (thanks Camino-Team!!!), but when you select a link from a folder, the window still opens in the background.

You must choose first the complete bookmark bar as Dock Menu. (see enclosed pictures)

http://www.bilder-upload.eu/show.php?file=q6w6mdcB1fjGmbp.png

and

http://www.bilder-upload.eu/show.php?file=Knu1QBDSfDWG43I.png



Thank you in advance !!!!

Regards
Mehmet





Reproducible: Always

Steps to Reproduce:
1. create a folder in to your bookmark bar and save a link in it.
2. open "bookmark manager"
3. right click in the tree on "bookmark bar"
4. then select "use as Dock Menu"
5. Bring another Application to the front!!!!!!
6. Now right click on the "camino dock icon"
7. select the link (inside the folder) from the camino dock menu 
Actual Results:  
The link, which was in the folder, opens in the background, behind the other application you brought to front before.

Expected Results:  
The link should be open in front of all applications.

Comment 1

[Stuart Morgan]

We'll need to make a method that walks up the menu hierarchy checking at each level to see if that's the dock menu.

Comment 2

[Chris Lawson (gone)]

I can take this if no one else gets to it within the next week or two.

Comment 3

[Christopher Henderson]

Bug 469788 would also benefit from such a method described in Comment 1.

Comment 4

[Chris Lawson (gone)]

Attachment: fix v1.0

This oughta do it.

Comment 5

[Christopher Henderson]

Comment on attachment 359966 [details] [diff] [review]
fix v1.0

Code looks good, and it fixes the bug.

Comment 6

[Chris Lawson (gone)]

Comment on attachment 359966 [details] [diff] [review]
fix v1.0

Heh, totally forgot to ask for sr on this, probably because I got tied up with bug 469788 at the time (which incorporated this fix, but which I've since lost the patch for). Let's get this fix in now and then I'll fix that one.

Comment 7

[Chris Lawson (gone)]

Comment on attachment 359966 [details] [diff] [review]
fix v1.0

Actually, is it necessary to walk the menu hierarchy at all?

The only menu that can be activated in Camino while Camino is not the foreground (i.e., active) application is the Dock menu, right?

So if we end up in any menu-triggered code and [NSApp isActive] == NO, we know we came in through the Dock menu, no walking needed.

Unless I'm missing something...

Comment 8

[Chris Lawson (gone)]

Oh, heh, apparently OS X (at least 10.5, dunno about 10.4) allows you to trigger contextual menus on background apps, too. You couldn't get to the "New Window" code that way, but you could definitely get to a bookmark/folder/tab group in the bookmarks bar. I don't know how we'd want to handle those cases.

Comment 9

[Chris Lawson (gone)]

Comment on attachment 359966 [details] [diff] [review]
fix v1.0

(In reply to Chris Lawson from comment #8)
> Oh, heh, apparently OS X (at least 10.5, dunno about 10.4) allows you to
> trigger contextual menus on background apps, too.

I don't have an easy way to test 10.5 or 10.4 at the moment (I doubt 10.4 has changed since this comment was written, but 10.5 might have), but 10.6 definitely does *not* let you trigger CMs on background apps. It brings the application to the foreground, then displays a CM. I assume 10.7 is the same, but someone who has Lion should confirm that for posterity.

That said, I probably should have tagged someone else for sr years ago, because this is a fairly simple fix.

Comment 10

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

(In reply to Chris Lawson from comment #8)
> Oh, heh, apparently OS X (at least 10.5, dunno about 10.4) allows you to
> trigger contextual menus on background apps, too. You couldn't get to the
> "New Window" code that way, but you could definitely get to a
> bookmark/folder/tab group in the bookmarks bar. I don't know how we'd want
> to handle those cases.

One of which options is "Open in New Window"; dunno offhand how that's routed.  10.3 allowed CMs on background apps, so I suspect 10.4 did as well; I can re-verify 10.5 later.

Comment 11

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

So, it doesn't seem to work for *us* on 10.5 anymore, but other apps running on 10.5 do apparently allow CMs while in the background in certain, but not all, situations (e.g., Colloquy's tab bar does not, but its Connections table view does; its WebView does only if there is an existing selection, etc.  Safari behaves similarly).

That said, it doesn't seem like it'd be too hard to actually walk the parents:

- (BOOL)isChildOfDockMenu:(BookmarkFolder*)currentParentFolder
{
  while (!([currentParentFolder isRoot]) {
    if ([currentParentFolder isDockMenu])
      return YES;
    else
     [self isChildOfDockMenu:[currentParentFolder parent]];
  }
  return NO;
}

or somesuch (not sure if I have the syntax right).

Comment 12

[philippe (part-time)]

(In reply to Chris Lawson from comment #9)

> I don't have an easy way to test 10.5 or 10.4 at the moment (I doubt 10.4
> has changed since this comment was written, but 10.5 might have), but 10.6
> definitely does *not* let you trigger CMs on background apps. It brings the
> application to the foreground, then displays a CM. I assume 10.7 is the
> same, but someone who has Lion should confirm that for posterity.

Hmm, unless I'm misunderstanding something… On both 10.6 and 10.7 I can trigger CMs on background apps in most if not all cases (Textedit, Safari, Finder, etc - and Camino). There is a difference though. It works perfectly from a right-click [1] but a control-click brings the apps forward and then calls the CM. Strangely (? uh, Gecko ?) Camino works in both cases.

[1] Fruit Co manufactured rodent in use here.

Comment 13

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

OK, I must be going crazy; checking again on 10.3, we're coming forward (this Mac only has ctrl key, old-old-school trackpad, and no rodent).

On 10.5, with "For secondary clicks, place two fingers on the trackpad then click the button" checked, two-finger click to a Bookmark Bar item does bring up a CM when in the background, whereas ctrl-click brings us forward.

(In the content area, ctrl-click just brings the app forward and doesn't show the CM, two-finger click typically behaves as two-finger click to chrome does--but I've also had it fail and just bring the app forward like ctrl-click does.)

:crazycrazy:

Comment 14

[Chris Lawson (gone)]

Attachment: fix v1.1

Unbitrotted (turns out patches against CVS don't apply, who knew?) and updated with a clearer argument name. Carrying over the r+ from before and re-requesting sr.

Comment 15

[Stuart Morgan]

Comment on attachment 603968 [details] [diff] [review]
fix v1.1

Review of attachment 603968 [details] [diff] [review]:
-----------------------------------------------------------------

sr=smorgan with nits.

::: src/extensions/NSMenu+Utils.h
@@ +99,5 @@
>  
>  // copy the title and enabled state from the given item
>  - (void)takeStateFromItem:(NSMenuItem*)inItem;
>  
> +// See whether a menu item has a given menu somewhere in its hierarchy

The comment is less clear that method name (since it sounds like it would be true if the parenting were reversed). Just say:
// Returns YES if the this item is a descendant (including direct child) of |aMenu|.

::: src/extensions/NSMenu+Utils.m
@@ +399,5 @@
> +  NSMenu* ancestor = [self menu];
> +  while (ancestor) {
> +    if (ancestor == aMenu)
> +      return YES;
> +    else

No else after a return, by degree of our glorious leader.

@@ +403,5 @@
> +    else
> +      ancestor = [ancestor supermenu];
> +  }
> +  // If we got here, ancestor was nil and we walked all the way
> +  // up the menu hierarchy without finding a match.

I'd take this comment out, since the code it is explaining is trivial, but if you feel strongly about leaving it that's fine.

Comment 16

[Chris Lawson (gone)]

Attachment: fix v1.11

Comment 17

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

http://hg.mozilla.org/camino/rev/88a7fb8c624d

This is your chicken-wrangler with a friendly reminder that it's not necessary (and is in fact confusing) to "carry forward" the r+/sr+ :-)