Convert more editing commands to use document.execCommand
Categories
(Thunderbird :: Message Compose Window, task)
Tracking
(thunderbird_esr78 wontfix)
| Tracking | Status | |
|---|---|---|
| thunderbird_esr78 | --- | wontfix |
People
(Reporter: khushil324, Unassigned)
References
Details
(Keywords: leave-open)
Attachments
(4 files, 3 obsolete files)
|
Reporter | |
Comment 1•4 years ago
•
|
||
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•4 years ago
|
||
And <tt>, see bug 1653545. Or maybe that should have a bug of it's own.
|
|
Reporter | |
Comment 3•4 years ago
|
||
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.
|
|
Reporter | |
Updated•4 years ago
|
|
|
||
Comment 4•4 years ago
|
||
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
|
|
Reporter | |
Comment 5•4 years ago
|
||
(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-forIt will not return "bad" values, like 0, so the callers should also be
cleaned upAlso, 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•4 years ago
|
||
Well we could parse it where needed there, I think.
|
|
Reporter | |
Comment 7•4 years ago
|
||
|
|
||
Comment 8•4 years ago
|
||
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
|
|
Reporter | |
Comment 9•3 years ago
|
||
(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.
|
|
Reporter | |
Comment 10•3 years ago
|
||
|
|
Reporter | |
Comment 11•3 years ago
|
||
|
|
||
Comment 12•3 years ago
|
||
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
|
|
Reporter | |
Comment 13•3 years ago
•
|
||
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)
|
|
||
Updated•3 years ago
|
|
||
Comment 14•3 years ago
|
||
|
|
||
Comment 15•3 years ago
|
||
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•3 years ago
|
||
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.
|
|
||
Updated•3 years ago
|
|
|
||
Comment 17•3 years ago
|
||
(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!
|
|
||
Updated•3 years ago
|
|
|
||
Updated•3 years ago
|
|
||
Comment 18•3 years ago
|
||
Pushed by mkmelin@iki.fi: https://hg.mozilla.org/comm-central/rev/3639a8372401 Convert fontSize command to use document.execCommand. r=mkmelin
|
|
||
Comment 19•3 years ago
|
||
Pushed by mkmelin@iki.fi: 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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
|
|
||
Comment 24•3 years ago
|
||
Pushed by mkmelin@iki.fi: 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•3 years ago
|
||
|
|
||
Updated•3 years ago
|
|
|
||
Comment 26•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/93b80c75ac92
Convert EditorSelectColor to use document.execCommand for text and background color. r=mkmelin
|
|
||
Comment 27•3 years ago
|
||
|
|
||
Comment 28•3 years ago
|
||
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...
|
|
||
Updated•3 years ago
|
|
|
||
Comment 29•3 years ago
|
||
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•3 years ago
|
||
Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/f795dfd8ebd5
Use document.execCommand("formatBlock",) for setting paragraphState. r=mkmelin
|
|
||
Updated•2 years ago
|
Description
•