Closed Bug 717111 Opened 9 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Composer, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
seamonkey2.9
Tracking Status
seamonkey2.6 --- wontfix
seamonkey2.7 --- fixed
seamonkey2.8 --- fixed

People

(Reporter: iann_bugzilla, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
> +               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); ...
(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 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?
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.
> 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?
Attached patch patch (obsolete) — Splinter Review
(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)
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?
(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
(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.
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)
> 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 ;(
Attachment #588743 - Flags: review?(iann_bugzilla) → review+
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+
Comment on attachment 588743 [details] [diff] [review]
Proposed patch [Checkin: Comments 17 and 18]

and SM2.7
Attachment #588743 - Flags: approval-comm-beta+
Pushed changeset 7ee4b898ee41 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 9 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]
You need to log in before you can comment on or make changes to this bug.