can't paste into a javascript prompt, the paste can bubble up to the focused text area on page instead

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: Josh Aas)

Tracking

({regression})

Trunk
x86
Mac OS X
regression
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

when attempting to paste into a javascript prompt, the paste goes to text area on page instead

I'm seeing this on my fresh, trunk, debug mac os x build.  I'm using blogger.com, attempting to author a new post.

I try to set the url for a link within a post, and I get a js prompt.  When I attempt to paste in the link, the paste shows up in the text area of the page I was on, instead of the text field of the prompt sheet.

I'll try to create a simplified test case.
Summary: when attempting to paste into a javascript prompt, the paste goes to text area on page instead → can't paste into a javascript prompt, the paste can bubble up to the focused text area on page instead
Blocks: 326469
Created attachment 240954 [details]
test case

try this:

1) have some text in your clipboard and load the test case
2) click in the text area (and dismiss the prompt)
3) type in the text area, so it has focus
4) click again in the text area
5) command v to paste
6) clipboard text goes to text area, and not prompt field
(Assignee)

Comment 2

11 years ago
The test case in comment #1 does reproduce the problem for me.
(Assignee)

Comment 3

11 years ago
I'm pretty sure this is happening because the menu bar displayed when the sheet is up is the menu bar for the parent window.
In carbon widget we're disabling the entire menubar when a sheet is being opened.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
*** Bug 360290 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

11 years ago
Created attachment 248559 [details] [diff] [review]
fix v1.0
Attachment #248559 - Flags: review?(mano)

Comment 7

11 years ago
Comment on attachment 248559 [details] [diff] [review]
fix v1.0

>Index: nsCocoaWindow.mm
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v
>retrieving revision 1.76
>diff -U8 -r1.76 nsCocoaWindow.mm
>--- nsCocoaWindow.mm	12 Dec 2006 18:55:24 -0000	1.76
>+++ nsCocoaWindow.mm	13 Dec 2006 22:07:29 -0000
>@@ -1090,16 +1090,65 @@
>   nsCOMPtr<nsIMenuBar> hiddenWindowMenuBar = GetHiddenWindowMenuBar();
>   if (hiddenWindowMenuBar) {
>     // printf("painting hidden window menu bar due to window losing main status\n");
>     hiddenWindowMenuBar->Paint();
>   }
> }
> 
> 
>+- (void)paintMenubarForWindow:(NSWindow*)aWindow
>+{
>+  id windowDelegate = [aWindow delegate];
>+  if ([windowDelegate class] != [self class])
>+    return;

Why this? WindowDelegate is always set as the delegate for all windows we create. Even if it were not, why compare the classes?

>+
>+  nsCocoaWindow* geckoWidget = [windowDelegate geckoWidget];
>+  if (!geckoWidget)
>+    return;
>+
>+  nsIMenuBar* geckoMenuBar = geckoWidget->GetMenuBar();

In which cases will geckoMenuBar be null here? Might be worth putting a comment here to explain how it works.

>+  if (geckoMenuBar) {
>+    geckoMenuBar->Paint();
>+  }
>+  else {
>+    NSMenu* newMenuBar = [[NSMenu alloc] init];
>+    NSMenuItem* newMenuItem = [[NSMenuItem alloc] initWithTitle:@"" action:nil keyEquivalent:@""];

These two above are leaked, AFAICS.

>+    [newMenuBar addItem:newMenuItem];
>+    
>+    NSMenuItem* applicationMenuItem = [[NSApp mainMenu] itemAtIndex:0];
>+    NSMenu* applicationMenu = [applicationMenuItem submenu];
>+
>+    // submenus cannot be submenus of more than one menu item, so we have
>+    // to first unhook the app menu before hooking it up to its new item
>+    [applicationMenuItem setSubmenu:nil];
>+    [newMenuItem setSubmenu:applicationMenu];
>+    [NSApp setMainMenu:newMenuBar];
>+  }
>+}
>+
>+
>+- (void)windowDidBecomeKey:(NSNotification *)aNotification
>+{
>+  NSWindow* window = [aNotification object];
>+  if ([window isSheet])
>+    [self paintMenubarForWindow:window];
>+}
>+
>+
>+- (void)windowDidResignKey:(NSNotification *)aNotification
>+{
>+  // If a sheet just resigned key then we should paint the menu bar
>+  // for whatever window is now main.
>+  NSWindow* window = [aNotification object];
>+  if ([window isSheet])
>+    [self paintMenubarForWindow:[NSApp mainWindow]];
>+}
>+
>+
> - (void)windowWillMove:(NSNotification *)aNotification
> {
>   // roll up any popups
>   if (gRollupListener != nsnull && gRollupWidget != nsnull)
>     gRollupListener->Rollup();
> }
> 
> 
>@@ -1170,9 +1219,15 @@
>   // we set that in nsCocoaWindow::Show. 'contextInfo' is always the top-level
>   // window, not another sheet itself.
>   [[sheet delegate] sendLostFocusAndDeactivate];
>   [sheet orderOut:self];
>   [[(NSWindow*)contextInfo delegate] sendGotFocusAndActivate];
> }
> 
> 
>+- (nsCocoaWindow*)geckoWidget;
>+{
>+  return mGeckoWindow;
>+}
>+
>+
> @end
(Assignee)

Comment 8

11 years ago
Oops - i had a note to self about doing the autoreleasing later, I forgot to do that before I posted the patch. Thanks!
(Assignee)

Comment 9

11 years ago
Created attachment 248566 [details] [diff] [review]
fix v1.0.1

The checks that hwaara asked about I just put there to be safe. If people are really against them I'll remove them, but I don't think they are problematic.
Attachment #248559 - Attachment is obsolete: true
Attachment #248566 - Flags: review?(mano)
Attachment #248559 - Flags: review?(mano)

Comment 10

11 years ago
(In reply to comment #9)
> Created an attachment (id=248566) [edit]
> fix v1.0.1
> 
> The checks that hwaara asked about I just put there to be safe. If people are
> really against them I'll remove them, but I don't think they are problematic.

Sure, safety is good. If you want to catch errors, I think adding an assertion is better. Returning early won't help us catch any future bug. (For example NSAssert([window delegate] == self, @"Why are we not delegate?"))

Also, I still think a comment about how this works is important, I couldn't understand from the patch, that's why I was asking...
(Assignee)

Updated

11 years ago
Attachment #248566 - Flags: review?(mano) → review?(mark)
(Assignee)

Updated

11 years ago
Attachment #248566 - Flags: review?(mark)
(Assignee)

Comment 11

11 years ago
Created attachment 248603 [details] [diff] [review]
fix v1.1
Attachment #248566 - Attachment is obsolete: true
Attachment #248603 - Flags: review?(hwaara)

Comment 12

11 years ago
Comment on attachment 248603 [details] [diff] [review]
fix v1.1

Looks good, I like the assertions! :)

>+- (void)windowDidResignKey:(NSNotification *)aNotification
>+{
>+  // If a sheet just resigned key then we should paint the menu bar
>+  // for whatever window is now main.
>+  NSWindow* window = [aNotification object];
>+  if ([window isSheet])
>+    [WindowDelegate paintMenubarForWindow:[NSApp mainWindow]];

It's dangerous to assume that [NSApp mainWindow] is one of our windows. For third-party apps (eudora comes to mind) that may use XUL windows in addition to other (potentially native) windows, this will break.
Attachment #248603 - Flags: review?(hwaara) → review-
(Assignee)

Comment 13

11 years ago
Before, when I had checks that returned if a window didn't have the right type of delegate your concern was taken care of, but you had me remove those so now it is an issue again. Undoing the assertions and going back to those checks would be the best way to resolve that issue I think.

Note: I should nil check the mainWindow return before painting it.

Comment 14

11 years ago
(In reply to comment #13)
> Before, when I had checks that returned if a window didn't have the right type
> of delegate your concern was taken care of, but you had me remove those so now
> it is an issue again. Undoing the assertions and going back to those checks
> would be the best way to resolve that issue I think.

Yes, didn't realize before that your delegate check was to protect from [NSApp mainWindow].

If you want to make sure the window is a gecko window, I think there are more direct ways of doing that than checking the class of the delegate. How about [aWindow respondsToSelector:@selector(geckoWidget)]; ?

Childview uses this approach to test if a window is a mozWindow.

Comment 15

11 years ago
Josh, do javascript prompts have the window type of popup? 

If so, it's possible that the real reason for this bug is similar to the cause of bug 355631. nsChildView::DispatchEvent() blindly redirects all events to the parent widget. 

My thought is that since key events would then be redirected to the parent widget, it would explain the behavior in comment 1.

Comment 16

11 years ago
Comment on attachment 248603 [details] [diff] [review]
fix v1.1

The class check makes sense since the method is a class method.  

r=me with a bug filed for some more generic & reusable way to check if a window is a mozWindow (since we do it in ChildView too, and will in more places as time goes on)
Attachment #248603 - Flags: review- → review+
(Assignee)

Updated

11 years ago
Attachment #248603 - Flags: review?(mark)
(Assignee)

Comment 17

11 years ago
+  id windowDelegate = [aWindow delegate];
+  NS_ASSERTION([windowDelegate class] == [self class], "Window delegate does not exist or is not of type WindowDelegate!");

I am changing that to:

+  // make sure we only act on windows that have this kind of
+  // object as a delegate
+  id windowDelegate = [aWindow delegate];
+  if ([windowDelegate class] != [self class])
+    return;

Comment 18

11 years ago
(In reply to comment #17)
> +  id windowDelegate = [aWindow delegate];
> +  NS_ASSERTION([windowDelegate class] == [self class], "Window delegate does
> not exist or is not of type WindowDelegate!");
> 
> I am changing that to:
> 
> +  // make sure we only act on windows that have this kind of
> +  // object as a delegate
> +  id windowDelegate = [aWindow delegate];
> +  if ([windowDelegate class] != [self class])
> +    return;

Why remove the assertion? If that check should _never_ return true, I don't see how you would ever detect the error except for weird indirect bugs.
(Assignee)

Comment 19

11 years ago
> Why remove the assertion? If that check should _never_ return true, I don't see
> how you would ever detect the error except for weird indirect bugs.

You said yourself that there is a possibility that native windows might be around, in which case it would not be an error for that assertion to fail. That is the whole point of making it a check instead of an assertion in the first place.

"For third-party apps (eudora comes to mind) that may use XUL windows in addition to other (potentially native) windows, this will break."

Comment 20

11 years ago
Sorry Josh, you're right about that; I should keep out of this bug from now on... ;-)

Comment 21

11 years ago
Comment on attachment 248603 [details] [diff] [review]
fix v1.1

>Index: nsCocoaWindow.mm

>++ (void)paintMenubarForWindow:(NSWindow*)aWindow

>+    NSMenu* oldMainMenu = [NSApp mainMenu];
>+    NSMenuItem* applicationMenuItem = [oldMainMenu itemAtIndex:0];
>+    NSMenu* applicationMenu = [applicationMenuItem submenu];
>+    
>+    // submenus cannot be submenus of more than one menu item, so we have
>+    // to first unhook the app menu before hooking it up to its new item
>+    [applicationMenuItem setSubmenu:nil];

How does the old main menu bar get its application submenu back?
(Assignee)

Comment 22

11 years ago
Created attachment 249132 [details] [diff] [review]
fix v1.2
Attachment #248603 - Attachment is obsolete: true
Attachment #249132 - Flags: review?(mark)
Attachment #248603 - Flags: review?(mark)

Comment 23

11 years ago
Comment on attachment 249132 [details] [diff] [review]
fix v1.2

ar-plus
Attachment #249132 - Flags: review?(mark) → review+
Comment on attachment 249132 [details] [diff] [review]
fix v1.2

sr=pink
Attachment #249132 - Flags: superreview+
(Assignee)

Comment 25

11 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.