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•6 years ago
|
||
|
|
Reporter | |
Updated•5 years ago
|
|
Assignee | |
Comment 2•5 years ago
|
||
I have added many comments so everyone will get the idea about how this file is working.
|
|
Assignee | |
Comment 3•5 years ago
|
||
A small change.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 4•5 years ago
|
||
|
|
Assignee | |
Comment 5•5 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•5 years ago
|
||
|
|
Reporter | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Comment 8•5 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•5 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•5 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•5 years ago
•
|
||
I think now, this is not necessary. I will confirm this and update the patch.
|
||
Comment 12•5 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•5 years ago
|
||
This patch seemed to fix bug 1625218 in the process.
|
|
Assignee | |
Comment 14•5 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•5 years ago
|
||
|
|
Assignee | |
Comment 16•5 years ago
|
||
|
|
Assignee | |
Comment 17•5 years ago
|
||
Missing commit message added.
|
|
||
Comment 18•5 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•5 years ago
•
|
||
|
|
||
Comment 20•5 years ago
|
||
|
|
||
Comment 21•5 years ago
|
||
Oh, do a try run. Signatures and switching identities aren't well covered, but there are some tests.
|
|
||
Comment 22•5 years ago
|
||
|
|
Reporter | |
Comment 23•5 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•5 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•5 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•5 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•5 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•5 years ago
|
||
|
|
||
Comment 29•5 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•5 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•5 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•5 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•5 years ago
|
||
Plaintext signature, paragraph composition, check resulting HTML and location of sig. separator.
|
|
Assignee | |
Comment 34•5 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•5 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•5 years ago
|
||
|
|
||
Comment 37•5 years ago
|
||
|
|
||
Comment 38•5 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•5 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•5 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•5 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•5 years ago
|
||
|
|
Assignee | |
Comment 43•5 years ago
|
||
|
|
Reporter | |
Comment 44•5 years ago
|
||
(I think you intended to submit an updated patch?)
|
|
Assignee | |
Comment 45•5 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•5 years ago
|
|
|
Reporter | |
Comment 46•5 years ago
|
||
|
|
Reporter | |
Comment 47•5 years ago
|
||
|
|
Reporter | |
Updated•5 years ago
|
|
|
||
Comment 48•5 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•5 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•5 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•5 years ago
|
||
|
||
Comment 52•5 years ago
|
||
|
||
Comment 53•5 years ago
|
||
| bugherder uplift | ||
Thunderbird 78.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/ffc62563f0d7
|
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 54•5 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
•