Closed
Bug 717111
Opened 12 years ago
Closed 12 years ago
Plain text editor context menu broken since landing of fix to bug 702019
Categories
(SeaMonkey :: Composer, defect)
SeaMonkey
Composer
Tracking
(seamonkey2.6 wontfix, seamonkey2.7 fixed, seamonkey2.8 fixed)
RESOLVED
FIXED
seamonkey2.9
People
(Reporter: iannbugzilla, Assigned: neil)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.17 KB,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-comm-aurora+
iannbugzilla
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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•12 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); ...
Updated•12 years ago
|
status-seamonkey2.6:
--- → wontfix
status-seamonkey2.7:
--- → affected
status-seamonkey2.8:
--- → affected
tracking-seamonkey2.7:
--- → ?
Comment 2•12 years ago
|
||
(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•12 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.
Comment 4•12 years ago
|
||
(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•12 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;
Comment 6•12 years ago
|
||
(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•12 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?
Comment 8•12 years ago
|
||
(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 | ||
Comment 9•12 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•12 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...
Comment 11•12 years ago
|
||
(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•12 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•12 years ago
|
||
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•12 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 ;(
Attachment #588743 -
Flags: review?(iann_bugzilla) → review+
Reporter | ||
Comment 15•12 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•12 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•12 years ago
|
||
Pushed changeset 7ee4b898ee41 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → seamonkey2.9
Comment 18•12 years ago
|
||
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]
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•