722676 - View menu item stays selected when returning from opened window

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Kevin Dangoor]

I think this is likely a Mac-only problem. I haven't tested this on Windows or Linux.

STR:

1. Select Tools->Web Developer->Style Editor
2. (Style Editor window pops up)
3. close the Style Editor or switch back to the main browser window
4. Tools menu is highlighted (appears selected) in the menu bar

The Tools menu should not appear selected at that point.

This also happens with Scratchpad from the same menu. Both Style Editor and Scratchpad change the menu bar. I've noticed that other pop up windows (Downloads, Page Info) don't change the menu bar and don't exhibit this behavior.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This is probably closely related to bug 688412, but may not be a dup -- I just reproduced it on OS X 10.5.8 using FF 9.0.1.  (Bug 688412 appears to only happen on OS X 10.7.X.)

Comment 2

[matthias koplenig [:metasieben]]

happens also with >view source<... possible dupe? bug 713357

Comment 4

[matthias koplenig [:metasieben]]

i'm aware that there are (far) more important things to work on, but is there any chance we could fix this?

Comment 5

[Brendan Eich [:brendan]]

Happens in Postbox (XUL app) and Firefox for me. Easy repro on Nightly:

cmd-shift-D
Cancel

and the Bookmarks menu item is still highlighted.

/be

Comment 6

[José Jeria]

Similar issues (or identical?) are bug 688412 and bug 722676.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Tnikkel, are you willing to take this bug?

I'm probably the one best suited to take it (who will be able to work on it most efficiently), but I'm currently busy with more serious bugs (including a difficult topcrasher).  So it'll be at least a week or two before I can get to it.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Thanks, Josh.

Comment 9

[Timothy Nikkel (:tnikkel)]

Does a patch like this https://bug562138.bugzilla.mozilla.org/attachment.cgi?id=503488 (unbitroted) that makes us paint synchronously during window show fix this?

Comment 10

[Todd Whiteman]

Attachment: Unbitrotted patch (from comment #9)

The patch did not help for me (tested on Mozilla 24 code base), the sticky menu highlighting remains - attached is the un-bitrotted patch for reference.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Todd Whiteman from comment #10)
> Created attachment 831688 [details] [diff] [review]
> Unbitrotted patch (from comment #9)
> 
> The patch did not help for me (tested on Mozilla 24 code base), the sticky
> menu highlighting remains - attached is the un-bitrotted patch for reference.

I realized after I said that that this is a persistent problem, not a flash of the wrong content, so it was unlikely to affect anything here.

Comment 12

[Josh Aas]

I can't reproduce this on 10.9, not receiving any more tips from people on how to do so. Given this, re-assigning.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

I don't have anything really urgent on my plate right now, so I should be able to get to this soon.

I haven't recently tried to reproduce it.  But I can test on all our supported versions of OS X -- which should help.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

With the following STR, I can reproduce this bug on OS X 10.7.5 and 10.8.5, but not 10.9 or 10.6.8:

1) Run Firefox
2) Press Cmd+u to display the View Source window.
3) Click on the red "close" button to close it.

Sometimes you need to repeat steps 2 and 3.

The variation must have something to do with timing differences in the behavior of native menus across different versions of OS X.

This is almost certainly an Apple bug.  But I hope it won't be too hard to hack out a workaround.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

This is definitely an Apple bug.  I've got a workaround that I'll post shortly, after it finishes its tryserver run.

The bug is present on OS X 10.7 and up, though for some reason I'm unable to reproduce its effects on OS X 10.9 (possibly because Apple has implemented its own workaround there).  Apparently a variant was also present on OS X 10.5 (per comment #1 I was able to reproduce this bug on that OS), but since we no longer support 10.5 I didn't look into that.

The bug can happen whenever you press a key combination that opens a child window/dialog, then close that window again.  Whenever you do that we change the native main menu (we create a new nsMenuBarX object, whose menu becomes the main menu in nsMenuBarX::Paint()) -- whether or not the contents of the menu actually changes.  Then when you close the child window/dialog we set the native main menu back to the menu (the nsMenuBarX object) that belongs to the parent window.  The bug is more likely to happen when the child window/dialog's menu is different from that of its parent window (as happens when you open a View Source window).  But it can still happen even without that (as for example when you use Cmd+d to open a "Bookmark this page" dialog).

The actual bug (Apple's bug) is a bug (or maybe a design flaw) in the following behavior:

Whenever you press a menu item's key combination (e.g. Cmd-u or Cmd-d), the OS momentarily highlights the menu item "above" it in the main menu (e.g. the Tools menu item).  This takes place in code called from the (documented) -[NSMenu performActionForItemAtIndex:(NSInteger)index] method (all of which is undocumented):  _NSHighlightCarbonMenu() and _NSUnhighlightCarbonMenu(), called from -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:(NSInteger)index].  _NSHighlightCarbonMenu() is always synchronous, but _NSUnhighlightCarbonMenu() is usually asynchronous -- it calls unhighlightMenuBarAtTime(), which usually calls itself back again 0.1 seconds later on a CFRunLoopTimer.

Problem is, sometimes the main menu has changed by the time the timer fires.  This means the menu item that needs to be unhighlighted is no longer present in the main menu.  So it remains semi-permanently highlighted -- until you quit Firefox or navigate through the appropriate menu by hand.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Workaround for Apple bug

Here's my workaround.  It postpones the first call to Paint() on an nsMenuBarX object by slightly more than the 0.1 second delay that Apple uses when calling unhighlightMenuBarAtTime().  This guarantees we'll never change the main menu for a new child window (created as the result of pressing a key combination) before Apple's timer has had a chance to unhighlight the appropriate top-level menu item in the parent window's main menu.

I might have limited my fix to OS X 10.7 and 10.8 (and I could still do so if a need is felt for this).  But this patch does no harm on 10.6 or 10.9 (subjectively the delay it causes is invisible, at least to me).  And I'm not entirely sure Apple's bug is absent from 10.9.

Josh, I asked you to review because my patch changes code that your wrote.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

The tryserver run hasn't finished yet, but so far it's going well:

https://tbpl.mozilla.org/?tree=Try&rev=f7368a3b70d3

Here's the opt build made from my patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f7368a3b70d3/try-macosx64/firefox-29.0a1.en-US.mac.dmg

Please try it out and let us know your results.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library for debugging

Figuring out this bug required a *lot* of reverse engineering.  And I went down several false trails before I found out what's really happening.

This interpose library doesn't take you down all those false trails.  But it does show proof of what I said in comment #15.

It's files tell you how to build it.

Comment 19

[Josh Aas]

Comment on attachment 8347449 [details] [diff] [review]
Workaround for Apple bug

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

::: widget/cocoa/nsMenuBarX.h
@@ +123,5 @@
>    nsTArray< nsAutoPtr<nsMenuX> > mMenuArray;
>    nsIWidget*         mParentWindow;        // [weak]
>    GeckoNSMenu*       mNativeMenu;            // root menu, representing entire menu bar
> +
> +  bool               mEverPainted;

Use of this variable prevents us from ever doing delayed drawing twice. I don't understand why, can you explain? What if someone has a window open, triggers the bug here with a child window's menu, closes that window, then does it again? Why would the bug not happen again?

Comment 20

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #19)

Every time a new window gets created, a new nsMenuBarX object gets created for it (which then gets garbage collected after the window is closed/destroyed).  So we don't ever need more than one instance of delayed menu "painting" (as the child window is opened) if the child window gets closed before the key combination that opened it gets used again.

But if you leave the child window open, in at least one case the same window can get focused again using the same key combination -- and in both cases the OS momentarily highlights a top-level menu item.  In this case we *would* (I think) need to use delayed "painting" more than once on the same nsMenuBarX object.

I hadn't thought of this.  But I'm glad you did ... at least indirectly.  Back to the drawing board :-(

Here's the "one case" I mentioned above:

1) Press Cmd+i to display the Page Info dialog/window.
2) Click on the browser window to make the Page Info window lose focus.
3) Press Cmd+i again.

The Tools menu gets briefly highlighted once again, and the same Page Info gets focused again.  (At least I *think* it's the same Page Info window.)

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Alternate workaround

Here's an alternative approach that fixes the problem mentioned in the 2nd paragraph of comment #20.  It also adds a small, unrelated optimization to nsMenuBarX::Paint() -- it doesn't reset the main menu if the "new" menu is the same as the "old" one.

I've started a tryserver build, whose results will be available here:
https://tbpl.mozilla.org/?tree=Try&rev=09f9c45ce2a5

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8349662 [details] [diff] [review]
Alternate workaround

I'm unwilling to work on this over the vacation.

Let's pick it up again when we get back.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8349662 [details] [diff] [review]
Alternate workaround

I'm back from vacation, and ready to start working on this again.

Comment 24

[Josh Aas]

Comment on attachment 8349662 [details] [diff] [review]
Alternate workaround

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

r+ with the following changes.

::: widget/cocoa/nsMenuBarX.mm
@@ +28,5 @@
>  #include "nsIDOMElement.h"
>  
>  NativeMenuItemTarget* nsMenuBarX::sNativeEventTarget = nil;
> +nsMenuBarX* nsMenuBarX::sLastGeckoMenuBarPainted = nullptr; // Weak
> +nsMenuBarX* nsMenuBarX::sLastGeckoMenuBarPaintDelayed = nullptr; // Weak

The name of this newer variable could be better. The existing variable parses as "the last menu bar that was painted". This new name parses as "the last menu bar paint(ed?) delayed", which sort of seems like a boolean variable.

How about "sDelayedOutgingGeckoMenuBar" or something like that?

@@ +773,5 @@
> +- (id)initWithTitle:(NSString *)aTitle
> +{
> +  if (self = [super initWithTitle:aTitle]) {
> +    self->mMenuBarOwner = nullptr;
> +    self->mDelayResignMainMenu = false;

Why the 'self->' here?

@@ +782,5 @@
> +- (id)initWithTitle:(NSString *)aTitle andMenuBarOwner:(nsMenuBarX *)aMenuBarOwner
> +{
> +  if (self = [super initWithTitle:aTitle]) {
> +    self->mMenuBarOwner = aMenuBarOwner;
> +    self->mDelayResignMainMenu = false;

Just access the member variables normally here as well.

@@ +787,5 @@
> +  }
> +  return self;
> +}
> +
> +- (nsMenuBarX *)menuBarOwner

This method seems unused, lets remove it.

@@ +792,5 @@
> +{
> +  return mMenuBarOwner;
> +}
> +
> +- (void)resetMenuBarOwner

I would change the name of this to "invalidateMenuBarOwner", just fits into more widely used and explicit terminology.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Attachment: Alternate workaround with Josh's suggested changes

Josh and I agreed on IRC that resetMenuBarOwner makes better sense than invalidateMenuBarOwner.  And I think that sCurrentPaintDelayedMenuBar is better than either sLastGeckoMenuBarPaintDelayed or sDelayedOutgingGeckoMenuBar.

Carrying forward Josh's r+.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8359430 [details] [diff] [review]
Alternate workaround with Josh's suggested changes

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91130616d83

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a91130616d83