Closed Bug 657161 Opened 8 years ago Closed 8 years ago

Make use of contentAreaContext in Composer

Categories

(SeaMonkey :: Composer, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.5

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Composer context menu patch v1.0 (obsolete) — Splinter Review
Currently, even though we overlay editor.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:
* Adapts EditorContextMenuOverlay.xul to overlay contentAreaContextOverlay.xul
* Rewrites EditorContextMenu.js to extend nsContextMenu for Composer window.
* Removes editorContentContext and editorSourceContext context menus.
* Adds IsInPreviewMode() function.
* Adds unique entities for cut, copy and paste toolbarbuttons.
* Remove unused entities from editorOverlay.dtd as they're now provided via contentAreaContextOverlay.
Attachment #532464 - Flags: review?(neil)
Comment on attachment 532464 [details] [diff] [review]
Composer context menu patch v1.0

>+      window.updateCommands("mode_switch");
Why do we need this? I can't work out what (if anything) it's replacing...

>+  this.showItem("menu_pasteNoFormatting_cm", aShow);
[Hmm, both editor and compose have paste without formatting? Might be worth a followup to move it to contentAreaContextOverlay. Possibly enabling it for all editable regions. Very possibly including paste as quotation, because otherwise the compose overlay will get lonely!]

>   var objectName = InitObjectPropertiesMenuitem("objectProperties_cm");
...
>   InitRemoveStylesMenuitems("removeStylesMenuitem_cm", "removeLinksMenuitem_cm", "removeNamedAnchorsMenuitem_cm");
...
>   InitJoinCellMenuitem("joinTableCells_cm");
Might be possible to avoid these if we're not showing them?

>+  var inCell = IsInTableCell() && aShow;
Nit: aShow && IsInTableCell()
Blocks: 657234
(In reply to comment #1)
>(From update of attachment 532464 [details] [diff] [review])
>>+      window.updateCommands("mode_switch");
>Why do we need this? I can't work out what (if anything) it's replacing...
From IRC, it's to fix a regression from bug 451960. But it's the wrong fix.
Attachment #532464 - Flags: review?(neil)
Attached patch Composer context menu patch v1.1 (obsolete) — Splinter Review
Changes since v1.0:
* Moved code only needed when aShow is true into its own if statement.
* Adjusted InitObjectPropertiesMenuitem function so that img special case work does not have to be duplicated.
* Fixed objectProperties regression, InitObjectPropertiesMenuitem now manipulates cmd_objectProperties.
* Separators are hidden/shown as required.
Attachment #532464 - Attachment is obsolete: true
Attachment #532537 - Flags: review?(neil)
Comment on attachment 532537 [details] [diff] [review]
Composer context menu patch v1.1

Just a skim; haven't tried this out yet.

>+  // Show "Create Link" if not in a link and not in source mode or sidebar.
>+  this.showItem("createLink_cm", !isInLink && aShow);
>+
>+  // Show "Edit link in new Composer" if in a link and
>+  // not in source mode or sidebar.
>+  this.showItem("editLink_cm", isInLink && aShow);
Nit: I'd probably write aShow first on these too for consistency.

>   // Update enable states for all table commands
>   goUpdateTableMenuItems(document.getElementById("composerTableMenuItems"));
[I wonder whether we can optimise this at all.]

>+  <menuitem id="objectProperties"
>+            command="cmd_objectProperties"/>
Won't this fit on one line? ;-)

Thunderbird has a fork of editorOverlay.xul, will these changes break it? In particular, I think they might need this specific change ported; I believe they deleted the other buttons from their copy.
Attachment #532537 - Flags: review?(neil)
Changes since v1.1:
* Changed position of aShow in expression for showItem on editLink_cm and createLink_cm.
* Moved goUpdateTableMenuItems into aShow if statement.
* Corrected objectProperties menuitem to be on a single line.
* Ported objectProperties change in editor's editorOverlay to TB's editorOverlay
Attachment #532537 - Attachment is obsolete: true
Attachment #532926 - Flags: review?(neil)
Attachment #532926 - Flags: review?(neil) → review+
Comment on attachment 532926 [details] [diff] [review]
Composer context menu patch v1.2 [Checked in: Comment 8]

Requesting review for changes that impact on TB.
Attachment #532926 - Flags: review?(mbanner)
Blocks: 660739
Blocks: 666259
Comment on attachment 532926 [details] [diff] [review]
Composer context menu patch v1.2 [Checked in: Comment 8]

Whoever can review the TB related bits first:
editor/ui/composer/content/editor.js
editor/ui/composer/content/editorUtilities.js
editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
mail/components/compose/content/editorOverlay.xul
I'd like to try and get this into comm-aurora before the merge.
Attachment #532926 - Flags: review?(dbienvenu)
Attachment #532926 - Flags: review?(mbanner)
Attachment #532926 - Flags: review?(dbienvenu)
Attachment #532926 - Flags: review+
Comment on attachment 532926 [details] [diff] [review]
Composer context menu patch v1.2 [Checked in: Comment 8]

http://hg.mozilla.org/comm-central/rev/04c68787486d
Only difference between this patch and what was checked in was a similar workaround as to in Bug 674246 for the issue in bug 672258
Attachment #532926 - Attachment description: Composer context menu patch v1.2 → Composer context menu patch v1.2 [Checked in: Comment 8]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.5
You need to log in before you can comment on or make changes to this bug.