message compose should use document.execCommand for editing commands
Categories
(Thunderbird :: Message Compose Window, task, P1)
Tracking
(thunderbird78 fixed)
Tracking | Status | |
---|---|---|
thunderbird78 | --- | fixed |
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(1 file, 7 obsolete files)
34.97 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
I have added many comments so everyone will get the idea about how this file is working.
Assignee | ||
Comment 3•4 years ago
|
||
A small change.
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 4•4 years ago
|
||
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?
Assignee | ||
Comment 5•4 years ago
|
||
(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.
Assignee | ||
Comment 6•4 years ago
|
||
Reporter | ||
Comment 7•4 years ago
|
||
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
Assignee | ||
Comment 8•4 years ago
|
||
(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?
Reporter | ||
Comment 9•4 years ago
|
||
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?
Assignee | ||
Comment 10•4 years ago
|
||
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
Assignee | ||
Comment 11•4 years ago
•
|
||
I think now, this is not necessary. I will confirm this and update the patch.
Comment 12•4 years ago
|
||
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.
Reporter | ||
Comment 13•4 years ago
|
||
This patch seemed to fix bug 1625218 in the process.
Assignee | ||
Comment 14•4 years ago
|
||
(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.
Reporter | ||
Comment 15•4 years ago
|
||
Assignee | ||
Comment 16•4 years ago
|
||
Assignee | ||
Comment 17•4 years ago
|
||
Missing commit message added.
Comment 18•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #15)
(related reading: bug 1430551, https://groups.google.com/forum/#!topic/mozilla.dev.platform/X2bhfUG49RE/discussion)
Magnus, I'm well aware of all this (given that I did the porting in bug 1354002, https://hg.mozilla.org/comm-central/rev/dd97865d7a80a00490e1cf1226b05ca6d2fe18bc). We're not using <div>. See:
https://searchfox.org/comm-central/rev/566c658a079a9601219ed66f049299fa1bd8b0be/mail/components/compose/content/MsgComposeCommands.js#8214-8224
Comment 19•4 years ago
•
|
||
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.
Comment 20•4 years ago
|
||
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 :-(
Comment 21•4 years ago
|
||
Oh, do a try run. Signatures and switching identities aren't well covered, but there are some tests.
Comment 22•4 years ago
|
||
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.
Reporter | ||
Comment 23•4 years ago
|
||
Isn't that exactly what the bug I quoted did? https://groups.google.com/forum/#!topic/mozilla.dev.platform/X2bhfUG49RE/discussion
Comment 24•4 years ago
•
|
||
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>.
Reporter | ||
Comment 25•4 years ago
|
||
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.
Comment 26•4 years ago
|
||
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.
Assignee | ||
Comment 27•4 years ago
|
||
(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?
Comment 28•4 years ago
|
||
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
Comment 29•4 years ago
|
||
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>
Reporter | ||
Comment 30•4 years ago
|
||
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>
Assignee | ||
Comment 31•4 years ago
|
||
(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.
Assignee | ||
Comment 32•4 years ago
|
||
(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>
Comment 33•4 years ago
|
||
Plaintext signature, paragraph composition, check resulting HTML and location of sig. separator.
Assignee | ||
Comment 34•4 years ago
•
|
||
(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?
Assignee | ||
Comment 35•4 years ago
|
||
(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 36•4 years ago
|
||
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.
Comment 37•4 years ago
|
||
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
Comment 38•4 years ago
|
||
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.
Assignee | ||
Comment 39•4 years ago
•
|
||
(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>
Assignee | ||
Comment 40•4 years ago
|
||
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
Comment 41•4 years ago
|
||
(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 42•4 years ago
|
||
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.
Assignee | ||
Comment 43•4 years ago
|
||
Reporter | ||
Comment 44•4 years ago
|
||
(I think you intended to submit an updated patch?)
Assignee | ||
Comment 45•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 46•4 years ago
|
||
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.
Reporter | ||
Comment 47•4 years ago
|
||
Reporter | ||
Updated•4 years ago
|
Comment 48•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #46)
I don't understand this comment.
Fix/clarify/remove the comment before landing?
Assignee | ||
Comment 49•4 years ago
|
||
(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.
Comment 50•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b5234d4b31c3
message compose should use document.execCommand for editing commands. r=mkmelin
Reporter | ||
Comment 51•4 years ago
|
||
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
Comment 52•4 years ago
|
||
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)
Comment 53•4 years ago
|
||
bugherder uplift |
Thunderbird 78.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/ffc62563f0d7
Updated•4 years ago
|
Reporter | ||
Comment 54•4 years ago
•
|
||
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?
Description
•