Closed Bug 731688 Opened 12 years ago Closed 7 years ago

Shift modifier key is ignored for "Edit Message as New" menu (Shift+Click, Shift+Enter to compose in non-default format: plaintext vs. HTML; message menu, msg context menu)

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: thomas8, Assigned: jorgk-bmo)

References

(Blocks 2 open bugs)

Details

(4 keywords)

Attachments

(2 files, 8 obsolete files)

We have Shift as a modifier key to toggle non-default format for starting a new message (Plaintext vs. HTML) on message header buttons, and on msg context menu commands like Reply, Forward etc. Surprisingly, and violating ux-consistency, holding Shift is ignored for the "Edit message as new" command, which will always assume the format of the original message, with or without Shift.

STR

1) Hold Shift while choosing "Edit message as new" from Message Menu, or Message Context Menu (we don't have a button, needs new bug).
1a) starting out from a plaintext msg
1b) starting out from a HTML msg
observe message format in composition window


2) For comparison:
Hold Shift while choosing any of the following commands from message *context* menu (or message header buttons):
Reply to Sender Only, Reply to All, Reply to List, Forward;
observe message format in composition window

Actual result

1a) with or without Shift, new msg will be plaintext format (like original msg)

1b) with or without Shift, new msg will be HTML format (like original msg)

2) For all other commands that start a new message (reply, forward etc.), we respect the Shift modifier to trigger non-default format composition (not yet in the main message menu, that's similar bug 731673)

Expected result

1a) Shift when original msg is plaintext: start the new msg in non-default format (currently HTML)

1b) Shift when original msg is HTML: start the new msg in non-default format (currently plaintext)

This is *not* a duplicate of bug 78794. It's a workaround minimal fix for that bug. The underlying problem of bug 78794 is that for original plaintext messages, there is *no way at all* to edit them as new in HTML format. We may or may not want to solve that bug by making the "edit as new" behaviour consistent with replying or forwarding: By default, replies to or forwards of a plain-text msg will have HTML format. Why behave different for "edit as new"? Regardless, this bug has a more limited scope: Whatever the default format for "edit as new" of original msgs in their flavors, holding Shift should trigger the *non-default* format, as we do for any other commands that start a new msg.
Related work was once done by Magnus in Bug 254931, and is currently going on in Bug 731673. Bug 731673 Comment 20 has information concerning this bug.
Richard, fixing this bug would be great to provide a minimal workaround fix for the major painpoint that "Edit as New" on an existing plaintext msg forces users to compose in plaintext with no way out, and regardless of their declared preference for HTML (bug 78794, which is really just the symptom of several other known design bugs in this corner). For details why this is bad, see Bug 78794 Comment 41.
 
As you successfully fixed related bug 731673 (Honor Shift modifier for starting new message in non-default format in message menu), perhaps there's a starting point for trying this one, too... :)
I tried to add the event variable on all occurrences of MsgEditMessageAsNew() but this didn't make a difference. I think this relies on the implementation of nsIMsgCompType in cpp. nsIMsgCompType::Template is different than nsIMsgCompType::Reply etc. and doesn't give the possibility to switch between plain and HTML.

But I never programmed C++ and to dig deeper is beyond my possibilities. Maybe aceman could have a look on it.
See Also: → 975816
Current "Edit As New" behavior:
(1) When text/plain mail : Open composer in Text mode
(2) When text/html mail : Open composer in HTML mode
(3) When multipart/alternative mail:
      (3-1) View/message Body As/Plain Text : Used part in display is text/plain part, so open composer in Text mode.   
      (3-2) View/message Body As/HTML       : Used part in display is text/html part, so open composer in HTML mode. 
It's pretty simple. If in Text mode, edit in Text mode. If in HTML mode, edit in HTML mode.
It's mainly because "Edit As New" == "Edit Template" == "Compose a new mail from Template", since initial of "Edit As New".

Another reason why current behavior is (3).
   When imap and mail is not held in auto-sync=on/Offline-Use=On folder
   and mail.imap.mime_parts_on_demand=true(default=true),
   if View/message Body As/Plain Text, "text/plain part under multipart/alternative" only  is fetched.
   text/html part is not fetched because it's not needed for mail display.
   So, it's impossible to use data in text/html part for mail composition.

Design/implementation change like next is better for consistency of Shift modifier in mail composition world?
(A) No change in behavior without Shift.
(B) Edit As New with Shift == Toggle composition mode.
(1-S) When text/plain mail : Open composer in HTML mode, using text in text/plain, 
                                              with "Delivery format=Plain Text Only" if and only if possible.
(2-S) When text/html mail : Open composer in Text mode using "text converted from HTML".
(3-S) When multiprt/alternative mail:
      (3-1-S) View/message Body As/Plain Text : 
                   Open composer in Text mode, using text in text/plain, with "Delivery format=Plain Text Only" if and only if possible.
                   (if imap offline-use=Off and View/Message Body As/Plain Text,                    )
                   (text perhaps becomes "This body part will be downloaded on demand."     )              
                   (if text/html part is used, unless text/html is fetched for "Edit As New".        )
      (3-2-S) View/message Body As/HTML       :
                   Open composer in Text mode, using text in text/plain part,
                   or using text converted from text/html part if possible.
FYI.
Definition/code for double click at threadPane.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#39
>    In ThreadPaneOnClick(event), if double clicked at non-twisty, call  ThreadPaneDoubleClick()
> http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#146
>   In ThreadPaneKeyDown(event), if "Enter", call  ThreadPaneDoubleClick()
> http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#129
>  ThreadPaneDoubleClick()
>     If Drafts, MsgComposeDraftMessage(),
>     Else If Templates, ComposeMessage(),
>     Else  MsgOpenSelectedMessages().
"Edit As New"
>  http://mxr.mozilla.org/comm-central/source/suite/mailnews/mail3PaneWindowCommands.js#504
>     calls MsgEditMessageAsNew();
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1867
>    calls composeMsgByType
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1540
> 1540 function composeMsgByType(aCompType, aEvent) {
> 1546   if (aEvent && aEvent.shiftKey) {
> 1547     ComposeMessage(aCompType,
> 1548                    Components.interfaces.nsIMsgCompFormat.OppositeOfDefault,
> 1549                    msgFolder, msgUris);
> 1550   }
> 1551   else {
> 1552     ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.Default,
> 1553                    msgFolder, msgUris);
> 1554   }

composeMsgByType already supports event parameter for Shift modifier processing. (for Shift+Compose etc.)
Problem in "Edit As New" is that it's uses oncommand="goDoCommand('cmd_editAsNew')" in command so there is no standard way to pass event object to composeMsgByType from menu.

A possible solution for passing event to composeMsgByType.
- Supports event parameter in MsgEditMessageAsNew() and pass event to composeMsgByType.
      function MsgEditMessageAsNew(event) { composeMsgByType(Components.interfaces.nsIMsgCompType.Template,event); }
- in command id=cmd_editAsNew, stop using oncommand="goDoCommand('cmd_editAsNew')".
  Directory call MsgEditMessageAsNew insteaad of via goDoCommand.
  command id="cmd_editAsNew" oncommand="MsgEditMessageAsNew(event)">

To get required behavior, needless to say,  "process in ComposeMessage() when aCompType==Components.interfaces.nsIMsgCompType.Template" should be modified.
FYI.
"Opening composer window for Edit As New(edit template) and Forward in Inline" is executed here.
> http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1449
"Forward in Inline" already does do "Toggle composition mode if Shift modifier is pressed" regardless of original mail format.
(because of Forward in Inline, if HTML, convert to text, and if overrideComposeFormat, set mode=OppositeOfDefault)
 i.e. If similar logic is applied to Edit As New(edit template), Shift modifier can be supported in Edit As New(edit template)  too.

Thomas D, you perhaps will want Shift+editDraft too :-)
Patch for Bug 321355 is one liner patch. It simply stops "setting previous/editing DraftId upon composition start by Edit As New".
>        if (mdd->format_out == nsMimeOutput::nsMimeMessageEditorTemplate)
>        {
> -        fields->SetDraftId(mdd->url_name);
>           After it, Open Composer Window.
Trick in original "Edit draft" / "Edit template"(Edit As New) was "if folder is not Drafts folder, previous DraftId is ignored in mail composition".
i.e. "What is needed in editDraft after bug 321355" is "set previous/editing DraftId upon composition start" only.
If this bug will be fixed, editDraft of bug 1106412 will automatically support Shift modifier.
(In reply to WADA:World Anti-bad-Duping Agency from comment #6)
> FYI.
> "Opening composer window for Edit As New(edit template) and Forward in
> Inline" is executed here.
> > http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1449

WADA, could you please update this link? To alleviate link rot, maybe quoting one line can be helpful.
Would you be able to fix this bug? I would like to fix this, but it's a bit deep...

Richard or Jörg, could you provide a complete call stack at a suitable line (maybe WADA's updated line)? Perhaps at
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1325

Jörg, any ideas how to fix this?
Flags: needinfo?(richard.marti)
Flags: needinfo?(m-wada)
Flags: needinfo?(jorgk)
I understand that the issue can be annoying. Say you wrote a message that got auto-downgraded. Now you want to use it again but add some formatting or an embedded image.

The idea or at least the implementation of editing a message as new is to reproduce the *exact* same message as the one before. Exact same means that you get the same format as well.

The processing is all done in 17+ y/o MIME code (04/20/2000, see first few lines of mimedrft.cpp) that we will not touch. Besides, the compose pipeline is highly complicated and regression prone (attachment 8819691 [details]).
Flags: needinfo?(jorgk)
I can't add more to Jörg's reply with his much more insight in this code.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #9)
> I can't add more to Jörg's reply with his much more insight in this code.

Thank you Jörg for sharing your extraordinary insight into compose pipeline from bug 1323377 attachment 8819691 [details], which is amazing and shocking at the same time. Must have been lots of hard work, and it's clearly impossible for any mere mortal like myself to figure that out...

(In reply to Jorg K (GMT+2) from comment #8)
> I understand that the issue can be annoying. Say you wrote a message that
> got auto-downgraded. Now you want to use it again but add some formatting or
> an embedded image.

Exactly. That's the main UX problem which is annoying lots of users: Bug 78794, currently 15 duplicates, and 17 years after filing, comments of annoyed users keep coming (last comments 3 months ago).

> The idea or at least the implementation of editing a message as new is to
> reproduce the *exact* same message as the one before. Exact same means that
> you get the same format as well.

Yes, I think this is historically accurate. However, times have changed, and it turns out that users have never liked that idea because it's a pita for their legitimate "Edit as new" workflows. For users who generally compose in HTML (and all prefs set accordingly), it just doesn't make sense, ever, to start a composition which is limited to plaintext.
That old idea is also pretty useless because even if we change it so that "Edit as new" of "plaintext msg" becomes "HTML composition", Auto-Downgrade feature of Delivery-Format:Auto-Detect will still send the unaltered HTML message as plaintext, so the net result for the plaintext-only scenario is the same, only that we'll no longer *force* HTML users into plaintext limitations.
Note that any other way of starting out from existing plaintext msg (Reply, Forward) will always compose in HTML, so it looks ux-inconsistent that "Edit as new" doesn't.

I think "reproduce exact same message incl. format" makes sense and is required for messages which are explicitly created as *templates* (in Templates folder), where format of template is intentionally defined by user. For any non-template messages, format is not necessarily intentionally set by user:
- Auto-Downgrade against user's will -> non-intentional plaintext msg in Sent folder
- Incoming plaintext messages -> non-intentional plaintext msg in inbox
- Even with Auto-Downgrade OFF (user pref/Options UI),
  problem of legacy plaintext msgs in Sent folder etc. remains -> non-intentional legacy plaintext msgs in Sent folder or elsewhere

So indeed (as Jörg hinted) it looks more like ux-implementation-level problem now: If I read compose pipeline of attachment 8819691 [details] correctly, "Edit as new" is implemented same as "Template", and it's non-trivial to disentangle the two.

> The processing is all done in 17+ y/o MIME code (04/20/2000, see first few
> lines of mimedrft.cpp) that we will not touch. Besides, the compose pipeline
> is highly complicated and regression prone (attachment 8819691 [details]).

Yes, I understand and appreciate that. Looking at attachment 8819691 [details], it's obvious that nobody wants to touch compose pipeline, and many including myself would probably fail or take ages even if they tried. However, rumor has it that TB has an ingenious developer called Jörg K. who has recently fixed a similar problem in compose pipeline in less than 100 comments including plenty of comments by WADA... :-)

So our only hope get this bug 731688 or bug 78794 fixed is if we can succeed to convince/bribe/pray this developer into a surge of ambition or a flush of genious (Geistesblitz) to tackle this issue head-on, something like...
... "Wtf, let's try this":
- create "nsIMsgCompType::EditAsNew"
- adjust compose pipeline to handle nsIMsgCompType::EditAsNew similar to "Forward-Inline"
- special case nsIMsgCompType::EditAsNew to match the expected behaviour matrix; see bug 78794, comment 59: Use compose-HTML for Edit-as-new for all scenarios except when (original msg == plaintext && identity/server-compose-pref == plaintext).
- pass through nsIMsgCompFormat::Default, nsIMsgCompFormat::OppositeOfDefault correctly with nsIMsgCompType::EditAsNew, and obey that according to the behaviour matrix.
- Maybe we could even keep "EditAsNew" bundled with "Template" like now, if we can find some other way of special-casing the nsIMsgCompFormat information, perhaps with another flag similar to "forward-inline" flag, which then tweaks the format at some appropriate spot?
I have a few burning fires right now, but I'll put it onto the "to do" list for the next six months.
(In reply to Jorg K (GMT+2) from comment #11)
> I have a few burning fires right now, but I'll put it onto the "to do" list
> for the next six months.

Thanks Jörg, you rock! Affected users will be forever grateful to you...

I had written another comment before seeing yours (in mid-air-collision) where I suggested this as a starting point:

> Jörg, I hardly dare asking, but would be possible (at any time of your convenience) to enrich your composition pipeline map
> of attachment 8819691 [details] so that it reflects how nsIMsgCompFormat parameters are passed through/handled for "Edit as New",
> similar to how you documented type parameter? That might go a long way towards approaching an understanding of what needs
> to be done for this bug 731688 or bug 78794, respectively.
Attached patch 731688-edit-as-new.patch (v1). (obsolete) — Splinter Review
Ingenious ;-)

To use it, you need to use the context menu and shift+click onto "Edit as New Message". Shift+Ctrl+E doesn't work and strangely shift+click also doesn't work on the main message menu.

I find wrapping the original into a <pre> block like is done for inline forward rather unfortunate since there are limited edit facilities for <pre>. I suggest to simply change \n to <br> and be done with it.

Anyway, this proves the point. We can now refine it a little.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8893607 - Flags: feedback?(acelists)
(In reply to Jorg K (GMT+2) from comment #13)
> Created attachment 8893607 [details] [diff] [review]
> 731688-edit-as-new.patch (v1).
> 
> Ingenious ;-)

Awesome! Thanks a lot for looking into this.

> To use it, you need to use the context menu and shift+click onto "Edit as
> New Message". Shift+Ctrl+E doesn't work

Traditionally we don't support Shift to toggle compose format for the keyboard shortcuts, because some of the shifted shortcuts are already taken for other commands (e.g. Ctrl+Shift+R).

> and strangely shift+click also
> doesn't work on the main message menu.

I also noticed that in my tests, we should unify that codepath if possible.

> I find wrapping the original into a <pre> block like is done for inline
> forward rather unfortunate since there are limited edit facilities for
> <pre>. I suggest to simply change \n to <br> and be done with it.

Plaintext created by TB is mostly format-flowed, where linebreaks don't matter.
I've composed flowing text with <p>, it's displayed as flowing text with blank lines (mimicking <p>), so when I "Edit as new" the same msg, I'd expect an HTML composition with flowing text again. Maybe we could even do this:

plaintext,format-flowed

par1 line1\n
par1 line2\n
\n
par2 line1\n
par2 line2\n

Upon "Edit as new", convert that to HTML:

<p>par1 line1\n
par1 line2\n
</p>\n
<p>par2 line1\n
par2 line2\n
</p>

That looks like a straightforward conversion to me.
If <p> is not possible, we could just replace \n\n with <br><br>, but leave single \n alone so that the body text will flow.

If there are other plaintext types where the linebreaks in source are supposed to be rendered, perhaps we could convert every \n to <br> as Jörg suggested, but only for the non-format-flowed types.
(In reply to Jorg K (GMT+2) from comment #13)
> We can now refine it a little.

Bug 78794, comment 67 describes the target matrix for compose formats of "Edit as new" on non-template messages.
Nothing new and nothing special, the only change is a) below.

Can we please ensure that using a real template from templates folder will preserve plaintext format?
Double-click and "Edit as new" for an explicit plaintext *template* should compose plaintext.
I think there might be legitimate workflows/use cases where explicit plaintext templates are required to prevent template users from applying HTML formatting.
"Edit as new" on any non-template message is different, because plaintext format is not guaranteed to be intentional.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #60)
> If we implement a) from comment 59, this means that "Edit as new" will
> behave slightly differently from using an explicit template from templates
> folder to start a new message:
>                    Original msg   Identity/server pref   composition format
> "Edit as new":  a) Plaintext      HTML                   HTML      (this bug)
> "Use template":    Plaintext      HTML                   Plaintext
> 
> Imo, this is required because for an explicit template, we must preserve the
> user-defined composition format flavor of the template. User is free to save
> de-facto plaintext template in HTML if we wants to compose in HTML from that
> template (e.g. by unchecking "Send the message in plaintext if possible", or
> by sending to self with "Delivery format: HTML", or by including one
> non-convertible HTML tag).
> 
> By contrast, user may not have a choice about the format of the original
> message when doing "Edit as new", so we should always allow HTML formatting
> for users composing in HTML.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #15)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #60)

Sorry, that's Bug 78794, comment 60.

> > If we implement a) from comment 59, this means that "Edit as new" will
> > behave slightly differently from using an explicit template from templates
> > folder to start a new message:

That's Bug 78794, comment 59.
I answered in bug 78794 comment #68.

You're complicating the proposed solution. We don't distinguish between using a template or editing a message as new. If I understand you correctly, you want different rules for these two.

I've proposed a simple implementation for the problem which gives the user the possibility to get the format they want.

Please don't stand in the way of fixing a 16 y/o bug (bug 78794) by trying to impose your rules.
Comment on attachment 8893607 [details] [diff] [review]
731688-edit-as-new.patch (v1).

I have no time for endless discussions. This is how it could be done. If someone wants to pick up the patch, feel free.
Attachment #8893607 - Attachment is obsolete: true
Attachment #8893607 - Flags: feedback?(acelists)
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
As I see it, there are two options here:

1) with the modifier key you get the opposite  message  format (as implemented).
2) with the modifier key you get the opposite *account* format.

So that would be for a HTML user:
New msg                  -> HTML
Reply to plaintext       -> HTML
Forward of plaintext     -> HTML
Edit as new of plaintext -> HTML

And for a plaintext user:
New msg                  -> plaintext
Reply to HTML            -> plaintext (loosing all rich content)
Forward of HTML          -> plaintext (loosing all rich content)
Edit as new of HTML      -> plaintext (loosing all rich content)
Clarifying:

And for a plaintext user:
New msg                  -> plaintext
Reply to HTML            -> plaintext (loosing all rich content) - EXISTING BEHAVIOUR!
Forward of HTML          -> plaintext (loosing all rich content) - EXISTING BEHAVIOUR!
Edit as new of HTML      -> plaintext (loosing all rich content) - New changed behaviour.

So the plaintext user would get all messages as plaintext, including for "edit as new". The last line changes existing behaviour, the same dataloss as in reply/forward, but highly visible and extremely good for ux-discovery ;-)
I had some private exchange with Thomas.

He suggests to use option 2) (opposite account format) for "edit as new" of a normal message and option 1) (opposite of message format) for "edit as new" of a template.

As I said before, we currently don't distinguish the two cases, but here's a patch from another bug which tried to introduce the distinction.
Oops, wrong patch. Here's the right one.
Attachment #8895042 - Attachment is obsolete: true
Rebased.
Attachment #8895043 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #21)
> Created attachment 8895042 [details] [diff] [review]
> edit-as-new.patch - Kent's patch from another bug to introduce "edit as new"
> as separate compose type
> 
> I had some private exchange with Thomas.
> 
> He suggests to use option 2) (opposite account format) for "edit as new" of
> a normal message and option 1) (opposite of message format) for "edit as
> new" of a template.

Thanks, that's a correct and concise bare-bones summary of my proposal, for the case when "Shift" modifier is used.
I think my proposal will be much easier to understand if we describe the non-SHIFTed default cases:

PROPOSAL

A) For "Edit as new" of a normal message:
   2) respect *account* format, ignore *message* format

B) For "Edit as new" of an explicit TEMPLATE (user-defined message in template folder):
   1) respect *message* format, ignore *account* format

A) is required to fix bug 78794 with a better default workflow: For HTML users, "Edit as new" of a random plaintext message should open HTML composition, because we don't want them to get stuck when they "Edit as new" their own auto-downgraded plaintext messages from Sent folder which were originally composed in HTML.

B) is required to allow both plaintext/HTML users sufficient control over the target format of their explicit, user-defined TEMPLATES: For an HTML user, we want to allow plaintext template which ensures/enforces plaintext composition, because that might be a legitimate and required use case. For a plaintext user, we want to allow HTML template which ensures/enforces HTML composition, dito. It doesn't make sense for plaintext user to have an explicit HTML template and expect that to be downgraded to plaintext every time the template is used in the default pattern (without holding undiscoverable Shift modifier).

> As I said before, we currently don't distinguish the two cases, but here's a
> patch from another bug which tried to introduce the distinction.

:)
For the avoidance of doubt, my proposal with SHIFTed flavors included

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #24)
> 
> PROPOSAL
> 
> A) For "Edit as new" of a normal message:
>       Default: Use *account* format, regardless of *message* format
     2) + Shift: Use opposite of *account* format, ...
>
> B) For "Edit as new" of an explicit TEMPLATE
>    (user-defined message in template folder):
>       Default: Use *message* format, regardless of *account* format
     1) + Shift: Use opposite of *message* format, ...

Needless to say, in many real-life cases where users compose in HTML (TB default), and existing normal messages or explicit templates have some HTML formatting, account format and message format will be the same by default.
With this bug, we'll implement Shift modifier to override the default format and get the opposite format.
So this proposal seems to offer a maximum of ux-consistency, ux-efficiency, and ux-control in good balance.
Attachment #8895053 - Attachment description: Kent's patch from another bug to introduce "edit as new" as separate compose type → Part 1: introduce "edit as new" as separate compose type (by Kent James)
This patch implements the following for "Edit as New".

Note: Starting a new compose window by double-clicking on a template in the Template folder is unchanged, so that always uses the template format. I noticed, that Shift+Double-Click doesn't set the "override" flag, so I decided to leave template processing as it is. Of course a template can also be "edited as new", in which case the "Edit as new" behaviour applies.

Summary of behaviour for "Edit as New":

No shift key used:
  Respect *account* format, ignore *message* format.
  This changes (!) existing behaviour which just reuses the message format.
  This brings "Edit as new" inline with New, Reply and Forward which
  all respect the account format.

With shift key used. Note that previously the shift key had no effect, so this is new behaviour:
  Use opposite *account* format, ignore *message* format.

Problem:
  Shift+click only works on the context-menu, not the main menu.

Please apply part 1 before part 2 :-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(m-wada)
Attachment #8895542 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #26)
> Created attachment 8895542 [details] [diff] [review]
> Part 2: 731688-edit-as-new.patch (v2)

Jörg never seizes to amaze, blazing-fast fixes for age-old problems deep in the internals. Thanks a lot! :)

> This patch implements the following for "Edit as New".
> 
> Note: Starting a new compose window by double-clicking on a template in the
> Template folder is unchanged, so that always uses the template format. I
> noticed, that Shift+Double-Click doesn't set the "override" flag, so I

OT: A flaw in Jörg's English! 8O *shocked*
Rule of thumb: No comma before 'that'... (rare exceptions exist).

Double-clicking triggers default action, which is currently realized in 'Edit as new'. So from an UX-consistency perspective, there's no reason why 'Edit as new' accepts Shift, but double-click doesn't. Let's make it consistent please, either here, or in a follow-up bug. Wada's analysis of comment 5 also covers double-click pipeline, which might help.

> decided to leave template processing as it is. Of course a template can also
> be "edited as new", in which case the "Edit as new" behaviour applies.
> 
> Summary of behaviour for "Edit as New":

Now I'm confused.
Does this implement my proposal of comment 25? (I think not)

'Edit as New' on a Template is currently the only available way of doing 'Compose new message using this template' action from menu.
For composing based on explicit template (where explicit means: user-defined template in Templates folder), we must use the *message* format, because we need to assume that the message format is an intentional property of the template which should be honored wrt composition mode (for legitimate use cases where ensuring/enforcing a particular format and composition mode is important).

From your description, your patch seems to implement the following behaviour (pls correct me if I'm wrong):

Double-click on template -> use *message* format.
"Edit as new" on template -> use *composition* format.

In the current UI dispensation, we can't have different behaviors for what is essentially the same action.
I see two possible solutions:

1) Implement my comment 25:
"Edit as new" on any other message    -> use *composition* format
"Edit as new" on an explicit template -> use *message* format

(+) We don't add another action to message (context) menus
(-) Slightly inconsistent behaviour where "Edit as new" acts differently on normal messages vs. templates.

2) Add a new menu item for "New Message from Template" (in addition to "Edit As New Message")
"Edit as new" on an any message (template or not)      -> use *composition* format
"New Message from Template" / double-click on template -> use *message* format
Obviously, we'll only show "New Message from Template" action in Templates folder.

(+) "Edit as new" always consistent in behaviour
(+) Dedicated default action for "Compose from Template" (currently missing; the whole templates experience is quite under-developed)
(-) Yet another action on the context menu

I need to give this some more thought, but maybe 2) is better, to keep things consistent and explicit.
Obviously we can't have a covert default action which is not represented in context menu.

As a variant of 2), maybe we could also hide "Edit as new" from context menu of explicit template (in templates folder), and show "New message from Template" instead, on top. It really doesn't make much sense, say, for a plaintext user, to keep a rich HTML template and then strip all HTML upon "Edit as new". I'm totally failing to see any use case for that.
However, it might make sense for an HTML user to keep an explicit plaintext template (maybe unintentionally saved as plaintext in the aftermath of Auto-Downgrade), but sometimes wanting to compose HTML from that. As Shift is undiscoverable, having dedicated, separate "Edit as new" command might come in handy.

> No shift key used:
>   Respect *account* format, ignore *message* format.
>   This changes (!) existing behaviour which just reuses the message format.
>   This brings "Edit as new" inline with New, Reply and Forward which
>   all respect the account format.

That sounds good and ux-consistent (although I'm still not 100% sure about stripping HTML for plaintext users).

> With shift key used. Note that previously the shift key had no effect, so
> this is new behaviour:
>   Use opposite *account* format, ignore *message* format.

So this is where Plaintext user can use Shift + 'Edit as new' on an HTML message to compose HTML from that message, which kind of mitigates my concern; otoh, the Shift trick is currently undiscoverable.
Iow, whenever plaintext user wants to compose HTML, he must hold shift to override his account preference.
(Except for composing from explicit templates, where imo we need to respect message format regardless of account preference)

> Problem:
>   Shift+click only works on the context-menu, not the main menu.

That's easy :) From WADA's well-researched comment 5, here's the solution:
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#252
>  <command id="cmd_editAsNew" oncommand="goDoCommand('cmd_editAsNew')"/>
Replace with this:
  <command id="cmd_editAsNew" oncommand="MsgEditMessageAsNew(event);"/>
As simple as that.
Per my comment 27, I withdraw my proposal of comment 25. So here's my NEW Proposal:

PROPSAL II:

A) For "Edit as new" of *any* message:
   Use *account* format (ignore message format)
   + Shift: Use opposite of *account* format (ignore message format)
   (This bug, as implemented in current patch, attachment 8895542 [details] [diff] [review])

For context menu of explicit *template* (message in Templates folder):
   (to be implemented, here or in a new bug)

B) Hide "Edit as new" from template context menu
   Keep the context menu clutter-free and ux-error-prevention for template users
   (ignoring *message* format when composing from an explicit template violates the inherent concept of a template, and it's hard to imagine use cases for that; note that saving a simple HTML composition as template will be saved as HTML, no Auto-Downgrade, so it takes intentional effort for HTML user to create plaintext template)
   Not sure if we also want to hide it in main menu, maybe not.
   We don't disable the command, so Ctrl+E will still work for "Edit as New" on Template.
   Note: The current design where "Edit as new" acts as "New Message from Template" is poor design based on the current technical implementation which conflates "Template" and "Edit as New" (ux-implementation-level), which is the root cause of this bug.

C) Show "New Message from Template" menu item instead
   (Default menu action for template, currently missing, same as double-click; so we should also allow Shift+Double-click)
   Use *message* format (ignore account format)
   + Shift: Use opposite of *message* format (ignore account format)
   (Inherent concept of "Template": Allow ensuring/enforcing target format and composition mode based on user-defined format of the template message).
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #28)
> Per my comment 27, I withdraw my proposal of comment 25. So here's my NEW
> PROPSAL II:
> C) Show "New Message from Template" menu item instead
>    (Default menu action for template, currently missing, same as
> double-click; so we should also allow Shift+Double-click)
>    Use *message* format (ignore account format)
>    + Shift: Use opposite of *message* format (ignore account format)
>    (Inherent concept of "Template": Allow ensuring/enforcing target format
> and composition mode based on user-defined format of the template message).

Since that's the current behaviour of "Edit as Draft" (before Kent's PART 1 patch, attachment 8895053 [details] [diff] [review]), I believe that this is quite simple to implement:

At https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1984,
just add the following function:
> function MsgEditMessageAsNew(aEvent)

function MsgNewMessageFromTemplate(aEvent)
{
  composeMsgByType(Components.interfaces.nsIMsgCompType.Template, aEvent);
}

Then implement the new command and UI in XUL:

https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#253
>   <command id="cmd_editAsNew" oncommand="goDoCommand('cmd_editAsNew')"/>
   <command id="cmd_newMessageFromTemplate" oncommand="MsgNewMessageFromTemplate(event)"/>

Plus menu items with respective calls of the command (or the function), plus strings.

Iow, current patches correctly disentangle and massage "Edit as new", but instead of dumping the current functionality of composing from template (wrongly branded as "Edit as new"), we must keep that functionality and just rebrand as "New Message from Template", which is currently missing.
Wow, a lot to take in here. Frankly, I never used templates, so I only found out recently that double-clicking one opens a new composition with that template like it were a draft, sadly no notification: "This is a template [Use]" like we have for drafts: "This is a draft [Edit]".

I was also looking for a "New Message from Template" menu item and didn't find one. As discussed, "Edit as new" on a template currently runs the "Edit as new" processing not the template processing.

I'm in favour of Thomas' suggestion to clean up the UX for templates just like he did for drafts in bug 1106412. Thanks, comment #5 is also very useful. Let's refresh the reference here:
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mail/base/content/threadPane.js#137

So what's the way forward? Take the proposed patch and add the fix for the main menu (bottom of comment #27) and move the template clean-up to a new bug?
Attached patch 731688-edit-as-new.patch (v3). (obsolete) — Splinter Review
Same as v2 but with an additional line that enables the shift key on the main menu, courtesy of Wada and Thomas. Thanks, nice teamwork!

What's left to do is to clean up the template handling UX, but since we already have two parts by two different authors here, that's best left for another bug.

Besides, Thomas is the specialist of these things, as bug 1106412 shows. There he added some tricky handling of those menu items which are hidden under certain circumstances:
https://hg.mozilla.org/comm-central/rev/e2845f0b82eb5e02a580d3d29e57dd84e36c21c2#l3.54

Maybe we can win him for the follow-up.
Attachment #8895542 - Attachment is obsolete: true
Attachment #8895542 - Flags: review?(acelists)
Attachment #8895783 - Flags: review?(acelists)
Blocks: 1389062
(In reply to Jorg K (GMT+2) from comment #31)
> Created attachment 8895783 [details] [diff] [review]
> 731688-edit-as-new.patch (v3).
> 
> Same as v2 but with an additional line that enables the shift key on the
> main menu, courtesy of Wada and Thomas. Thanks, nice teamwork!

Indeed! :)

> What's left to do is to clean up the template handling UX, but since we
> already have two parts by two different authors here, that's best left for
> another bug.

+1

> Besides, Thomas is the specialist of these things, as bug 1106412 shows.
> There he added some tricky handling of those menu items which are hidden
> under certain circumstances:
> https://hg.mozilla.org/comm-central/rev/
> e2845f0b82eb5e02a580d3d29e57dd84e36c21c2#l3.54
> 
> Maybe we can win him for the follow-up.

Maybe :)
Blocks: 1389083
OS: Windows XP → All
Thomas pointed out to me (in a PM) that converting plain text to HTML by sticking it into a <pre> as currently implemented and left unchanged in my patch is somewhat suboptimal, as I already mentioned myself in comment #13.

Let's just see what Aceman, who hasn't spoken at all, thinks about the approach in general and then we address this aspect further.

There is certainly already code in the box that converts text to HTML, for example when viewing a plain text message, *bold* received <b> tags, links are converted to <a>, etc. I think it's in mozTXTToHTMLConv::ScanTXT().

I'll give that a try.
This does the trick, I get for example:

This is <b class="moz-txt-star"><span class="moz-txt-tag">*</span>bold<span class="moz-txt-tag">*</span></b>, and this is <i class="moz-txt-slash"><span class="moz-txt-tag">/</span>italics<span class="moz-txt-tag">/</span></i>

when starting with:
  This is *bold*, and this is /italics/

Needs some polishing.
Attachment #8896404 - Flags: feedback?(acelists)
Comment on attachment 8896404 [details] [diff] [review]
731688-edit-as-new.patch (v4) - WIP

Hmm, doesn't work for quotes (yet). And the malloc/strcat should have just been a strdup.
Just for the record, the blockquote processing doesn't come for free, for displaying plain text messages (where the display *is* HTML), we seem to be doing it here in MIME:
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/mime/src/mimetpla.cpp#365
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/mime/src/mimetpfl.cpp#413

So to get this to work here, we'd have to (understand and) replicated that code, and by the looks of it, for plain text and flowed plain text. Not easy.
(In reply to Jorg K (GMT+2) from comment #34)
> Created attachment 8896404 [details] [diff] [review]
> 731688-edit-as-new.patch (v4) - WIP
> 
> This does the trick, I get for example:
> 
> This is <b class="moz-txt-star"><span class="moz-txt-tag">*</span>bold<span
> class="moz-txt-tag">*</span></b>, and this is <i class="moz-txt-slash"><span
> class="moz-txt-tag">/</span>italics<span class="moz-txt-tag">/</span></i>
> 
> when starting with:
>   This is *bold*, and this is /italics/
> 
> Needs some polishing.

I suspect that using conversion code originally written for *display* for our purposes of *composition* here might backfire: Remember that all moz-classes will be auto-downgraded. I.e. we're pretending to have a lot of real HTML formatting in the message, then after sending it'll all be gone. Let's make sure that whatever formatting we see in composition will actually survive sending! (ux-wysiwyg)

What if you'd use that code which converts a plaintext message for HTML reply? That's more likely to do the right thing.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #37)
> What if you'd use that code which converts a plaintext message for HTML
> reply? That's more likely to do the right thing.
HTML reply whacks a non-flowed plaintext original into a <blockquote><pre> :-(
HTML reply to a flowed message is better, but the *bold* and /italics/ are not detected.
Anyway, there is no code reuse here in the sense of calling a function, all this processing sits deep down in MIME and can at best be copied and adapted. I'd say it still happens where indicated in comment #36.

BTW, if they upgrade plain text to HTML and then don't add any rich formatting, what's wrong with downgrading it again?

Also, <b class="moz-txt-star"> and <i class="moz-txt-slash"> should most definitely *not* be downgraded again.
(In reply to Jorg K (GMT+2) from comment #38)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #37)
> > What if you'd use that code which converts a plaintext message for HTML
> > reply? That's more likely to do the right thing.
> HTML reply whacks a non-flowed plaintext original into a <blockquote><pre>
> :-(
> HTML reply to a flowed message is better,

So you could use that for non flowed plaintext, too.
> but the *bold* and /italics/ are
> not detected.

They shouldn't be detected imo. Per ux-wysiwyg, any thing displayed bold must be sent. If you have real bold print in composition, it won't downgrade. But as you say, an unchanged plaintext msg should downgrade. Let's just take the plaintext as-is, but in a flowed style with bock quotes.

> Anyway, there is no code reuse here in the sense of calling a function, all
> this processing sits deep down in MIME and can at best be copied and
> adapted. I'd say it still happens where indicated in comment #36.
> 
> BTW, if they upgrade plain text to HTML and then don't add any rich
> formatting, what's wrong with downgrading it again?
Nothing.
> Also, <b class="moz-txt-star"> and <i class="moz-txt-slash"> should most
> definitely *not* be downgraded again.
Why not? These are for display, not production. We do downgrade all moz-classes
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #39)
> Let's just take the plaintext as-is, but in a flowed style with bock quotes.
Still not easy, the tricks need to be copied from the MIME code (as I said twice before).

> These are for display, not production. We do downgrade all
> moz-classes.
You're right, I stand corrected:
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/compose/src/nsMsgCompose.cpp#5430
No, I'm not going senile, I (or Aceman?) wrote that, I just thought the <b> and <i> were checked before.
(In reply to Jorg K (GMT+2) from comment #40)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #39)
> > Let's just take the plaintext as-is, but in a flowed style with bock quotes.
> Still not easy, the tricks need to be copied from the MIME code (as I said
> twice before).
> 
> > These are for display, not production. We do downgrade all
> > moz-classes.
> You're right, I stand corrected:
> https://dxr.mozilla.org/comm-central/rev/
> d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/compose/src/nsMsgCompose.
> cpp#5430
> No, I'm not going senile, I (or Aceman?) wrote that, I just thought the <b>
> and <i> were checked before.

Moz-classes are downgraded, <b> & friends are not. The point being, even if we use moz-class to show *bold* as actual bold print in HTML, from a conceptual perspective, we can't downgrade that, because downgrading formatting which is visible in composition would violate ux-wysiwyg. It would become totally unpredictable which visible formatting survives and which doesn't.
In short, please let's not meddle with structs, let's just show the plaintext message as-is in an HTML composition, but in a flowed style, without hard linebreaks. So I think we want <blockquote>s, and possibly <p>s. Just that. No bells and whistles trying to convert structs into HTML, then we get stuck because there's visible formatting which we can't downgrade per ux-wysiwyg, but we'd actually want to downgrade an unchanged plaintext message. I think.

Unless if we think our struct conversion into HTML totally rocks and we should assist the use with his HTML formatting, (accepting that even an unchanged plaintext original will not be downgraded again after we converted it to HTML), but I doubt that. Structs conversion is another one of those things which is very hard to ever get right. No machine can tell if maybe *v* has another meaning, and * must be preserved. What if *v* is an ASCII art for a bird? So we can't just rip out and convert structs, we could bold print formatting but the stars *...* must remain to avoid dataloss of text. Which doesn't help much because you'd still have to go through your message and remove all the structs. Most other things I could think of, like bulleted lists and so on - it's just not worth the hassle, and even higher risk to get it wrong. Because again we wouldn't know:

 * 1 star if you buy x
 * 1 star if you buy y
** 2 stars if you buy xx
** 2 stars if you buy yy

Machine structs conversion would convert the first two lines into bulleted list, resulting in dataloss of text content.
Comment on attachment 8896404 [details] [diff] [review]
731688-edit-as-new.patch (v4) - WIP

OK, this didn't cut it, so we might want to go with the other patch and leave better plain text to HTML conversion to another bug.
Attachment #8896404 - Attachment is obsolete: true
Attachment #8896404 - Flags: feedback?(acelists)
Blocks: 1389771
Refreshed.
Attachment #8895053 - Attachment is obsolete: true
This should do it.

For "Edit as New Message" we get the account format or the opposite account format if used with shift.

For "Edit Draft" and "New Message from Template" we get the original message format or the opposite message format if used with shift.

The text to HTML routine needs improvement, but let's do this in another bug and not halt this bug here for that reason. At least the user can get a compose window of the desired type and then edit its content accordingly.
Attachment #8895783 - Attachment is obsolete: true
Attachment #8895783 - Flags: review?(acelists)
Attachment #8899166 - Flags: review?(acelists)
Oh, the shift-click on the "Edit" button in the notification bar doesn't seem to work, so I'm happy to be advised on that.
Comment on attachment 8899164 [details] [diff] [review]
731688-edit-as-new-compose-type.patch - Part 1 by Kent James

I've checked that Kent covered all the places where nsIMsgCompType::Template or nsIMsgCompType.Template was used and added the appropriate code.

It looks good to me and works in conjunction with Part 2. Certainly before landing, we run the test suite on in to see whether there is anything we overlooked.
Attachment #8899164 - Flags: review+
Comment on attachment 8899166 [details] [diff] [review]
731688-edit-as-new.patch - Part 2 (v5)

Review of attachment 8899166 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting :) With e.g. a HTML template and HTML composition turned off, using the template produces HTML message. Edit as new on that template produces plain text message. But in all cases, I could get the opposite format with holding Shift. So it seems this is what is wanted here.
I tried normal message, draft and template, using the context menu and also the Message menu.

Yes, the "Edit" button on the notification in a displayed draft does not handle the Shift and does not toggle the format.
Attachment #8899166 - Flags: review?(acelists) → review+
Removed a hunk which isn't necessary here.
Attachment #8899164 - Attachment is obsolete: true
Attachment #8899223 - Flags: review+
(In reply to :aceman from comment #49)
> Yes, the "Edit" button on the notification in a displayed draft does not
> handle the Shift and does not toggle the format.
I've done a bit of digging around
https://dxr.mozilla.org/comm-central/rev/7b75c2e0b9deb590c18aaddb2a6168146cde8ca4/mail/base/content/mailWindowOverlay.js#3182
where the callback for the button click is defined.

Looks like the callback is called from here:
https://dxr.mozilla.org/mozilla-central/rev/833f84d0d5c729054a3aa8b3f34735f56fe6436b/toolkit/content/widgets/notification.xml#491
var result = callback(this, button, aEvent.target);
sadly not passing the full event. We need aEvent.shiftKey.

So fixing the <shift>-button-click needs to be done in another bug.

Meanwhile, let's do a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9fff8ad405762c95e9a616f0e6c33474b5645d0c
Blocks: 1392051
Blocks: 1392052
Blocks: 1392053
Blocks: 1392054
Blocks: 1392055
Blocks: 1392056
Filed follow-up bugs:
Bug 1392051 - Write test for bug 731688: Shift modifier for "Edit draft", "Edit as new", "New message from template"
Bug 1392052 - Improve text to HTML conversion when shift modifier is used
Bug 1392055 - Pass the full event or at least event.shiftKey to the callback in toolkit/content/widgets/notification.xml
Bug 1392056 - Fix shift-click on "Edit" button in drafts folder
Try is green, so this can be landed (as soon as I fix the bustage in bug 1392057).
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ace64c7317e8
Introduce compose type EditAsNew. r=jorgk
https://hg.mozilla.org/comm-central/rev/0998375b22a0
Support conversion from HTML to plain text and vice versa in 'Edit as New Message', 'Edit Draft' and 'New Message from Template'. r=aceman DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Landed that without the print statements ;-)

Plenty of follow-up work.
Target Milestone: --- → Thunderbird 57.0
No longer blocks: 1392055
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: