Closed
Bug 655529
Opened 14 years ago
Closed 14 years ago
Make use of contentAreaContext in message compose window
Categories
(SeaMonkey :: MailNews: Composition, enhancement)
SeaMonkey
MailNews: Composition
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.2
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
Use contentAreaContext for messenger compose window alternative patch v1.2a [Checked in: Comment 15]
12.17 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
Currently, even though we overlay messengercompose.xul with contentAreaContextOverlay.xul we don't actually make use of the context menu from it. If we did, we could remove some code duplication.
This patch:
* Adds a getBrowser function to the compose window.
* Removes unneeded openEditorContextMenu function.
* Adds event listener for popupshowing so that updateEditItems is still called.
* Removes msgComposeContext context menu.
* Adds overlay that gives two missing context items for context menu.
Attachment #530875 -
Flags: review?(neil)
Comment 1•14 years ago
|
||
I notice that the mailContext has a hybrid approach - it defines its own menuitems but then calls on nsContextMenu to set things up.
This alternative patch:
* Replaces the onpopupshowing on contentAreaContextMenu, this means a getBrowser function is not needed nor is adding an event listener.
Attachment #530935 -
Flags: review?(neil)
This patch:
* Does the context menu the same way as mailContext does in mailWindowOverlay, but I do think this one adds as well as taking away some duplication of code.
Attachment #531470 -
Flags: review?(neil)
Comment 4•14 years ago
|
||
Comment on attachment 530935 [details] [diff] [review]
Use contentAreaContext for messenger compose window alternative patch v1.0
>+ gContextMenu =
>+ new nsContextMenu(popup, document.getElementById("content-frame"));
> updateEditItems();
[As it happens, nsContextMenu already updates the standard edit items.]
>+ <popupset id="contentAreaContextSet">
Nit: don't need this, because you know the id of the popup.
>+ <menupopup id="contentAreaContextMenu"
>+ onpopupshowing="return event.target != this ||
>+ openEditorContextMenu(this);">
>+ <menuitem id="context-pasteNoFormatting"
>+ insertafter="context-paste"
>+ command="cmd_pasteNoFormatting"/>
>+ <menuitem insertafter="context-pasteNoFormatting"
>+ label="&pasteQuote.label;"
>+ accesskey="&pasteQuote.accesskey;"
>+ command="cmd_pasteQuote"/>
This almost works, but the sidebar uses the same popup, so the menuitems appear (permanently) on the sidebar content context menu.
Comment 5•14 years ago
|
||
Comment on attachment 530875 [details] [diff] [review]
Use contentAreaContext for messenger compose window patch v1.0
Actually I can't decide between either of these patches. I guess it depends on which way turns out to be easier to hide the extra menuitems in the sidebar.
Comment 6•14 years ago
|
||
Comment on attachment 531470 [details] [diff] [review]
Do it the same way as mailContext alternative patch v1.0
I see what you mean about the duplication, because we already have a context menu in this window, which means that we don't get the right overlay love.
Attachment #531470 -
Flags: review?(neil)
Comment 7•14 years ago
|
||
Comment on attachment 530935 [details] [diff] [review]
Use contentAreaContext for messenger compose window alternative patch v1.0
>+ gContextMenu =
>+ new nsContextMenu(popup, document.getElementById("content-frame"));
> updateEditItems();
>+ return gContextMenu.shouldDisplay;
This version has the advantage that you could write something like this:
gContextMenu =
new nsContextMenu(popup, document.getElementById("content-frame"));
if (gContextMenu.shouldDisplay) {
// handle extra paste items here
}
return gContextMenu.shouldDisplay;
Attachment #530875 -
Flags: review?(neil)
Attachment #530875 -
Attachment is obsolete: true
Attachment #531470 -
Attachment is obsolete: true
Attachment #530935 -
Flags: review?(neil)
Changes since v1.0:
* Hide extra menuitems as default and unhide with JS in onpopupshowing.
* Remove unneeded <popupset> tags in overlay.
* Only update extra menuitems as rest are done by nsContextMenu code.
Attachment #530935 -
Attachment is obsolete: true
Attachment #532052 -
Flags: review?(neil)
Comment 9•14 years ago
|
||
Comment on attachment 532052 [details] [diff] [review]
Use contentAreaContext for messenger compose window alternative patch v1.1
[I don't think this is going to work, because the sidebar also uses context="contentAreaContextMenu" which will call openEditorContextMenu.]
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9)
> Comment on attachment 532052 [details] [diff] [review] [review]
> Use contentAreaContext for messenger compose window alternative patch v1.1
>
> [I don't think this is going to work, because the sidebar also uses
> context="contentAreaContextMenu" which will call openEditorContextMenu.]
Okay a custom sidebar tab (added using http://edmullen.net/mozilla/sidebarpreview.php) gives a:
Error: window.getBrowser is not a function
Source file: chrome://navigator/content/navigator.xul
Line: 1
which suggests to me that, probably due to the load order and how overlays work (at the moment), it is not getting overlaid by the new file.
Comment 11•14 years ago
|
||
(In reply to comment #10)
> Error: window.getBrowser is not a function
Oh dear. Regression from bug 562339 :-(
Attachment #532052 -
Flags: review?(neil)
Assignee | ||
Comment 12•14 years ago
|
||
Changes since v1.1:
* Use WhichElementHasFocus() to determine if on message body.
* Take into account fix for non-browser sidebar issue (bug 656886).
Attachment #532052 -
Attachment is obsolete: true
Attachment #532199 -
Flags: review?(neil)
Comment 13•14 years ago
|
||
Comment on attachment 532199 [details] [diff] [review]
Use contentAreaContext for messenger compose window alternative patch v1.2
>+ var focusedElement = WhichElementHasFocus();
Nit: could optimise this, since you're only interested in == content
>+ var showPasteExtra = focusedElement && focusedElement == content;
Nit: superfluous extra test for focusedElement &&
Attachment #532199 -
Flags: review?(neil) → review+
Assignee | ||
Comment 14•14 years ago
|
||
For checkin
Attachment #532199 -
Attachment is obsolete: true
Attachment #532331 -
Flags: review+
Assignee | ||
Comment 15•14 years ago
|
||
Comment on attachment 532331 [details] [diff] [review]
Use contentAreaContext for messenger compose window alternative patch v1.2a [Checked in: Comment 15]
http://hg.mozilla.org/comm-central/rev/ac811b06e1c2
Attachment #532331 -
Attachment description: Use contentAreaContext for messenger compose window alternative patch v1.2a → Use contentAreaContext for messenger compose window alternative patch v1.2a [Checked in: Comment 15]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•