Closed Bug 1582410 Opened 5 years ago Closed 4 years ago

message compose should use document.execCommand for editing commands

Categories

(Thunderbird :: Message Compose Window, task, P1)

Tracking

(thunderbird78 fixed)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird78 --- fixed

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(1 file, 7 obsolete files)

The message compose window editing commands aren't doing things in a very webby way a the moment. Things like cmd_fontFace do a funny little roundtrip through c++, when all that really should be needed is something like editor.document.execCommand

We should convert these over to document.execCommand commands.
See https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand and also https://codepen.io/chrisdavidmills/full/gzYjag/

doStatefulCommand is also now called for every keypress in the editor (due to observing document changed I think), which can't be very great for performance.

Assignee: nobody → khushil324

I have added many comments so everyone will get the idea about how this file is working.

Attachment #9150896 - Flags: review?(mkmelin+mozilla)

A small change.

Attachment #9150896 - Attachment is obsolete: true
Attachment #9150896 - Flags: review?(mkmelin+mozilla)
Attachment #9150899 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9150899 [details] [diff] [review]
Bug-1582410_document-execCommand-1.patch

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

::: mail/components/compose/content/ComposerCommands.js
@@ +7,5 @@
> + * Implementations of nsIControllerCommand for composer commands. These commands
> + * are related to editing. You can fire these commands with following functions:
> + * goDoCommand and goDoCommandParams(If command requires any parameters).
> + * Sometimes, we want to reflect the changes in the UI also. We have two functions
> + * for that: pokeStyleUI and pokeMultiStateUI. pokeStyleUI function is for those

The popeStyleUI

@@ +8,5 @@
> + * are related to editing. You can fire these commands with following functions:
> + * goDoCommand and goDoCommandParams(If command requires any parameters).
> + * Sometimes, we want to reflect the changes in the UI also. We have two functions
> + * for that: pokeStyleUI and pokeMultiStateUI. pokeStyleUI function is for those
> + * commands which are bollean in nature for example "cmd_bold" command, text can

boolean

@@ +9,5 @@
> + * goDoCommand and goDoCommandParams(If command requires any parameters).
> + * Sometimes, we want to reflect the changes in the UI also. We have two functions
> + * for that: pokeStyleUI and pokeMultiStateUI. pokeStyleUI function is for those
> + * commands which are bollean in nature for example "cmd_bold" command, text can
> + * be bold or not. pokeMultiStateUI function is for the commands who can have

The

Please also divide into paragraphs a bit to make it easier to read.

@@ +13,5 @@
> + * be bold or not. pokeMultiStateUI function is for the commands who can have
> + * multiple values for example "cmd_fontFace" can have different values like
> + * arial, variable width etc. Here, some of the commands are getting executed
> + * by document.execCommand. Those are listed in documentCmd object. In that
> + * also, some commands are of type bolean and some are of multiple states.

boolean
state.

@@ +18,5 @@
> + * We have two functions to execute them: doStatefulCommand and
> + * doStyleUICommand. All commands are not executable through
> + * document.execCommand. In all those cases, we will use goDoCommand or
> + * goDoCommandParams. goDoCommandParams is implemented in this file.
> + * goDoCommand is coming from globalOverlay.js file in Mozilla Central.

I think "goDoCOmmand is from globalOverlay.js" is enough.

@@ +20,5 @@
> + * document.execCommand. In all those cases, we will use goDoCommand or
> + * goDoCommandParams. goDoCommandParams is implemented in this file.
> + * goDoCommand is coming from globalOverlay.js file in Mozilla Central.
> + * Commands which can be executed by document.execCommand,
> + * we will give priority to doStatefulCommand and doStyleUICommand there.

Can you clarify this sentence?

@@ +35,5 @@
>  
>  var gComposerJSCommandControllerID = 0;
>  
> +/**
> + * It is used to register commands we have created manually.

s/It/SetupHTMLEditorCommands/

@@ +150,5 @@
> +/**
> + * This function is used to register the command controller in the
> + * editor document.
> + *
> + * @returns {nsIControllerCommandTable|null} - It will be used to

A controller used to register .....

@@ +190,5 @@
>  
>  /* eslint-disable complexity */
> +
> +/**
> + * It will get the state of the given command and call the pokeStyleUI

Please don't use "It". 

"Get the state of...."

@@ +192,5 @@
> +
> +/**
> + * It will get the state of the given command and call the pokeStyleUI
> + * or pokeMultiStateUI according to the type of the command to reflect
> + * the UI changes in the edittor.

editor

@@ +194,5 @@
> + * It will get the state of the given command and call the pokeStyleUI
> + * or pokeMultiStateUI according to the type of the command to reflect
> + * the UI changes in the edittor.
> + * It does a C++ trip to get the current state of the command. We want
> + * to remove that and start doing things through Javascript.

True, but please remove the comment.

@@ +270,5 @@
> + * goUpdateComposerMenuItems. For any commandset events get fired,
> + * this function will be called. This function is used to update
> + * the UI state of the editor buttons and menulist. Whenever you change
> + * your selection in the editor part i.e. if you move your cursor,
> + * you will find this functions getting called and updating the ediot UI

editor

@@ +293,5 @@
>    }
>  }
>  
> +/**
> + * Directly called the commands with the multiple state attribute

Execute the command with the provided parameters.

This is directly calling commands with multiple state attributes, which ware not supported by
document.execCommand()

@@ +361,5 @@
>  
> +/**
> + * All the commands which can be excecuted by the document.execCommand().
> + */
> +let documentCmd = {

can we make this 

/** Maps internal command names to their document.execCommand() command string. */
var gCommandMap = new Map();

@@ +390,5 @@
> +  top.document
> +    .querySelector("editor")
> +    .contentDocument.execCommand(documentCmd[cmdStr], false, null);
> +  let commandNode = top.document.getElementById(cmdStr);
> +  let newState = "true" != commandNode.getAttribute("state");

comparing the other way is more common

@@ +454,5 @@
>    }
>  }
>  
> +/**
> + * It is used for the multiple states type commands available through

Used for...

@@ +458,5 @@
> + * It is used for the multiple states type commands available through
> + * document.execCommand().
> + * We are not using pokeMultiStateUI for updating the UI because it
> + * required command parameters through newCommandParams function. It
> + * will do a C++ trip and we want to avoid that.

This comment can be removed. Please consider future readers (where it will hopefully be all gone).

@@ +463,5 @@
> + *
> + * @param {string} commandID - The id of the command.
> + * @param {string} newState - The parameter value.
> + * @param {boolean} updateUI - updates the UI if true. Used when
> + * function is called in another Javascript function.

JavaScript

Please also for line wrapping indent two spaces (from @ above)

::: mail/components/compose/content/messengercompose.xhtml
@@ +1393,5 @@
>                          type="radio"
>                          name="1"
>                          label="&bodyTextCmd.label;"
>                          accesskey="&bodyTextCmd.accesskey;"
> +                        value="div"

what is this change about?
Attachment #9150899 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #4)

what is this change about?

This is equivalent to "body Text". It will insert Text directly into <div> container.
This patch is solving the issue of default Paragraph selection.
Also, if you click on the A+ or A-, previously, it was not getting disabled when reached "Tiny" or "Huge" until you select the editor. This issue is also resolved with this patch.

Attachment #9150899 - Attachment is obsolete: true
Attachment #9151114 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9151114 [details] [diff] [review]
Bug-1582410_document-execCommand-2.patch

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

It seems to work for the most part. However, it looks like something is inserted into the message body so you now have two paragraphs (or something) in there to start with. Please fix that.

::: mail/components/compose/content/ComposerCommands.js
@@ +10,5 @@
> + *
> + * Sometimes, we want to reflect the changes in the UI also. We have two functions
> + * for that: pokeStyleUI and pokeMultiStateUI. The pokeStyleUI function is for those
> + * commands which are boolean in nature for example "cmd_bold" command, text can
> + * be bold or not. The pokeMultiStateUI function is for the commands who can have

which (not who)

@@ +15,5 @@
> + * multiple values for example "cmd_fontFace" can have different values like
> + * arial, variable width etc.
> + *
> + * Here, some of the commands are getting executed by document.execCommand.
> + * Those are listed in gCommandMap Map object. In that also, some commands are of

in the

@@ +154,5 @@
> +/**
> + * Used to register the command controller in the editor document.
> + *
> + * @returns {nsIControllerCommandTable|null} - A controller used to
> + * register the manually created commands.

please indent the second comment row by two spaces more
Attachment #9151114 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #7)

It seems to work for the most part. However, it looks like something is
inserted into the message body so you now have two paragraphs (or something)
in there to start with. Please fix that.

It is coming from here: https://searchfox.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#529,581,628
I don't know what is the purpose of these functions. What do we want to do with them now?

The purpose there is to either start out in paragraph mode, or in body, depending on pref. If you use paragraph mode you'll write in <p>s. Otherwise it's just text directly in <body>.

But why do we get more than one with the patch?

Here, we are adding <p> and <br> and then we start with the paragraph in the 3rd line. I not sure why we are adding this <p> and <br>. We can remove them and still start with paragraph now.
https://searchfox.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#567-569

I think now, this is not necessary. I will confirm this and update the patch.

Yes, as you pointed out, https://searchfox.org/comm-central/rev/566c658a079a9601219ed66f049299fa1bd8b0be/mail/components/compose/content/MsgComposeCommands.js#567-569 inserts a <p><br></p> since otherwise the cmd_paragraphState two lines below doesn't work.

Before you make changes in this area, can you please fix bug 1625218 first. With the paragraph mode indicator broken, no one can see what the consequence of any further changes will be.

What is this about: document.getElementById("cmd_paragraphState").setAttribute("state", "div");

That doesn't look right. During compose, we use "Paragraph" (<p>) or "Body Text" (no block at all, not even <div>) and we try to avoid <div> as much as we can, although it happens if people start typing into the <div class="moz-cite-prefix">, for example.

This patch seemed to fix bug 1625218 in the process.

(In reply to Jorg K (CEST = GMT+2) from comment #12)

Yes, as you pointed out, https://searchfox.org/comm-central/rev/566c658a079a9601219ed66f049299fa1bd8b0be/mail/components/compose/content/MsgComposeCommands.js#567-569 inserts a <p><br></p> since otherwise the cmd_paragraphState two lines below doesn't work.

Before you make changes in this area, can you please fix bug 1625218 first. With the paragraph mode indicator broken, no one can see what the consequence of any further changes will be.

This patch is solving that problem also. Previously, we were not informing the C++ code to update the editor controller after setting up the state on the command node. I have added the comments in the patch so one can look at how things are working with commands.

About the <div> part: we are using switching to use document.execCommand for "cmd_paragraphState". Through "cmd_paragraphState', we are not able to insert the text directly into the body but we can insert the text into the <div> through "div" param.

I tried removing those extra <p> and <br> and it worked. Submitting the patch in a few minutes.

Attachment #9151114 - Attachment is obsolete: true
Attachment #9151297 - Flags: review?(mkmelin+mozilla)

Missing commit message added.

Attachment #9151297 - Attachment is obsolete: true
Attachment #9151297 - Flags: review?(mkmelin+mozilla)
Attachment #9151298 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9151298 [details] [diff] [review]
Bug-1582410_document-execCommand-4.patch

I think I'll need to review this. I'm convinced that we don't want this: `document.getElementById("cmd_paragraphState").setAttribute("state", "div");`. What attribute states are possible? Why does "" not work any more? Can you set "br"?

BTW, have you tried all these test cases?
- Compose in paragraph or body text mode with and without signature (4 test cases)
- Switch identity and make sure the signature is properly replaced (2 test cases)
- Reply with below the quote with and without signature. Then switch identity. (4 test cases)
- Reply above the quote with without and with signature, both above and below the quote. Then switch identity. (6 test cases).
You might also try switching between identities were one has a signature and another one doesn't.

EDIT: I forgot to say: You also need to test forwarding, there you can also add signatures and switch identities.
Attachment #9151298 - Flags: review?(jorgk-bmo)
Comment on attachment 9151298 [details] [diff] [review]
Bug-1582410_document-execCommand-4.patch

I did one test: Start a compose window with a plaintext signature. That already doesn't work :-(
Attachment #9151298 - Flags: review?(jorgk-bmo) → review-

Oh, do a try run. Signatures and switching identities aren't well covered, but there are some tests.

Comment on attachment 9151298 [details] [diff] [review]
Bug-1582410_document-execCommand-4.patch

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

I looked at the patch some more now. Why are we switching "body text" editing to "div" everywhere? That's not desired. Who decided to abandon "traditional" <br> line breaks and introduce <div> blocks? You're opening a total can of worms with that.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +564,4 @@
>      } else {
> +      document
> +        .getElementById("cmd_paragraphState")
> +        .setAttribute("state", "div");

Here, and a few times further down.

::: mail/components/compose/content/editFormatButtons.inc.xhtml
@@ +11,5 @@
>                      crop="right"
>                      tooltiptext="&ParagraphSelect.tooltip;"
>                      observes="cmd_renderedHTMLEnabler">
>              <menupopup id="ParagraphPopup">
> +              <menuitem id="toolbarmenu_bodyText" label="&bodyTextCmd.label;" value="div"/>

Here.

::: mail/components/compose/content/editor.js
@@ +434,5 @@
>  
>    // force match with "normal"
> +  if (state == "body" || state == "x" || state == "") {
> +    state = "div";
> +    commandNode.setAttribute("state", "div");

Here.

::: mail/components/compose/content/messengercompose.xhtml
@@ +1393,5 @@
>                          type="radio"
>                          name="1"
>                          label="&bodyTextCmd.label;"
>                          accesskey="&bodyTextCmd.accesskey;"
> +                        value="div"

Here.
Attachment #9151298 - Flags: review?(mkmelin+mozilla)

Sure. For FF editing. That started in FF 60 or earlier, but we never followed it, so why now, especially a week before the code goes to TB 78 (EDIT: 68->78) beta and then ESR? Has someone looked at the potential consequences in TB(*)? As far as I read the bug, you want to "modernise" some editing commands, fair enough. So why change the "body text" make up? As you can see from the changeset quoted in comment #18, the slightest change in this area has far reaching effects. Coincidentally the patch destroys composition with signatures and has been presented for review, apparently without any testing of real life cases :-(

BTW, where is the "funny little roundtrip through c++"?

Just a small list of possible consequences:

  • What happens during HTML->plaintext conversion? How do the <div>m convert?
  • How do the divs look in a reply, or plaintext reply to a HTML mail?
  • What does out test suite say? No of the tests expects <div>, they all expect <br>.
  • Are there any add-ons which will break now? QuickText?
  • How does "Body text" to "Paragraph" conversion work if <div> is used instead of <br>?
  • (That list is just a three minute brainstorming result, I'm sure there is more).
    Keep in mind that composition has always happened with <br> for decades, and at some stage we switched the default to <p>.

Those would all have to be checked/investigated. The why is simple though: we're using something that's completely non-standard: a dead end internal technology in an area mozilla central are working on changing. That's a recipe for disaster to keep using. If we can, there are only upsides to dropping that.

Assuming the issues can be resolved and we can use the standard way chances of breakages will be much less, plus we can give the m-c people a good test case for an editor.

Why don't you put the paragraph/div/br stuff into a new bug instead of mixing it in here and do it after TB 78.

(In reply to Jorg K (CEST = GMT+2) from comment #22)

I looked at the patch some more now. Why are we switching "body text"
editing to "div" everywhere? That's not desired. Who decided to abandon
"traditional" <br> line breaks and introduce <div> blocks? You're opening a
total can of worms with that.

We are using <br> everywhere for line breaking.
With this patch.

Previously:

<body>
Hello!
<br>
How are you?
<br>
Where are you nowdays?
<p>I am fine.</p>
</body>

Now:

<body>
<div>
  Hello!
  <br>
  How are you?
  <br>
  Where are you nowdays?
</div>
<p>I am fine.</p>
</body>

You can't insert a text directly into the body through "formatBlock". So we are inserting it through <div> which is the only option available and I think it does the same thing, inserting the text into the HTML body without using any tags related to formatting. I didn't find any useful resources other than this one: https://javascript.programmingpedia.net/en/tutorial/1613/execcommand-and-contenteditable

There was an issue with the current patch regarding the start. I had removed a few of the necessary lines from NotifyComposeBodyReadyForwardInline, NotifyComposeBodyReadyReply, NotifyComposeBodyReadyNew. I have made a few changes and it's working for all the cases you have mentioned. I have also sent it for a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6ac436f29e394ce3d86a6521661047205018b46d&selectedTaskRun=aDmivSXfShKSlEGZddctAQ-0

All other test cases are passing. I have made changes in browser_ext_compose_begin.js as now we are not adding any line before starting. Now if the compose window opens, no line gets added in the editor part(When there is no signature). But when you select the editor part, <p> or <div> gets added according to your preference. Magnus, can you also test all the edge cases?

Attachment #9151298 - Attachment is obsolete: true
Attachment #9151659 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9151659 [details] [diff] [review]
Bug-1582410_document-execCommand-5.patch

Have you tested what I mentioned in comment #19 and comment #24? MsgComposeCommands.js  is unchanged to the previous version, so I assume signatures still don't work. Also, if you really want to switch to <div> in this bug (although I suggested to do it elsewhere), you need to change here:
https://searchfox.org/comm-central/rev/835caa88371d68ca95a4d77108435dc5c174add2/mail/components/compose/content/MsgComposeCommands.js#8218
Attachment #9151659 - Flags: review?(mkmelin+mozilla) → review-

Oh, and what you have in comment #27 is unacceptable since you ended up with a mix of <div> and <br>.

<body>
<div>
  Hello!
  <br>
  How are you?
  <br>
  Where are you nowdays?
</div>
<p>I am fine.</p>
</body>

Apparently the brave new world that Magnus wants would be:

<body>
<div>Hello!</div>
<div>How are you?</div>
<div>Where are you nowdays?</div>
<p>I am fine.</p>
</body>

Hm, testing the patch, I don't actually get the <div> in there.

<body text="#000000" bgcolor="#FFFFFF">
    row1<br>
    row2<br>
    <p>and a paragraph<br>
    </p>
  </body>

(In reply to Jorg K (CEST = GMT+2) from comment #28)

Have you tested what I mentioned in comment #19 and comment #24?
MsgComposeCommands.js is unchanged to the previous version, so I assume
signatures still don't work. Also, if you really want to switch to <div> in
this bug (although I suggested to do it elsewhere), you need to change here:
https://searchfox.org/comm-central/rev/
835caa88371d68ca95a4d77108435dc5c174add2/mail/components/compose/content/
MsgComposeCommands.js#8218

Yes, I have tested it. Changes needed to be made in NotifyComposeBodyReadyForwardInline, NotifyComposeBodyReadyReply, NotifyComposeBodyReadyNew functions. Plus, all the test cases are passing and we have many test cases covering different scenarios in the message compose window.

(In reply to Magnus Melin [:mkmelin] from comment #30)

Hm, testing the patch, I don't actually get the <div> in there.

I also tried this. For the first insertion of body-text default results in this. We need to insert div elements first for those cases.

<body text="#000000" bgcolor="#FFFFFF">
  row1<br> // body text default so direct inserting it into html. Need to have a <div> element in the starting for consistency. We were previouly doing this for <p> element.
  row2<br> // body text default so direct inserting it into html.
  <p>and a paragraph<br>
  </p>
  <div> // Changing state from Paragraph to bodytext will now create <div> element.
    Hello<br>
    Hello<br> 
  </div>
</body>

Plaintext signature, paragraph composition, check resulting HTML and location of sig. separator.

(In reply to Jorg K (CEST = GMT+2) from comment #29)

Oh, and what you have in comment #27 is unacceptable since you ended up with a mix of <div> and <br>.

Why this? All the browsers based editor will be following this. Shouldn't we also follow this? It is not changing anything. It's just wrapping a <div> container on the directly inserted elements into the body. It's also a good way to identify body text now as you just need to find <div> nodes.

And we are using <div> for signature also in HTML mode. We are doing the same there:

<div>
  ---
  <br>
  Thanks and Regards, Khushil
</div>

Can't we use this way at all other places too?

(In reply to Jorg K (CEST = GMT+2) from comment #33)

Plaintext signature, paragraph composition, check resulting HTML and location of sig. separator.

It's the same as ESR 68.

Comment on attachment 9151659 [details] [diff] [review]
Bug-1582410_document-execCommand-5.patch

OK, sorry, v5 is slightly different to v4 and working better. I'll check it in detail later. I still think what Magnus wants is what I have at the end of comment #29.
Attachment #9151659 - Flags: review- → review?(mkmelin+mozilla)
Comment on attachment 9151659 [details] [diff] [review]
Bug-1582410_document-execCommand-5.patch

Apologies again, I played around with v5 a bit more and it's working with signatures, reply and forward. However. for "Body Text", you get a mix of <div> and <br> behaviour. Some examples: Switch to non-paragraph mode (in options) and start a new composition, type two lines. Result:
```
  <body>
    line 1<br>
    line 2<br>
  </body>
```
Now select all, change to paragraph, and back to body text. Result:
```
  <body>
    <div>line 1</div>
    <div>line 2</div>
  </body>
```
And adding a line in between, I get:
```
  <body>
    <div>line 1<br>
      line 1a<br>
    </div>
    <div>line 2</div>
```
So I suggest to remain at "all <br>" or switch "all <div>". In the latter case, you need to change
https://searchfox.org/comm-central/rev/835caa88371d68ca95a4d77108435dc5c174add2/mail/components/compose/content/MsgComposeCommands.js#8218
Attachment #9151659 - Flags: feedback-

Oh, and what you have in comment #27 is unacceptable since you ended up with a mix of <div> and <br>.

Why this? All the browsers based editor will be following this. Shouldn't we also follow this? It is not changing anything. It's just wrapping a <div> container on the directly inserted elements into the body. It's also a good way to identify body text now as you just need to find <div> nodes.

Just repeating: Either the old state of "all <br>" or a new state of "all <div>", not a mix. "a good way to identify body text now as you just need to find <div> nodes", clearly not since you get a mix.

(In reply to Jorg K (CEST = GMT+2) from comment #37)
At the starting case:

  <body>
    line 1<br>
    line 2<br>
  </body>

This is a problem, doing these changes in the patch 5:
From document.getElementById("cmd_paragraphState").setAttribute("state", "div"); to doStatefulCommand("cmd_paragraphState", "div", true);

Result with the changes:

  <body>
    <div>
      line 1<br>
      line 2<br>
   </div>
  </body>

Removed the "formatblock" part right now.
I will open a follow-up bug. I am targeting to do:

<body>
  <div>Hello</div>
  <div>How are you?</div>
  <p>I am good.</p>
</body>

But this is for the another bug.
A try-run for the current patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedTaskRun=APuSiw7CTPaRlPc102MZXw-0&revision=9ab061f1387f3478dbc29170be21bd6fdd2114d3

Attachment #9151659 - Attachment is obsolete: true
Attachment #9151659 - Flags: review?(mkmelin+mozilla)

(In reply to Khushil Mistry [:khushil324] from comment #40)

I will open a follow-up bug. I am targeting to do:

Excellent. Who suggested that ;-)

Comment on attachment 9152536 [details] [diff] [review]
Bug-1582410_document-execCommand-6.patch

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

BTW, does this still fix bug 1625218?

::: mail/components/compose/content/ComposerCommands.js
@@ +373,5 @@
> +  ["cmd_ol", "InsertOrderedList"],
> +  ["cmd_fontFace", "fontName"],
> +  // This are currently implemented with the help of
> +  // color selection dialog box in the editor.js.
> +  // ["cmd_paragraphState", "formatBlock"],

That doesn't fit under the preceding comment.

(In reply to Jorg K (CEST = GMT+2) from comment #42)

BTW, does this still fix bug 1625218?

Yes.

(I think you intended to submit an updated patch?)

Priority: P2 → P1

Yes, I will submit a patch for cmd_paragraphState soon. All other chnages are in this patch. For cmd_paragraphState, I will open a new bug.

Attachment #9152536 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9152536 [details] [diff] [review]
Bug-1582410_document-execCommand-6.patch

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

Looks good. I'll attach an updated unbitrotted patch with the few changes.

::: mail/components/compose/content/ComposerCommands.js
@@ +273,5 @@
> + * your cursor, you will find this functions getting called and
> + * updating the editor UI of toolbarbuttons and menulists. This is mainly
> + * to update the UI according to your selection in the editor part.
> + *
> + * @param {Object} commandset - The XUL commandset object.

{XULElement} commandset - The <xul:commandset> element to update for.

@@ +282,5 @@
>      var commandNode = commandset.children[i];
>      var commandID = commandNode.id;
>      if (commandID) {
> +      // enable or disable. Check isCommandEnabled in any manually
> +      // created command to get the idea.

I don't understand this comment.
Attachment #9152536 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9152536 - Attachment is obsolete: true
Attachment #9155613 - Flags: review+
Target Milestone: --- → Thunderbird 79.0

(In reply to Magnus Melin [:mkmelin] from comment #46)

I don't understand this comment.

Fix/clarify/remove the comment before landing?

Blocks: 1625218

(In reply to Jorg K (CEST = GMT+2) from comment #48)

(In reply to Magnus Melin [:mkmelin] from comment #46)

I don't understand this comment.

Fix/clarify/remove the comment before landing?

We can enable or disable the command in the editor. If the command is disabled, we can not execute it. For manually created command, we put this logic in the "isCommandEnabled" function contains this logic. Magnus has updated the comment in the final patch.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b5234d4b31c3
message compose should use document.execCommand for editing commands. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 9155613 [details] [diff] [review]
Bug-1582410_document-execCommand.patch

[Approval Request Comment]
User impact if declined: Paragraph mode selector broken (this bug fixes bug 1625218 which broke for unknown reasons)
Risk to taking this patch (and alternatives if risky): not risky
Attachment #9155613 - Flags: approval-comm-beta?
Comment on attachment 9155613 [details] [diff] [review]
Bug-1582410_document-execCommand.patch

Approved for beta.  

(Not sure I would say "not risky". If you mean it shouldn't be risky given it's a conversion, I agree - one for one code replacement. But it is bigish and subject to error.  That said, I find no regressions reported so far although only been on nightly ~3 days)
Attachment #9155613 - Flags: approval-comm-beta? → approval-comm-beta+

Khushil, can you file a bug to handle more of the commands to using document.execCommand? I think there were more that could be handled, right?

Flags: needinfo?(khushil324)

Yes, sure.

Flags: needinfo?(khushil324)
Regressions: 1653545
See Also: → 1655014
Regressions: 1655279
Regressions: 1658156
Regressions: 1658908
Regressions: 1658999
Regressions: 1659067
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: