Make use of contentAreaContext in message compose window

RESOLVED FIXED in seamonkey2.2

Status

enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Tracking

(Blocks 1 bug)

Trunk
seamonkey2.2
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

8 years ago
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)
I notice that the mailContext has a hybrid approach - it defines its own menuitems but then calls on nsContextMenu to set things up.
Assignee

Comment 2

8 years ago
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)
Assignee

Updated

8 years ago
Blocks: 650885
Assignee

Comment 3

8 years ago
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 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 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 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 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;
Assignee

Updated

8 years ago
Attachment #530875 - Flags: review?(neil)
Assignee

Updated

8 years ago
Attachment #530875 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #531470 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #530935 - Flags: review?(neil)
Assignee

Comment 8

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

8 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.
(In reply to comment #10)
> Error: window.getBrowser is not a function
Oh dear. Regression from bug 562339 :-(
Assignee

Updated

8 years ago
Depends on: 656886
Assignee

Updated

8 years ago
Attachment #532052 - Flags: review?(neil)
Assignee

Comment 12

8 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 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 15

8 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]
Assignee

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.2a1
Assignee

Updated

8 years ago
Blocks: 657234

Updated

8 years ago
Depends on: 660739
You need to log in before you can comment on or make changes to this bug.