1655014 - Convert more editing commands to use document.execCommand

Status: NEW

(Thunderbird :: Message Compose Window, task)

Comment 1

[Khushil Mistry [:khushil324]]

In Bug 1582410, we converted a few of the editing commands to make it use document.execCommand. But few of them were left and were causing major problems and we were reaching the deadline of the ESR 78 release. Following commands can be converted to document.execCommand and are left:

cmd_paragraphState
cmd_highlight
cmd_fontColor
fontSize

Check this: https://searchfox.org/comm-central/search?q=EditorSetTextProperty&path= (for font size, font color and background color).

Comment 2

[Magnus Melin [:mkmelin]]

And <tt>, see bug 1653545. Or maybe that should have a bug of it's own.

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1655014_part-1_convert-font-size-command-0.patch

Check the UX when we select the lines containing text with different font size. Previously, we were disabling the Increase/Decrease buttons and none of the menuitem in size menu popup was selected. Now queryCommandValue returns the fontSize of the 1st character when we select any line. So menuitems and Increase/Decrease buttons are set according to that font size. We can achieve the previous behaviour but we need to use getInlinePropertyWithAttrValue API which will be the same as what we were doing previosuly i.e. we are not modernising anything.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9185993 [details] [diff] [review]
Bug-1655014_part-1_convert-font-size-command-0.patch

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

Looks pretty good, but some cleanup could be needed

::: mail/base/modules/MailMigrator.jsm
@@ +517,4 @@
>          this._migrateSMTPToOAuth2("smtp.aol.com");
>        }
>  
> +      if (currentUIVersion < 24) {

Add documentation here about what the migration does

::: mail/components/compose/content/MsgComposeCommands.js
@@ +8163,1 @@
>    if (fontSize) {

we always get a value...

::: mail/components/compose/content/editFormatButtons.inc.xhtml
@@ +87,3 @@
>              <menuitem id="toobarmenu_fontSize_x-small"
>                        label="&size-tinyCmd.label;"
>                        type="radio" name="1"

seems strange to have a name="1". Maybe while we're here, clean this up to be 
name="fontSize" 

Also add value="1" - value="6", and move the oncommand to the menupopup which can check event.target.value

::: mail/components/compose/content/editor.js
@@ +824,3 @@
>   * Helper function
>   */
>  function getFontSizeIndex() {

I'd suggest changing the name to 
getLegacyFontSize(), and add a @see https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#legacy-font-size-for

It will not return "bad" values, like 0, so the callers should also be cleaned up

Also, you always need it as a string (with my suggested changes), so just return  the string

@@ +842,1 @@
>      if (setIndex >= 0) {

would be nicer to loop the children and check value (per above)

::: mail/components/compose/content/messengercompose.xhtml
@@ +1239,4 @@
>                <menuitem id="menu_size-x-small"
>                          label="&size-tinyCmd.label;"
>                          accesskey="&size-tinyCmd.accesskey;"
> +                        oncommand="EditorSetFontSize('1')"

same for these

Comment 5

[Khushil Mistry [:khushil324]]

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

I'd suggest changing the name to
getLegacyFontSize(), and add a @see
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#legacy-font-size-for

It will not return "bad" values, like 0, so the callers should also be
cleaned up

Also, you always need it as a string (with my suggested changes), so just
return the string

We are using this function in nsIncreaseFontCommand and nsDecreaseFontCommand where we need it as an integer to compare if it's between 1 and 6. I suggest to keep as a function that returns the integer value.

Comment 6

[Magnus Melin [:mkmelin]]

Well we could parse it where needed there, I think.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1655014_part-1_convert-font-size-command-1.patch

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=93cbfa1e3e1ee03e03079c200954c69b49c198c4

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9186214 [details] [diff] [review]
Bug-1655014_part-1_convert-font-size-command-1.patch

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

The try has some orange from this, likely comm/mail/test/browser/cloudfile/browser_attachmentUrls.js and  comm/mail/test/browser/composition/browser_forwardHeaders.js at least

::: mail/components/compose/content/editFormatButtons.inc.xhtml
@@ +83,5 @@
>                         type="menu"
>                         observes="cmd_renderedHTMLEnabler">
>            <menupopup id="AbsoluteFontSizeButtonPopup"
> +                     onpopupshowing="initFontSizeMenu(this);"
> +                     oncommand="EditorSetFontSize(event.target.value)">

Please prefer just passing in the event.
In the future inline command handling should go away as much as we can. The replacement, of hooking up event listeners would require things to be focused around the event.

::: mail/components/compose/content/editor.js
@@ +664,5 @@
>  function EditorSetFontSize(size) {
> +  // Increase Font Menuitem and Decrease Font Menuitem from the main menu
> +  // will call this function becuase of oncommand attribute on the menupopup
> +  // and size will be null for such function calls.
> +  if (!size) {

By passing in the event, this gets much clearer and we don't end up with a function that doesn't set size when it was told to set size...

@@ +836,5 @@
>   */
> +function getLegacyFontSize() {
> +  let fontSize = GetCurrentEditor().document.queryCommandValue("fontSize");
> +  // If one selects all the texts in the editor and deletes it, editor sets fontSize
> +  // value to null. We will set it to default value then.

.... "the editor will return null fontSize" I suppose? Not set it.

@@ +839,5 @@
> +  // If one selects all the texts in the editor and deletes it, editor sets fontSize
> +  // value to null. We will set it to default value then.
> +  if (!fontSize) {
> +    fontSize = Services.prefs.getCharPref("msgcompose.font_size", "3");
> +    EditorSetFontSize(fontSize);

Do we need to set it here? It's basically a getter method so this is unexpected

Comment 9

[Khushil Mistry [:khushil324]]

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

Please prefer just passing in the event.
In the future inline command handling should go away as much as we can. The
replacement, of hooking up event listeners would require things to be
focused around the event.

We are using EditorSetFontSize function directly at some so places so we should pass only the size parameter.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1655014_part-1_convert-font-size-command-2.patch

Comment 11

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=b7e35f5c760701213564b25e5bf573f921b33d9d

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9192438 [details] [diff] [review]
Bug-1655014_part-1_convert-font-size-command-2.patch

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

Unfortunately still test failures

Comment 13

[Khushil Mistry [:khushil324]]

There are three test failures.
I am not able to reproduce locally these two:
comm/mail/components/extensions/test/browser/browser_ext_compose_begin.js
comm/mail/components/extensions/test/browser/browser_ext_compose_onBeforeSend.js

comm/mail/test/browser/folder-widget/browser_messageFilters.js (This test is failing due to patch from bug 244347)

Comment 14

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1655014 - Convert fontSize command to use document.execCommand. r=mkmelin

Comment 15

[Henry Wilkes (they/them) [:henry-x]]

Why are we switching to document.execCommand when it is listed as deprecated? https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

Is the idea that this is deprecated as a cross browser feature, but will remain reliably supported within firefox?

Comment 16

[Magnus Melin [:mkmelin]]

document.execCommand is deprecated, but there is no alternative yet... so everyone is using it. Yes, maybe sometime in the distant future it would no longer be supported, but that's a bridge to cross at that point. Before it goes away replacements need to be available.

The idea now is to move over from using untested Thunderbird specific code so that we get into a state where what we're using is at least partly tested. It should also give some performance gains.

Comment 17

[Henry Wilkes (they/them) [:henry-x]]

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

document.execCommand is deprecated, but there is no alternative yet... so everyone is using it. Yes, maybe sometime in the distant future it would no longer be supported, but that's a bridge to cross at that point. Before it goes away replacements need to be available.

The idea now is to move over from using untested Thunderbird specific code so that we get into a state where what we're using is at least partly tested. It should also give some performance gains.

ok, thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3639a8372401
Convert fontSize command to use document.execCommand. r=mkmelin

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/09a75fe88186
followup - don't set msgcompose.font_size value as that will change behavior, breaking tests. rs=bustage-fix

Comment 20

[José M. Muñoz]

That last changeset seems to have broken the ability to use the "Smaller/Larger font size" buttons, they are disabled on a new composition unless you explicitly set a size. You can see it on a new profile or when clearing msgcompose.font_size. It's a bit surprising that such an important preference will now have no default value.

Comment 21

[Magnus Melin [:mkmelin]]

Yes we'll need to fix that.
But when it's 3 (=medium) we should not output a font when writing: that should just be normal text that is not within a <font>.

Comment 22

[José M. Muñoz]

Agreed. However, setting the pref to an empty string may fix test failures on new profiles but due to profile migration https://hg.mozilla.org/comm-central/rev/3639a8372401#l1.31 the pref will end up at a value of 3 anyway, so your change does generally not mitigate what you described. Or am I missing something? Where was serialising (writing) out of a medium size suppressed before?

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: Bug 1655014 - use "medium" as default font size, and clear font size when that is used. r=henry

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dce7d6dd2ab2
use "medium" as default font size, and clear font size when that is used. r=henry

Comment 25

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1655014 - Convert EditorSelectColor to use document.execCommand for text and background color. r=mkmelin

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/93b80c75ac92
Convert EditorSelectColor to use document.execCommand for text and background color. r=mkmelin

Comment 27

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1655014 - Use document.execCommand("formatBlock",) for setting paragraphState. r=mkmelin

Comment 28

[Magnus Melin [:mkmelin]]

I think for this bug we should see if we can completely get rid of goUpdateCommandState - https://searchfox.org/comm-central/rev/549d1c21c788dd2c82f4de8381275ce19ec2ddee/mail/components/compose/content/ComposerCommands.js#199 - which is doing very odd things...

Comment 29

[Henry Wilkes (they/them) [:henry-x]]

Notes on what has been implemented so far

The commands listed in Comment 1 have been partially converted. The missing area in each case is the "remove" case, where we want to remove formatting (for normal font-size, default colors, and body text, respectively). The execCommand API exposes adding individual formatting, or removing all formatting, but it does not expose a means of removing individual formatting. There is an undo command, but that wouldn't work in general. I do find it weird that the API is so one-directional, but I don't think it is going to change given that its now listed as deprecated.

This has meant we've had to keep some of the old approach for these "remove" cases. For font size and colors, this meant keeping EditorRemoveTextProperty. For the paragraph state, this meant keeping the obtuse goDoCommandParams("cmd_paragraphState", ""). See the following for more context on the latter:

https://phabricator.services.mozilla.com/D114748#3739031
https://phabricator.services.mozilla.com/D114748#inline-639449

So the current methods are in this split approach between adding and removing formatting.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f795dfd8ebd5
Use document.execCommand("formatBlock",) for setting paragraphState. r=mkmelin