Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition

ASSIGNED
Assigned to

Status

SeaMonkey
Composer
ASSIGNED
8 years ago
2 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

125.75 KB, patch
stefanh
: feedback-
InvisibleSmiley
: feedback+
Details | Diff | Splinter Review
22.35 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
6.50 KB, patch
Ian Neal
: review+
stefanh
: ui-review+
Details | Diff | Splinter Review
19.96 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
680 bytes, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 485506 [details] [diff] [review]
Make toolbars customizable patch v0.1

At the moment the toolbars in the Composer and MailNews Composition windows are not customizable - that should be fixed.
Attachment #485506 - Flags: feedback?(stefanh)
(Assignee)

Comment 1

8 years ago
I did look at putting on an ignore whitespace change patch but less than 1K smaller.
(Assignee)

Updated

8 years ago
Attachment #485506 - Flags: feedback?(jh)

Comment 2

8 years ago
Comment on attachment 485506 [details] [diff] [review]
Make toolbars customizable patch v0.1

I have not looked at the code (except when I tracked the console warning), so this is more of a ui-behaviour feedback. Overall, it works fine - nice job :-)

But there are a few things that we need to settle:

1: The format toolbar buttons have no label text (and we don't want them to have label text). The icons also only comes in one size (anything else would of course be strange) so the choices for "Settings for this toolbar" doesn't make sense here.

2: When switching between modes (normal, html tags, html source,  preview), your choice of showing/hiding the format toolbar doesn't persist (try for example to hide the format toolbar in "normal" mode, then switch to "html tags" and back). I'm also not clear how we should handle the format toolbar in the different modes - but we should probably treat all modes as they have the same toolbar ("persisting through modes")? Need then to consider the fact that the source mode view then would have a disabled format toolbar... :-/

3: Showing/hiding a toolbar by selecting/deselecting in the context menu doesn't update the menuitem in the View menu. Doing it from the View Menu updates the context menuitems, though

4: When you have the customize sheet in front of you, it's possible to type in the editable area - but you knew that already since we talked about it :-)

5:
-  gEditorToolbarPrefListener = new nsPrefListener(kEditorToolbarPrefs);

EditorShutdown() does gEditorToolbarPrefListener.shutdown(), so I get a warning in the error console when closing the editor window:
----------------------------------------------
Error: gEditorToolbarPrefListener is undefined
Source File: chrome://editor/content/editor.js
Line: 673
----------------------------------------------

Minusing since I think we need to settle the behaviour in 1-4 first.
Attachment #485506 - Flags: feedback?(stefanh) → feedback-
Comment on attachment 485506 [details] [diff] [review]
Make toolbars customizable patch v0.1

Sorry for the delay, first had to get a clean tree...

(In reply to comment #0)
> At the moment the toolbars in the Composer and MailNews Composition windows are
> not customizable

AFAICS MailNews compose windows are already partly customizable, "only" the ability to customize the Formatting Toolbar was added. Care to update the bug summary (which will probably also help the check-in comment)? Just asking, maybe I'm wrong. ;-)

It seems Stefan already did a good job at checking the behavior (much better than what I would have come up with, in fact). Since I'm generally happy with it I give a f+ if you address the View menu issue, but if in doubt, take Stefan's feedback (check-in comment etc.).

One more To Do for you (just FTR, I'm sure you'd have taken care of that eventually): Help needs to be adapted, either here or in a follow-up.

(In reply to comment #2)
> 1: The format toolbar buttons have no label text (and we don't want them to
> have label text).

We don't? Why? If the user decides to go with text labels, shouldn't that s/he be able to achieve that? Or am I getting you wrong?

> The icons also only comes in one size (anything else would of
> course be strange) so the choices for "Settings for this toolbar" doesn't make
> sense here.

I wonder whether any choice we make here affects all themes (i.e. whether there's only one switch, so if we disabled it for Classic and Modern, would third-party themes be able to provide bigger icons?). More generally speaking, as long as selecting another option doesn't completely break the display, I'd be OK if we allowed selecting a size that we don't actually provide icons for in the bundled themes.

> 2: When switching between modes (normal, html tags, html source,  preview),
> your choice of showing/hiding the format toolbar doesn't persist

True.

> we should probably treat all modes as they have the same
> toolbar ("persisting through modes")? Need then to consider the fact that the
> source mode view then would have a disabled format toolbar... :-/

Actually I'd support that idea. I see that the content is needlessly jumping around because of the showing and hiding when switching modes. From my POV it's not a blocker, though, i.e. you could do that in a follow-up or even leave it like it is.

> 3: Showing/hiding a toolbar by selecting/deselecting in the context menu
> doesn't update the menuitem in the View menu. Doing it from the View Menu
> updates the context menuitems, though

Good catch. This indeed needs to be fixed.

> 4: When you have the customize sheet in front of you, it's possible to type in
> the editable area

Hmm, isn't that actually a feature? ;-) No seriously, does this have any negative implications?

Note: I skimmed through the patch, but didn't even attempt to do a code review. Ratty might be a good candidate.

Thanks for working on this!
Attachment #485506 - Flags: feedback?(jh) → feedback+

Comment 4

8 years ago
(In reply to comment #3)
> > 1: The format toolbar buttons have no label text (and we don't want them to
> > have label text).
> 
> We don't? Why? If the user decides to go with text labels, shouldn't that s/he
> be able to achieve that? Or am I getting you wrong?

I don't think the text will fit into the current toolbar... That is, we don't have any text labels and we (imo) don't want any.

> 
> > The icons also only comes in one size (anything else would of
> > course be strange) so the choices for "Settings for this toolbar" doesn't make
> > sense here.
> 
> I wonder whether any choice we make here affects all themes (i.e. whether
> there's only one switch, so if we disabled it for Classic and Modern, would
> third-party themes be able to provide bigger icons?). More generally speaking,
> as long as selecting another option doesn't completely break the display, I'd
> be OK if we allowed selecting a size that we don't actually provide icons for
> in the bundled themes.

Yes, but why would you want to give users an option that doesn't have any effect?


> > 4: When you have the customize sheet in front of you, it's possible to type in
> > the editable area
> 
> Hmm, isn't that actually a feature? ;-) No seriously, does this have any
> negative implications?

It's a bit weird that you can type behind a pane since when you're customizing all menus are disabled - you don't really want a user do do anything but customizing (and we don't do that in mailNews iirc).

Updated

8 years ago
Duplicate of this bug: 604721
So, where are we regarding this? I think we should have it in a beta, so requesting status2.1.

As for "Settings for this toolbar", I think we could just leave that one for now as it doesn't break anything if we have/keep it. If someone really wants the labels (with or without icons) despite the waste of space (think users with visual disabilities), I think they should be able to activate it. And bigger icons, if the theme provides them (again I don't think it matters that we don't have some in the default themes!).

As for the ability to type while customizing: I'll leave that to Stefan and Ian; I would think that such nits could go to a follow-up, but then that's just my opinion.
status-seamonkey2.1: --- → ?
(Assignee)

Updated

8 years ago
Summary: Allow customization of toolbar in Composer and MailNews Composition → Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition
(Assignee)

Updated

7 years ago
Depends on: 635792
(Assignee)

Comment 7

7 years ago
Created attachment 514376 [details] [diff] [review]
Tidy up some broadcasters and commandsets

I'm going to be trying to split the patches into easily reviewable patches, so this is the first one.

This patch:
* Moves some common commandset overlays into editorOverlay.
* Moves a common broadcaster into editorOverlay.
* Moves a singular use broadcaster out of editorOverlay into editor.
* Renames the Editor/Compose toolbars to have a common name so that code can be shared and moved into editorOverlay.
Attachment #514376 - Flags: review?(neil)

Comment 8

7 years ago
Comment on attachment 514376 [details] [diff] [review]
Tidy up some broadcasters and commandsets

I'm not sure that changing the toolbar's id is a good idea. The rest looks OK.
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> Comment on attachment 514376 [details] [diff] [review]
> Tidy up some broadcasters and commandsets
> 
> I'm not sure that changing the toolbar's id is a good idea. The rest looks OK.

To effectively share code those ids need to be the same across the 3 windows (editor, text editor and message compose), unless you know another way of doing it.
(Assignee)

Updated

7 years ago
Attachment #514376 - Flags: review?(neil)
(Assignee)

Comment 10

7 years ago
Created attachment 514671 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2

Changes since last version:
* No renaming of toolbar ids and keeping separate <command>s for editor vs message compose toggling.
Attachment #514376 - Attachment is obsolete: true
Attachment #514671 - Flags: review?(neil)
Comment on attachment 514671 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2

>+  <broadcasterset id="editorBroadcasters">
>     <broadcaster id="Communicator:WorkMode"/>
>+    <broadcaster id="args" value="about:blank"/>
>+
>+    <!-- view menu -->
>+    <command id="cmd_viewEditModeToolbar"
>+             oncommand="goToggleToolbar('EditModeToolbar','cmd_viewEditModeToolbar');"
>+             checked="true"/>
[Huh, I wonder why this command lives in a broadcasterset...]

>+  <commandset id="tasksCommands">
>+    <commandset id="globalEditMenuItems"/>
>+    <commandset id="selectEditMenuItems"/>
>+    <commandset id="undoEditMenuItems"/>
>+    <commandset id="clipboardEditMenuItems"/>
>+    <command id="toggleSidebar"/>
>+  </commandset>
I think it would be neater to overlay editorCommands instead.

> <!ENTITY % editorDTD SYSTEM "chrome://editor/locale/editor.dtd">
> %editorDTD;
>+<!ENTITY % editorOverlayDTD SYSTEM "chrome://editor/locale/editorOverlay.dtd">
>+%editorOverlayDTD;
IMHO it would be better to move the string into editor.dtd
(given that the overlay doesn't use the string...)

>-<broadcasterset id="composeBroadcasters">
>+<broadcasterset id="editorBroadcasters">
This change doesn't look right, since it's in compose.xul, not editor.xul - unless you can explain why, of course...
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> Comment on attachment 514671 [details] [diff] [review]
> Tidy up some broadcasters and commandsets v2
> 
> >+  <broadcasterset id="editorBroadcasters">
> >     <broadcaster id="Communicator:WorkMode"/>
> >+    <broadcaster id="args" value="about:blank"/>
> >+
> >+    <!-- view menu -->
> >+    <command id="cmd_viewEditModeToolbar"
> >+             oncommand="goToggleToolbar('EditModeToolbar','cmd_viewEditModeToolbar');"
> >+             checked="true"/>
> [Huh, I wonder why this command lives in a broadcasterset...]
There are a whole load that live in a broadcasterset in editorOverlay.xul

> >-<broadcasterset id="composeBroadcasters">
> >+<broadcasterset id="editorBroadcasters">
> This change doesn't look right, since it's in compose.xul, not editor.xul -
> unless you can explain why, of course...
Well editorOverlay.xul is providing an overlay for the broadcasterset, I could have left them with an id of "broadcasterset" I suppose...
(Assignee)

Updated

7 years ago
Attachment #514671 - Flags: review?(neil)
(Assignee)

Comment 13

7 years ago
Created attachment 515238 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.1

Changes since v2:
* Moved commands from broadcasterset to commandset.
* Overlay using editorCommands rather than tasksCommands.
* Moved entity from editorOverlay.dtd to editor.dtd.
* Changed id for broadcastersets.
Attachment #514671 - Attachment is obsolete: true
Attachment #515238 - Flags: review?(neil)
(Assignee)

Comment 14

7 years ago
One change I missed but I have fixed locally, in messengercompose.xul changed broadcaster_throbber to Editor:Throbber in the navigator-throbber button observes element.
(Assignee)

Comment 15

7 years ago
Created attachment 515405 [details] [diff] [review]
toolbar item tidy up

This patch:
* Turns menulist and stack elements in toolbar's children into toolbaritems
* Adds missing classes to toolbaritems/buttons.
* Minor tidy ups.
Attachment #515405 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #515405 - Flags: review?(neil)
(Assignee)

Comment 16

7 years ago
Created attachment 515407 [details] [diff] [review]
toolbar item tidy up v1.1

Changes since v1.0:
* Also included changes made to editorSmileyOverlay.xul
Attachment #515405 - Attachment is obsolete: true
Attachment #515407 - Flags: review?(neil)
Comment on attachment 515407 [details] [diff] [review]
toolbar item tidy up v1.1

>-function EditorSelectFontSize()
I don't think it's safe to change this because it's shared code (the menulist itself is OK because Thunderbird have forked editorOverlay.xul).
[There's actually a better way of doing this by setting the value attribute on each of the menuitems so that you can simply read the menulist value.]

>+  <toolbarbutton id="AlignPopupButton"
>+                 class="toolbarbutton-1"
Why the class?

>+  <toolbaritem id="paragraph-select-container"
>+               class="chromeclass-toolbar-additional"
>+               tooltiptext="&ParagraphSelect.tooltip;"
>+               observes="cmd_renderedHTMLEnabler">
>+    <menulist id="ParagraphSelect"
>+              class="toolbar-focustarget"
>+              crop="right"
>+              observes="paragraph-select-container">
Not sure this will work. (Not even sure what it's trying to do.)

>   </menulist>
>+  </toolbaritem>
I assume the indentation will be sorted out at some point?
(Assignee)

Comment 18

7 years ago
(In reply to comment #17)
> Comment on attachment 515407 [details] [diff] [review]
> toolbar item tidy up v1.1
> 
> >+  <toolbarbutton id="AlignPopupButton"
> >+                 class="toolbarbutton-1"
> Why the class?
I have another patch coming up that switches the style sheet from using the toolbar id to using the toolbarbutton/item class.
> 
> >+  <toolbaritem id="paragraph-select-container"
> >+               class="chromeclass-toolbar-additional"
> >+               tooltiptext="&ParagraphSelect.tooltip;"
> >+               observes="cmd_renderedHTMLEnabler">
> >+    <menulist id="ParagraphSelect"
> >+              class="toolbar-focustarget"
> >+              crop="right"
> >+              observes="paragraph-select-container">
> Not sure this will work. (Not even sure what it's trying to do.)
We use it elsewhere for keeping the disable attribute in sync.
> 
> >   </menulist>
> >+  </toolbaritem>
> I assume the indentation will be sorted out at some point?
I could, but I was trying to avoid a lot of whitespace change and taking blame of massive sections of code.

Comment 19

7 years ago
(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 515407 [details] [diff] [review]
> > toolbar item tidy up v1.1
> > 
> > >+  <toolbarbutton id="AlignPopupButton"
> > >+                 class="toolbarbutton-1"
> > Why the class?
> I have another patch coming up that switches the style sheet from using the
> toolbar id to using the toolbarbutton/item class.

It will look the same, right?
(Assignee)

Comment 20

7 years ago
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Comment on attachment 515407 [details] [diff] [review]
> > > toolbar item tidy up v1.1
> > > 
> > > >+  <toolbarbutton id="AlignPopupButton"
> > > >+                 class="toolbarbutton-1"
> > > Why the class?
> > I have another patch coming up that switches the style sheet from using the
> > toolbar id to using the toolbarbutton/item class.
> 
> It will look the same, right?

That is the intention, yes.
(Assignee)

Updated

7 years ago
Attachment #515407 - Flags: review?(neil)
(Assignee)

Comment 21

7 years ago
Created attachment 515412 [details] [diff] [review]
toolbar item tidy up v1.2

Changes since v1.1:
* Removed FontSize changes - will spin those off to another bug.
* Changed class for AlignPopup, InsertPopup and absolutePostion buttons.
Attachment #515407 - Attachment is obsolete: true
Attachment #515412 - Flags: review?(neil)
(Assignee)

Comment 22

7 years ago
Created attachment 515415 [details] [diff] [review]
Switch from toolbar id to class patch v1.0

This patch (when applied on top of the toolbar item tidy up patch):
* Switched the style sheets to use class rather than toolbar id to determine style.
Attachment #515415 - Flags: ui-review?(stefanh)
Attachment #515415 - Flags: review?(neil)
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 515407 [details] [diff] [review])
> > >+  <toolbarbutton id="AlignPopupButton"
> > >+                 class="toolbarbutton-1"
> > Why the class?
> I have another patch coming up that switches the style sheet from using the
> toolbar id to using the toolbarbutton/item class.
OK, but it was a really bad choice of class. (The latest patch is still a bad choice of class, since it's intended for use with chrome flags. So you should really think up a new class name.)

> > >+  <toolbaritem id="paragraph-select-container"
> > >+               class="chromeclass-toolbar-additional"
> > >+               tooltiptext="&ParagraphSelect.tooltip;"
> > >+               observes="cmd_renderedHTMLEnabler">
> > >+    <menulist id="ParagraphSelect"
> > >+              class="toolbar-focustarget"
> > >+              crop="right"
> > >+              observes="paragraph-select-container">
> > Not sure this will work. (Not even sure what it's trying to do.)
> We use it elsewhere for keeping the disable attribute in sync.
OK, but you should then avoid putting any attributes on the toolbaritem, since the observes will copy those too (particularly bad in the case of the toolbar-focustarget class).

> > >   </menulist>
> > >+  </toolbaritem>
> > I assume the indentation will be sorted out at some point?
> I could, but I was trying to avoid a lot of whitespace change and taking blame
> of massive sections of code.
In that case, would it be better putting the <toolbaritem> at the "wrong" indentation too, so at least that block is internally consistent?
Comment on attachment 515415 [details] [diff] [review]
Switch from toolbar id to class patch v1.0

> #FormatToolbar > toolbarbutton > .toolbarbutton-text {
>   display: none;
> }
So, are you planning on giving the buttons text and setting the format toolbar to default to icons only?
(Assignee)

Comment 25

7 years ago
(In reply to comment #23)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 515407 [details] [diff] [review])
> > > >+  <toolbarbutton id="AlignPopupButton"
> > > >+                 class="toolbarbutton-1"
> > > Why the class?
> > I have another patch coming up that switches the style sheet from using the
> > toolbar id to using the toolbarbutton/item class.
> OK, but it was a really bad choice of class. (The latest patch is still a bad
> choice of class, since it's intended for use with chrome flags. So you should
> really think up a new class name.)

editorPrimaryToolbar.css already uses toolbarbutton-1 to assign a list-style-image or are you saying this is wrong?
(Assignee)

Comment 26

7 years ago
(In reply to comment #24)
> Comment on attachment 515415 [details] [diff] [review]
> Switch from toolbar id to class patch v1.0
> 
> > #FormatToolbar > toolbarbutton > .toolbarbutton-text {
> >   display: none;
> > }
> So, are you planning on giving the buttons text and setting the format toolbar
> to default to icons only?

I plan on changing the above with the actual customisation patch.
(Assignee)

Updated

7 years ago
Attachment #515412 - Flags: review?(neil)
(Assignee)

Comment 27

7 years ago
Created attachment 515533 [details] [diff] [review]
toolbar item tidy up v1.3

Changes since toolbar item tidy up v1.2:
* Added new class "format-bar-class" for use with toolbarbuttons/items typically found on the format toolbar.
* Changed start indentation vele for new toolbaritems so the block is internally consistent.
* Changed observe lines to observe specific attribute.
Attachment #515412 - Attachment is obsolete: true
Attachment #515533 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #515415 - Flags: ui-review?(stefanh)
Attachment #515415 - Flags: review?(neil)
(Assignee)

Comment 28

7 years ago
Created attachment 515534 [details] [diff] [review]
Switch from toolbar id to class patch v1.1

Changes since Switch from toolbar id to class patch v1.0:
* Changed class used to "format-bar-class"
Attachment #515415 - Attachment is obsolete: true
Attachment #515534 - Flags: ui-review?(stefanh)
Attachment #515534 - Flags: review?(neil)
(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > (From update of attachment 515407 [details] [diff] [review])
> > > > >+  <toolbarbutton id="AlignPopupButton"
> > > > >+                 class="toolbarbutton-1"
> > > > Why the class?
> > > I have another patch coming up that switches the style sheet from using the
> > > toolbar id to using the toolbarbutton/item class.
> > OK, but it was a really bad choice of class. (The latest patch is still a bad
> > choice of class, since it's intended for use with chrome flags. So you should
> > really think up a new class name.)
> editorPrimaryToolbar.css already uses toolbarbutton-1 to assign a
> list-style-image or are you saying this is wrong?
That's different, since those are primary toolbar buttons... the ones that have traditionally been the large images above text. Although eventually I guess we could get rid of that class, since all toolbarbuttons will work the same in the end, the only difference being the toolbar modes.

(In reply to comment #26)
> (In reply to comment #24)
> > (From update of attachment 515415 [details] [diff] [review])
> > > #FormatToolbar > toolbarbutton > .toolbarbutton-text {
> > >   display: none;
> > > }
> > So, are you planning on giving the buttons text and setting the format toolbar
> > to default to icons only?
> I plan on changing the above with the actual customisation patch.
Right, it was just helpful for you to clarify that.
Comment on attachment 515534 [details] [diff] [review]
Switch from toolbar id to class patch v1.1

>+toolbarbutton.format-bar-class {
"-class" from the Department of Redundancy department :-)
Mind you, my original suggestion was toolbarbutton.formatting-button ;-)
Attachment #515534 - Flags: review?(neil) → review+
Comment on attachment 515533 [details] [diff] [review]
toolbar item tidy up v1.3

>+                 class="chromeclass-toolbar-additional format-bar-class"
OK I'm confused. Why do you need the chromeclass-toolbar-additional class?
Comment on attachment 515533 [details] [diff] [review]
toolbar item tidy up v1.3

r=me if you remove the chromeclass-additional-toolbar class. (I'll let you decide what you want to do with the format-bar-class class.)
Attachment #515533 - Flags: review?(neil) → review+
(Assignee)

Comment 33

7 years ago
(In reply to comment #13)
> Created attachment 515238 [details] [diff] [review]
> Tidy up some broadcasters and commandsets v2.1
> 
> Changes since v2:
> * Moved commands from broadcasterset to commandset.
> * Overlay using editorCommands rather than tasksCommands.
> * Moved entity from editorOverlay.dtd to editor.dtd.
> * Changed id for broadcastersets.

I've noticed the debugQATextEditor.xul uses <commands> rather than <commandset>, is it best to change that?
(Assignee)

Comment 34

7 years ago
Created attachment 515745 [details] [diff] [review]
toolbar item tidy up v1.4 [Checked in: Comment 43]

Changes since toolbar item tidy up v1.3:
* Removed chromeclass-additional-toolbar class.
* Renamed format-bar-class class to formatting-button.
Attachment #515533 - Attachment is obsolete: true
Attachment #515745 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #515534 - Flags: ui-review?(stefanh)
(Assignee)

Comment 35

7 years ago
Created attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

Changes since Switch from toolbar id to class patch v1.1:
* Renamed class format-bar-class to formatting-button.
Attachment #515534 - Attachment is obsolete: true
Attachment #515748 - Flags: ui-review?(stefanh)
Attachment #515748 - Flags: review+
Comment on attachment 515238 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.1

>+  <broadcasterset id="editorOverlayBroadcasters">
>+    <broadcaster id="Editor:Throbber" busy="false"/>
>   </broadcasterset>
Not sure it's worth sharing one broadcaster. Also I would just call the set editorBroadcasters (compare nav/mailBroadcasters).

>-  <command id="cmd_showComposeToolbar" oncommand="goDoCommand('cmd_showComposeToolbar')"/>
>-  <command id="cmd_showFormatToolbar" oncommand="goDoCommand('cmd_showFormatToolbar')"/>
>-  <command id="toggleSidebar"/>
You didn't remove the code for these.

>-<broadcasterset id="composeBroadcasters">
>-  <broadcaster id="Editor:Throbber" busy="false"/>
>+<!-- broadcaster nodes are appended here from the overlays -->
>+<broadcasterset id="editorOverlayBroadcasters">
I don't think Editor:Throbber belongs in this window. Mind you, the compose throbber doesn't seem to be hooked up to the progress listener, either directly or via the (missing) broadcaster_throbber element.

>                   <menuitem id="menu_showComposeToolbar"
>                             type="checkbox"
>                             label="&showComposeToolbarCmd.label;"
>                             accesskey="&showComposeToolbarCmd.accesskey;"
>-                            command="cmd_showComposeToolbar"
>-                            checked="true"/>
>+                            command="cmd_viewComposeToolbar"/>
>                   <menuitem id="menu_showFormatToolbar"
>                             type="checkbox"
>                             label="&showFormatToolbarCmd.label;"
>                             accesskey="&showFormatToolbarCmd.accesskey;"
>-                            command="cmd_showFormatToolbar"
>-                            checked="true"/>
>+                            command="cmd_viewFormatToolbar"/>
For a moment I thought this was going to be a problem with persistence, but I think it's only a niggle for someone with a hidden toolbar (the checkbox will reappear until the toolbar is toggled twice.)
Attachment #515238 - Flags: review?(neil) → review-
(In reply to comment #36)
> Mind you, the compose throbber doesn't seem to be hooked up to the progress
> listener, either directly or via the (missing) broadcaster_throbber element.
Filed bug 637630.

Comment 38

7 years ago
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

jftr, I will try to get to this in 1-2 days from now
(Assignee)

Comment 39

7 years ago
Created attachment 516079 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]

Changes since Tidy up some broadcasters and commandsets v2.1:
* Removed Editor:Throbber broadcaster from overlay.
* Removed js code relating to cmd_showComposeToolbar and cmd_showFormatToolbar.
* Gave broadcastersets different ids.
* Changed from commands to commandsets in debugQATextEditorShell.xul.
Attachment #515238 - Attachment is obsolete: true
Attachment #516079 - Flags: review?(neil)
Comment on attachment 516079 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]

Note to self: make -C suite/debugQA chrome doens't work.
Attachment #516079 - Flags: review?(neil) → review+

Comment 41

7 years ago
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

+toolbarbutton.formatting-button {

Do you have .formatting-button's that are not toolbarbuttons (not my call really, just curious on why you need the extra selector)?
Attachment #515748 - Flags: ui-review?(stefanh) → ui-review+
(Assignee)

Comment 42

7 years ago
Comment on attachment 516079 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]

http://hg.mozilla.org/comm-central/rev/369ad40b3fc1
Attachment #516079 - Attachment description: Tidy up some broadcasters and commandsets v2.2 → Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]
(Assignee)

Comment 43

7 years ago
Comment on attachment 515745 [details] [diff] [review]
toolbar item tidy up v1.4 [Checked in: Comment 43]

http://hg.mozilla.org/comm-central/rev/a4afd59a66b2
Attachment #515745 - Attachment description: toolbar item tidy up v1.4 → toolbar item tidy up v1.4 [Checked in: Comment 43]
(Assignee)

Comment 44

7 years ago
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

http://hg.mozilla.org/comm-central/rev/118029396d1b
Attachment #515748 - Attachment description: Switch from toolbar id to class patch v1.2 → Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]
(Assignee)

Comment 45

7 years ago
(In reply to comment #41)
> Comment on attachment 515748 [details] [diff] [review]
> Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]
> 
> +toolbarbutton.formatting-button {
> 
> Do you have .formatting-button's that are not toolbarbuttons (not my call
> really, just curious on why you need the extra selector)?

There are toolbaritems which have the formatting-button class, but neither of our themes style them, but that doesn't mean a theme out there won't want to.
Comment on attachment 515745 [details] [diff] [review]
toolbar item tidy up v1.4 [Checked in: Comment 43]

>diff --git a/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd

>-<!ENTITY alignjustifyButton.tooltip "Align text along left and right margins">
>+<!ENTITY alignJustifyButton.tooltip "Align text along left and right margins">

Please be careful changing editor/ - this change broke the Thunderbird tree (MozMill tests) as this caused a missing string in our version of editorOverlay.xul.

(I'd also query the real benefit of changing the string name like this, as although it gives a prettier name, it also causes more work for l10n).

I fixed the Thunderbird tree with this: http://hg.mozilla.org/comm-central/rev/c2ea4943b532
(Assignee)

Updated

7 years ago
Depends on: 638964
(Assignee)

Comment 47

7 years ago
Created attachment 517045 [details] [diff] [review]
Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]

When I was doing the other css changes for switching from toolbar id to class, it looks like I missed one.
Attachment #517045 - Flags: review?(neil)
Comment on attachment 517045 [details] [diff] [review]
Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]

>+.formatting-button menulist:-moz-lwtheme {
Please can you insert a > as we know the menulist will always be a direct child of the toolbaritem in this case. r=me with that fixed.
Attachment #517045 - Flags: review?(neil) → review+
(Assignee)

Comment 49

7 years ago
Comment on attachment 517045 [details] [diff] [review]
Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]

Checked in with > added.

http://hg.mozilla.org/comm-central/rev/7c2bd99a597d
Attachment #517045 - Attachment description: Fix css for formatting-button menulist v1.0 → Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]
clearing status request; IanN if this is close to being ready and has no remaining l10n impact, feel free to re-request.
status-seamonkey2.1: ? → ---
(Assignee)

Updated

7 years ago
Depends on: 694027

Updated

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