289366 - editorOverlay inserts a second menupopup into message compose view show/hide menu

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[[e-mail]]

If you use DOM inspector to inspect the message compose window and look at the
View menu's Show/Hide menuitem you'll see it has two child menupopus, the real
one provided by messengercompose.xul and a spurious one provided by
editorOverlay.xul; fortunately only the real one displays but as it has no other
consumers the spurious one should be moved to editor.xul where it won't conflict.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

->me.  Note that I have no time, so please steal the bug from me if you can fix
it yourself :)

Comment 2

[[e-mail]]

Callek, as CTho is too busy perhaps you can find time to do this?

Comment 3

[Justin Wood (:Callek)]

Attachment: Port TB's Fix

Port the TB compose window's fix. (Change ID, add now missing label and
accesskey)

Comment 4

[[e-mail]]

Comment on attachment 180346 [details] [diff] [review]
Port TB's Fix

Sorry, this is no good, I need that id to fix the dependent bug. Also, is that
a UTF-8 BOM that crept into line 1?

Comment 5

[Justin Wood (:Callek)]

Attachment: Do not use editorOverlay for view menu Show/Hide

Yes that first line change is a BOM, according to bz anyway these should be ok
in-tree.

Gavin may want to adopt this, or similar, for Composer perhaps. (I can provide
a patch here; against Mozilla CVS if he asks)

Comment 6

[Karsten Düsterloh]

(In reply to comment #5)
> Yes that first line change is a BOM, according to bz anyway these should be ok
> in-tree.

We don't use BOMs anywhere in XUL, so you shouldn't introduce one. Not even in
MailNews. ;-)
Please remove that (on check-in).

Comment 7

[[e-mail]]

Comment on attachment 180449 [details] [diff] [review]
Do not use editorOverlay for view menu Show/Hide

>diff -u -r1.279 messengercompose.xul
Neither of these changes are in fact necessary, because utilityOverlay.xul
contains the label and access key.

>diff -u -r1.155 editor.xul
>+      <menu id="menu_Toolbars" label="&viewToolbarsMenu.label;" accesskey="&viewToolbarsMenu.accesskey;">
Again, the label and accesskey aren't necessary because of utilityOverlay.xul -
in fact you can remove the entities from editorOverlay.dtd

>+          <menuitem id="viewComposerToolbar" label="&compositionToolbarCmd.label;" type="checkbox" accesskey="&compositiontb.accesskey;" observes="cmd_viewCompToolbar"  />
Ideally these should work using command= which is preferred.

r+sr=me with these nits fixed.

Comment 8

[Justin Wood (:Callek)]

Attachment: Final

This addresses Neils Review Nits, porting r=, requesting sr just to assure
myself that I got the nits on editoroverlay.dtd correct.

Also requesting a= now, (since neil did r+sr pending nits), this is a
low-impact change affecting SeaMonkey only.

Comment 9

[Ian Neal]

Comment on attachment 180859 [details] [diff] [review]
Final

>Index: mozilla/editor/ui/composer/content/editor.xul
>===================================================================
>@@ -143,7 +143,13 @@
>     <menu id="viewMenu" label="&viewMenu.label;" accesskey="&viewmenu.accesskey;">
>     <!-- id pulls in "Show Sidebar" item from sidebarOverlay -->
>     <menupopup id="menu_View_Popup">
>-      <menu id="menu_Toolbars"/>
>+      <menu id="menu_Toolbars">
>+        <menupopup id="view_toolbars_popup"> 
>+          <menuitem id="viewComposerToolbar" label="&compositionToolbarCmd.label;" type="checkbox" accesskey="&compositiontb.accesskey;" command="cmd_viewCompToolbar"  />           <menuitem id="viewFormatToolbar"   label="&formattingToolbarCmd.label;"  type="checkbox" accesskey="&formattingtb.accesskey;"  command="cmd_viewFormatToolbar" />
The second menuitem should be on a line of its own.

>Index: mozilla/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
>===================================================================
>@@ -144,16 +144,6 @@
> <!ENTITY editcheckspelling.keybinding "k">
> 
> <!-- View menu items -->
>-<!ENTITY viewToolbarsMenu.label "Show/Hide"> 
>-<!ENTITY viewToolbarsMenu.accesskey "w"> 
>-<!ENTITY compositionToolbarCmd.label "Composition Toolbar">
>-<!ENTITY compositiontb.accesskey "c">
>-<!ENTITY formattingToolbarCmd.label "Format Toolbar">
>-<!ENTITY formattingtb.accesskey "f">
>-<!ENTITY editmodeToolbarCmd.label "Edit Mode Toolbar">
>-<!ENTITY editmodetb.accesskey "E">
>-<!ENTITY taskbarCmd.label "Status Bar">  
>-<!ENTITY taskbarCmd.accesskey "S"> 
> <!ENTITY viewPageSource.label "Page Source">
> <!ENTITY viewpagesource.accesskey "s">
> <!ENTITY viewParagraphMarks.label "Paragraph Marks">

These entities still need to exist somewhere, did you miss out a file when you
were creating your diff? editor.dtd perhaps?

Comment 10

[[e-mail]]

Comment on attachment 180859 [details] [diff] [review]
Final

>-<!ENTITY compositionToolbarCmd.label "Composition Toolbar">
>-<!ENTITY compositiontb.accesskey "c">
>-<!ENTITY formattingToolbarCmd.label "Format Toolbar">
>-<!ENTITY formattingtb.accesskey "f">
>-<!ENTITY editmodeToolbarCmd.label "Edit Mode Toolbar">
>-<!ENTITY editmodetb.accesskey "E">
Preferably what IanN said or at least put these six lines back.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 180859 [details] [diff] [review]
Final

a=asa

Comment 12

[Justin Wood (:Callek)]

Attachment: for checkin

I did miss editor.dtd (fixed it as well to include only the non-duplicate
items) [I had status-bar in there as well].  Porting flags [already has
a1.8b2=Asa I cannot set flag]

Comment 13

[Justin Wood (:Callek)]

Attachment: For real

289366.diff not 289366.files, damn patchmaker confusing me. ;-)

Comment 14

[Robert Kaiser]

checked in as requested by Callek via Standard8 ;-)

Comment 15

[Justin Wood (:Callek)]

For note, the whitespace change that IanN had a nit about, has been fixed and
checked in by CTho.  a=Asa over IRC.