The default bug view has changed. See this FAQ.

Plain text editor context menu broken since landing of fix to bug 702019

RESOLVED FIXED in seamonkey2.9

Status

SeaMonkey
Composer
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ian Neal, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
seamonkey2.9
regression

SeaMonkey Tracking Flags

(seamonkey2.6 wontfix, seamonkey2.7 fixed, seamonkey2.8 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

5 years ago
> +               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); ...
status-seamonkey2.6: --- → wontfix
status-seamonkey2.7: --- → affected
status-seamonkey2.8: --- → affected
tracking-seamonkey2.7: --- → ?
(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

5 years ago
Well minor point: makes it easier for Firefox extension authors to port their extensions to SeaMonkey if the signature of nsContextMenu() is the same.
(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

5 years ago
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;
(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

5 years ago
> 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?
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...
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #588698 - Flags: review?(neil)
(Assignee)

Comment 9

5 years ago
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?
(Assignee)

Comment 10

5 years ago
(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...
(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.
Assignee: jh → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
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.)
Assignee: nobody → neil
Attachment #588698 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #588698 - Flags: review?(neil)
Attachment #588743 - Flags: review?(iann_bugzilla)

Comment 14

5 years ago
> 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 ;(
(Reporter)

Updated

5 years ago
Attachment #588743 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Comment 15

5 years ago
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

This will be needed for SM2.8 too
Attachment #588743 - Flags: approval-comm-aurora+
(Reporter)

Comment 16

5 years ago
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

and SM2.7
Attachment #588743 - Flags: approval-comm-beta+
(Assignee)

Comment 17

5 years ago
Pushed changeset 7ee4b898ee41 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.9
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
Attachment #588743 - Attachment description: Proposed patch → Proposed patch [Checkin: Comments 17 and 18]
status-seamonkey2.7: affected → fixed
status-seamonkey2.8: affected → fixed
tracking-seamonkey2.7: ? → ---
You need to log in before you can comment on or make changes to this bug.