Closed Bug 473030 Opened 15 years ago Closed 14 years ago

Cmd+Shift+Y creates two sticky notes (Services -> Make New Sticky Note only creates one)

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: tom.dyas)

References

Details

(Whiteboard: [should not block])

Attachments

(3 files, 3 obsolete files)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090110 Minefield/3.2a1pre

Steps to reproduce:
1. Select a word (in Firefox).
2. Press Cmd+Shift+Y.

Result: Two identical sticky notes are created.  They may appear in the same place on the screen or in different places.

Expected: Only one sticky note should be created.
Does the same behavior occur if you press Cmd+Shift+Y while in another application?  (Just want to make sure it is not a problem with Stickies.)
In Safari and TextEdit, Cmd+Shift+Y only creates one sticky.
I verified this bug using a checkout from today.  When I invoked the "Make New Sticky Note" service directly from the Services menu, only one Sticky note was created.  When I used the accelerator key, it produced two sticky notes. This leads me to believe that the accelerator keypress is somehow being handled twice.
After a little more work, I verified that this behavior is not limited to Stickies, but rather any service activated by an accelerator key.  (I used my service debugging application, "Pasteboard Inspector," as the test service.)  When activated from the menu, only one inspector window is displayed.  When activated by an accelerator key, two inspector windows are displayed, which means the service was activated twice.

Josh: I'm not that familiar with the Gecko handling of performKeyEquivalent:.  Any ideas as to why an accelerator key might be processed twice?
Tom, is your "Pasteboard Inspector" publicly available?  If so, where?

By the way, I'm also able to repro this bug -- though only on the trunk (which is currently the only place Services support is available).  Stickies is bundled with the OS (at least with OS X 10.5.6).
Attached patch logging patchSplinter Review
I believe I determined what is going on.  The single press of the accelerator key is being translated by the widget library into two calls into the Cocoa menu, which results in the service being activated twice.

I used the attached patch to add some logging and this is the result:

nsCocoaWindow.mm: performKeyEquivalent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
nsChiildView.mm: performKeyEquivalent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
nsChiildView.mm: performKeyEquivalent #1
nsChiildView.mm: performKeyEquivalent #1.1
nsChiildView.mm: performKeyEquivalent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
nsChiildView.mm: performKeyEquivalent #1
nsChiildView.mm: performKeyEquivalent #2
nsChiildView.mm: performKeyEquivalent #3
nsChiildView.mm: performKeyEquivalent #4
nsChiildView.mm: performKeyEquivalent #5.1
nsMenuBarX.mm: performMenuUserInterfaceEffectsForEvent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
nsMenuBarX.mm: performKeyEquivalent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
SERVICE: writeSelectionToPasteboard:types:
nsMenuBarX.mm: performKeyEquivalent returing YES
nsChiildView.mm: performKeyEquivalent #5.2
nsChiildView.mm: performKeyEquivalent #6modifierFlags=1179648, keyCode=34, keyDownNeverFiredEvent=true
nsChiildView.mm: performKeyEquivalent #7
nsChiildView.mm: performKeyEquivalent #8
nsChildView.mm: processKeyDownEvent:theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34 keyEquiv=YES
nsMenuBarX.mm: performKeyEquivalent: theEvent=NSEvent: type=KeyDown loc=(0,864) time=72569.0 flags=0x12010a win=0x0 winNum=7702 ctxt=0xf67f chars="i" unmodchars="I" repeat=0 keyCode=34
SERVICE: writeSelectionToPasteboard:types:
nsMenuBarX.mm: performKeyEquivalent returing YES

What is happening is that the service is activated indirectly as a result of the call to:
[(GeckoNSMenu*)mainMenu performMenuUserInterfaceEffectsForEvent:theEvent];
in nsChildView.mm (between 5.1 and 5.2 logging statements). That call returns void so there is no way for performKeyEquivalent to know to bail out if the accelerator key was handled by the Services menu.

The execution then continues until nsChildView.mm:performKeyEquivalent calls nsChildView.mm:performKeyDownEvent which indirectly calls into the menu code again, thereby duplicating the service activation.
Yeah, this is going to be tricky to fix. I can take a look in a week or two.
(In reply to comment #5)
> Tom, is your "Pasteboard Inspector" publicly available?  If so, where?
> 
> By the way, I'm also able to repro this bug -- though only on the trunk (which
> is currently the only place Services support is available).  Stickies is
> bundled with the OS (at least with OS X 10.5.6).

I just uploaded it to my domain.  Fetch source code from http://www.zecador.org/pbinspector.zip

Compile in Xcode and then drop the application bundle into ~/Library/Services (or the equivalent system folder).  Run the application once manually and choose "Inspect -> Update Dynamic Services" to force Mac OS X to rebuild the Services menu for all applications (or log out and then log in again).

Minor bugs: When you use the accelerator keypress (Cmd+Shift+I), only one window pops on top and the windows do not cascade. Bring all windows to front and move the top window a little bit to see how the service is activated twice from Firefox.
(In reply to comment #8)
> Compile in Xcode and then drop the application bundle into ~/Library/Services
> (or the equivalent system folder).  Run the application once manually and
> choose "Inspect -> Update Dynamic Services" to force Mac OS X to rebuild the
> Services menu for all applications (or log out and then log in again).

It'll also work obviously if you drop the bundle into the /Applications directory.
This bug makes me sad.  I set "New To-do Containing Selection as Note" to Cmd+Ctrl+N, but since Firefox triggers the service action twice, Things gets confused and doesn't leave the entry panel up at all.

Josh, have you had a chance to look at this?
Assignee: joshmoz → nobody
Has any effort been made to fix this bug?

Services support was not released with 3.5, but will be with 3.6 so this bug is going to end up being encountered publicly.
Flags: blocking1.9.2?
Just tested trunk, the bug still exists.
Whiteboard: [should not block]
Attached patch fix v1 (obsolete) — Splinter Review
This patch fixes the bug (at least on 10.6).

It observes a notification sent right before a System Service is activated and increments a count. This count is then used to determine when the key equivalent processing should be stopped.

I overrode pKE on the Services menu as well in case 10.5 or 10.4 calls that instead. The notification is what is triggered on 10.6 (and not pKE which is odd).
Assignee: nobody → tom.dyas
Status: NEW → ASSIGNED
Attachment #410156 - Flags: review?(joshmoz)
Flags: blocking1.9.2? → wanted1.9.2?
So, specifically, the problem is...

"performMenuUserInterfaceEffectsForEvent:" calls "performKeyEquivalent:" after setting "gActOnSpecialCommands" to "NO". This tells the command receiver to ignore the command, but since we don't own the service menu items they don't know to ignore the command, they don't check "gActOnSpecialCommands". They act on the command, we think they didn't, and then we dispatch it again.

Tom - agree with this?
Sorry, slight problem with my statement at the end. I think we perform the command and then we have the failure in the menu system, not the other way around. Either way, my point is that the unexpected execution comes from "gActOnSpecialCommands" not being respected by the service menu items.

I wonder if we can just find a way to do nothing to menu items we don't own in "performMenuUserInterfaceEffectsForEvent:".
Correct.  Your first formulation is right; the extra invocation of the service comes from the call to |performMenuUserInterfaceEffectsForEvent:|.  When I stopped all key equivalents from passing that call, Gecko never received its key equivalents (like Cmd-T) so the command is performed by Gecko after the call to |performMenuUserInterfaceEffectsForEvent:|.

The reason why I used an observer is because I did not find a way to intercept the invocation of the NSMenuItem's on the Services menu and then veto the action.  (For whatever reason, the |performKeyEquivalent:| method in my NSMenu-subclass used as the Services menu was not called. Other overridden methods (not present in the final patch) were called.)

Since NSMenu will post NSMenuWillSendActionNotification and NSMenuDidSendActionNotification for all NSMenuItem's activated including the ones owned by the Services menu, I just hooked the notification and used the counter to detect when a Service was activated.  This can actually be generalized since the notification receives the specific NSMenuItem in the userInfo dictionary and the containing NSMenu as the notification object. If the NSMenuItem's target is not the Gecko shared menu item target, we know then that we don't own that menu item. Where we install the observer and how this integrates into |performMenuUserInterfaceEffectsForEvent:| can probably differ from how I did it in my patch.

Maybe |performMenuUserInterfaceEffectsForEvent:| should detect the situation itself and return YES if an action actually occurred and NO otherwise?  (as opposed to how I modified it to return the result of the underlying call to |performKeyEquivalent:|)
In case you want to see what happens with the Services menu, this is a patch which intercepts most method calls made on the Services menu instance.
Tried overriding |performActionForItemAtIndex:| in the custom NSMenu subclass. I found that this method is called when a Service menu item is selected using the mouse, but, for some reason, not when the item's key equivalent is pressed. The patch would actually be simpler with that override worked. Ah well.
Attached patch fix v2 (obsolete) — Splinter Review
This patch fixes the bug and does not change the semantics of |performMenuUserInterfaceEffectsForEvent:| at all.

I tried observing the NSMenuWillSendAction and NSMenuDidSendAction notifications and clearing the target/action as necessary when we wanted to stop a key equivalent.  However, Cocoa has already retrieved the target/action before the NSMenuWillSendAction notification so that method did not work.

This patch makes each NSMenuItem on the Services menu become a custom subclass of NSMenuItem. It redirects the target/action to a dummy whenever we want the Services menu to eat a key equivalent. (A dummy is used so that a NULL selector is not passed up if an action was set.)

Does someone have a Mac OS X 10.4 or 10.5 system available?  I want to make sure that object_setClass is available in the Obj-C runtime on those systems.  If not, the patch will need to set "isa" directly.
Attachment #410156 - Attachment is obsolete: true
Attachment #413780 - Flags: review?(joshmoz)
Attachment #410156 - Flags: review?(joshmoz)
Found the right docs.  object_setClass was added in 10.5.  Will probably need to set "isa" directly on 10.4 then.

http://developer.apple.com/mac/library/documentation/cocoa/Reference/ObjCRuntimeRef/Articles/ocr10_5delta.html
Attached patch fix v2.1 (obsolete) — Splinter Review
Add an #ifdef so that we directly swizzle "isa" on 10.4.
Attachment #413780 - Attachment is obsolete: true
Attachment #413850 - Flags: review?(joshmoz)
Attachment #413780 - Flags: review?(joshmoz)
I've been thinking about your patch, obviously we don't want to have to resort to this sort of thing but I have not come up with anything better yet.

If we go this route we'd want to check the class on the item we're overriding and if it isn't a regular NSMenuItem then we probably want to leave it alone. Better to have this bug than totally mess up any subclassing Apple might do/allow.

On 10.6 those items already look special, have you checked to see that they are regular NSMenuItem objects? How does the service menu on 10.6 get categories on the left of its display with our existing code if it only has regular NSMenu and NSMenuItem objects involved?
(In reply to comment #22)
> If we go this route we'd want to check the class on the item we're overriding
> and if it isn't a regular NSMenuItem then we probably want to leave it alone.
> Better to have this bug than totally mess up any subclassing Apple might
> do/allow.

The patch already does this.  It checks the class of the menu item directly against [NSMenuItem class].  (It avoids [menuitem isKindOfClass:[NSMenuItem class]] to avoid the subclass problem.)

> On 10.6 those items already look special, have you checked to see that they are
> regular NSMenuItem objects? How does the service menu on 10.6 get categories on
> the left of its display with our existing code if it only has regular NSMenu
> and NSMenuItem objects involved?

It uses a custom view.
I think we should take whatever patch we decide on for the 1.9.2 branch, wanted1.9.2+.
Flags: wanted1.9.2? → wanted1.9.2+
Comment on attachment 413850 [details] [diff] [review]
fix v2.1

>+- (id) target
>+{
>+  id realTarget = [super target];
>+  if (gActOnSpecialCommands)
>+    return realTarget;
>+  else {
>+    return realTarget != nil ? self : nil;
>+  }
>+}

Remove the curly braces around the "else" code.

Thanks Tom!
Attachment #413850 - Flags: review?(joshmoz) → review+
Attached patch fix v2.2Splinter Review
Modified as per reviewer comment.
Attachment #413850 - Attachment is obsolete: true
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/416facdd97da
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #419492 - Flags: approval1.9.2.1?
Attachment #419492 - Flags: approval1.9.2.2? → approval1.9.2.3?
Comment on attachment 419492 [details] [diff] [review]
fix v2.2

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.
Attachment #419492 - Flags: approval1.9.2.4?
I'm not sure I completely understand the versioning here, but in my up-to-date installation of Firefox I still get the behaviour that two notes (in Stickies as well as in SlipBox) are created.
(In reply to comment #30)
> I'm not sure I completely understand the versioning here, but in my up-to-date
> installation of Firefox I still get the behaviour that two notes (in Stickies
> as well as in SlipBox) are created.

The patch has only been landed on the "trunk" for mozilla-central.  So Firefox 3.7 (or whatever it will be called) will likely pick up the patch, but Firefox 3.6.6 does not have the patch.  That is what I believe comment 29 is asking: whether we want this patch landed for Gecko 1.9.2.x (Firefox 3.6.x).
Comment on attachment 419492 [details] [diff] [review]
fix v2.2

I think we should take this for 1.9.2. Any Mac OS X services user is likely to hit this and the problem is pretty bad. This has been on trunk for a while without any regressions that I can recall.
Attachment #419492 - Flags: approval1.9.2.8?
Attachment #419492 - Flags: approval1.9.2.9? → approval1.9.2.11?
Comment on attachment 419492 [details] [diff] [review]
fix v2.2

Not going to get this one in 1.9.2.11
Attachment #419492 - Flags: approval1.9.2.11? → approval1.9.2.11-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: