Last Comment Bug 717111 - Plain text editor context menu broken since landing of fix to bug 702019
: Plain text editor context menu broken since landing of fix to bug 702019
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Composer (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.9
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 702019
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 17:12 PST by Ian Neal
Modified: 2012-01-23 15:33 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
patch (1.56 KB, patch)
2012-01-14 18:10 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Proposed patch [Checkin: Comments 17 and 18] (2.17 KB, patch)
2012-01-15 09:02 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Review

Description Ian Neal 2012-01-10 17:12:43 PST
When the fix to bug 702019 landed it broke the context menu in the plain text editor. The context menu is now very long and in the error console you get:
Error: gBrowser is not defined
Source File: chrome://debugqa/content/debugQATextEditorShell.xul
Line: 1
Even though gBrowser gets passed it does not seem to get used, so either we should not pass it or we should define it for the plain text editor.
Comment 1 Philip Chee 2012-01-10 19:18:31 PST
> +               onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu(this, gBrowser, event.shiftKey); return gContextMenu.shouldDisplay;"

Since we never use gBrowser we can just do:
... new nsContextMenu(this, null, event.shiftKey); ...
Comment 2 Jens Hatlak (:InvisibleSmiley) 2012-01-10 23:45:38 PST
(In reply to Philip Chee from comment #1)
> > +               onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu(this, gBrowser, event.shiftKey); return gContextMenu.shouldDisplay;"
> 
> Since we never use gBrowser we can just do:
> ... new nsContextMenu(this, null, event.shiftKey); ...

I know we don't, but I was trying to stay API-compatible with FF. If you don't pass it in, then having the param at all is pointless and it can be removed altogether.
Comment 3 Philip Chee 2012-01-11 10:49:47 PST
Well minor point: makes it easier for Firefox extension authors to port their extensions to SeaMonkey if the signature of nsContextMenu() is the same.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2012-01-11 13:28:47 PST
(In reply to Philip Chee from comment #3)
> Well minor point: makes it easier for Firefox extension authors to port
> their extensions to SeaMonkey if the signature of nsContextMenu() is the
> same.

Right, but if we don't pass in the corresponding parameters it might actually break them (if they rely on it).

Can't we just pass in getBrowser() or something at the point where it breaks?
Comment 5 Philip Chee 2012-01-13 11:25:06 PST
Well the only place the Firefox nsContextMenu uses gBrowser is to do this:

    this.browser = aBrowser

We on the other hand figure it out ourselves:

    var win = this.target.ownerDocument.defaultView;
    if (win) {
      var webNav = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                      .getInterface(Components.interfaces.nsIWebNavigation);
      this.browser = webNav.QueryInterface(Components.interfaces.nsIDocShell)
                           .chromeEventHandler;

So in my opinion, the least hacky solution is to modify:

    <menupopup id="contentAreaContextMenu"
               pagemenu="start"
               onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu(this, gBrowser, event.shiftKey); return gContextMenu.shouldDisplay;"
               onpopuphiding="if (event.target == this) gContextMenu = null;">

To just pass in null:

if (event.target != this) return true; gContextMenu = new nsContextMenu(this, null, event.shiftKey); return gContextMenu.shouldDisplay;

Or if you insist:

if (event.target != this) return true; gContextMenu = new nsContextMenu(this, window.gBrowser ? gBrowser : null, event.shiftKey); return gContextMenu.shouldDisplay;
Comment 6 Jens Hatlak (:InvisibleSmiley) 2012-01-13 11:45:00 PST
(In reply to Philip Chee from comment #5)
> Well the only place the Firefox nsContextMenu uses gBrowser is to do this:
> (...)

I know, I ported that code.

> To just pass in null:

I think I already wrote what I think about this approach.

> Or if you insist:
> 
> if (event.target != this) return true; gContextMenu = new
> nsContextMenu(this, window.gBrowser ? gBrowser : null, event.shiftKey);
> return gContextMenu.shouldDisplay;

This I could live with. But in the end it's Neil that you have to get this past.

Until now I didn't know what the Plain Text Editor is. Would have been nice to have that in the initial comment (proper, complete STR). [For those who like me never use Debug & QA: It's accessible through an entry in the Composer's Debug menu with Debug & QA installed.] Now that I know, I guess there is no way to have/create a gBrowser object in that context, so your "if you insist" approach would allow us to keep the current behavior for normal contexts while unbreaking the special case.
Comment 7 Philip Chee 2012-01-14 11:12:50 PST
> This I could live with. But in the end it's Neil that you have to get this past.
Why don't you CC Neil or ping him on IRC?
Comment 8 Jens Hatlak (:InvisibleSmiley) 2012-01-14 18:10:50 PST
Created attachment 588698 [details] [diff] [review]
patch

(In reply to Philip Chee from comment #7)
> > This I could live with. But in the end it's Neil that you have to get this past.
> Why don't you CC Neil or ping him on IRC?

No, that's all just wasting time. I thought since you provided the proposals you would just go ahead. Since you don't, I turned your approach into a patch and am requesting review now. Please note that until now I wasn't the assignee of this bug, so *anyone* could have done what I did now. Think the reporter (Ian), a frequent commenter (you), and, yes, also me (the one who broke it). The sooner this bug is fixed, the sooner I can stop thinking about the whole pointless back and forth that happened here...
Comment 9 neil@parkwaycc.co.uk 2012-01-15 04:52:34 PST
It actually broke the context menu in the HTML editor too, but you don't notice until you right-click on a page with an HTML5 context menu in it.

Mail and newsgroups is another kettle of fish - do we want HTML5 context menus running in RSS preview pages?
Comment 10 neil@parkwaycc.co.uk 2012-01-15 04:58:01 PST
(In reply to Jens Hatlak from comment #2)
> (In reply to Philip Chee from comment #1)
> > > +               onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu(this, gBrowser, event.shiftKey); return gContextMenu.shouldDisplay;"
> > Since we never use gBrowser we can just do:
> > ... new nsContextMenu(this, null, event.shiftKey); ...
> I know we don't, but I was trying to stay API-compatible with FF. If you
> don't pass it in, then having the param at all is pointless and it can be
> removed altogether.
Well, it's better than passing in a gBrowser we never use...
Comment 11 Jens Hatlak (:InvisibleSmiley) 2012-01-15 05:07:05 PST
(In reply to neil@parkwaycc.co.uk from comment #9)
> It actually broke the context menu in the HTML editor too, but you don't
> notice until you right-click on a page with an HTML5 context menu in it.

OK...

> Mail and newsgroups is another kettle of fish - do we want HTML5 context
> menus running in RSS preview pages?

Please read bug 702019 comment 4.

(In reply to neil@parkwaycc.co.uk from comment #10)
> Well, it's better than passing in a gBrowser we never use...

OK guys, since everyone seems to think this should not be done the way I think, I'll let someone who actually cares take over and fix this any way you like. Good luck.
Comment 12 neil@parkwaycc.co.uk 2012-01-15 08:51:51 PST
(In reply to Jens Hatlak from comment #11)
> (In reply to comment #9)
> > Mail and newsgroups is another kettle of fish - do we want HTML5 context
> > menus running in RSS preview pages?
> Please read bug 702019 comment 4.
Looks like the pagemenu check doesn't actually help :-( Anyway, MailNews bypasses the code, so it makes no difference there.
Comment 13 neil@parkwaycc.co.uk 2012-01-15 09:02:06 PST
Created attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

The nsContextMenu change fixes editor by disabling the context menu when script is disabled in the document. (This also means that you don't get useless HTML5 context menus when you have disabled script, possibly via an add-on.)
Comment 14 Philip Chee 2012-01-15 10:54:01 PST
> No, that's all just wasting time. I thought since you provided the proposals you would
> just go ahead.
Sorry, I would have but my self builds have been crashing on startup lately so I've been unable to test any patches ;(
Comment 15 Ian Neal 2012-01-20 13:49:50 PST
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

This will be needed for SM2.8 too
Comment 16 Ian Neal 2012-01-20 13:50:37 PST
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

and SM2.7
Comment 17 neil@parkwaycc.co.uk 2012-01-20 15:57:00 PST
Pushed changeset 7ee4b898ee41 to comm-central.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2012-01-23 15:32:38 PST
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

http://hg.mozilla.org/releases/comm-aurora/rev/ed09e1575902
http://hg.mozilla.org/releases/comm-beta/rev/91d4005d9780

Note You need to log in before you can comment on or make changes to this bug.