User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

After replying to an e-mail, I wanted to remove all text styles from my and the initial e-mail. I marked the whole e-mail and pressed CTRL+SHIFT+Y and all the font definitions are shown as well.

This did not happen in previous versions of Thunderbird, as I am using this feature for years already without having had problems.

Actual results:

Dear Michi,

thanks for your message

Chris

Am 2019-03-26 um 12:33 schrieb Michi:

<!-- /* Font Definitions / @font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;} @font-face {font-family:Calibri; panose-1:2 15 5 2 2 2 4 3 2 4;} @font-face {font-family:Consolas; panose-1:2 11 6 9 2 2 4 3 2 4;} / Style Definitions */ p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0cm; margin-bottom:.0001pt; font-size:12.0pt; font-family:"Times New Roman",serif; color:black;} a:link, span.MsoHyperlink {mso-style-priority:99; color:blue; text-decoration:underline;} a:visited, span.MsoHyperlinkFollowed {mso-style-priority:99; color:purple; text-decoration:underline;} p {mso-style-priority:99; mso-margin-top-alt:auto; margin-right:0cm; mso-margin-bottom-alt:auto; margin-left:0cm; font-size:12.0pt; font-family:"Times New Roman",serif; color:black;} pre {mso-style-priority:99; mso-style-link:"HTML Vorformatiert Zchn"; margin:0cm; margin-bottom:.0001pt; font-size:10.0pt; font-family:"Courier New"; color:black;} span.HTMLVorformatiertZchn {mso-style-name:"HTML Vorformatiert Zchn"; mso-style-priority:99; mso-style-link:"HTML Vorformatiert"; font-family:Consolas; color:black;} span.E-MailFormatvorlage20 {mso-style-type:personal-reply; font-family:"Calibri",sans-serif; color:#1F497D;} .MsoChpDefault {mso-style-type:export-only; font-size:10.0pt;} @page WordSection1 {size:612.0pt 792.0pt; margin:70.85pt 70.85pt 2.0cm 70.85pt;} div.WordSection1 {page:WordSection1;} -->

Lieber Christian,

Super!

Liebe Grüsse, Michi

Expected results:

Dear Michi,

thanks for your message

Chris

Am 2019-03-26 um 12:33 schrieb Michi:

Lieber Christian,

Super!

Liebe Grüsse, Michi

Are you really using TB 66 beta?

I apologize – no. It’s 606.1. I accidentally used the Firefox version.

What puzzles me here is

This did not happen in previous versions of Thunderbird, as I
am using this feature for years already without having had problems.

I'm not aware of any changes in the area. Personally, I've never used "Discontinue Text Styles" and "Remove All Text Styles", but it appears to work, at least on a "simple" HTML e-mail.

So here you replied to an e-mail created with one of the fabulous products of our friends at Microsoft. I've just grabbed such an e-mail, and lo and behold, the ugly CSS block that it contains is indeed put into the e-mail body:

<!-- /* Font Definitions */ @font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;} @font-face {font-family:Calibri; panose-1:2 15 5 2 2 2 4 3 2 4;} @font-face {font-family:"Calisto MT"; panose-1:2 4 6 3 5 5 5 3 3 4;} /* Style Definitions */ p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0cm; margin-bottom:.0001pt; font-size:11.0pt; font-family:"Calibri",sans-serif; mso-fareast-language:EN-US;} a:link, span.MsoHyperlink {mso-style-priority:99; color:#0563C1; text-decoration:underline;} a:visited, span.MsoHyperlinkFollowed {mso-style-priority:99; color:#954F72; text-decoration:underline;} span.E-MailFormatvorlage17 {mso-style-type:personal-compose; font-family:"Calibri",sans-serif; color:windowtext;} .MsoChpDefault {mso-style-type:export-only; font-family:"Calibri",sans-serif; mso-fareast-language:EN-US;} @page WordSection1 {size:612.0pt 792.0pt; margin:70.85pt 70.85pt 2.0cm 70.85pt;} div.WordSection1 {page:WordSection1;} -->

I tried this in TB 52 and indeed it was working correctly there.

For the record, Thunderbird uses the Mozilla core editor to remove the text styles, which is registered here:
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/editor/libeditor/HTMLEditorController.cpp#133

Looks like the code is here:
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/editor/libeditor/HTMLEditorCommands.cpp#1149
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/editor/libeditor/HTMLStyleEditor.cpp#1240

So something in the Mozilla platform code went broken. Now we need to find where. Coming up ;-)

Alice, can you please find the regression for us.

STR:

• Import the attached message into a folder.
• Reply, make sure it's a HTML reply.
• Select all
• Format > Remove All Text Styles (Ctrl+Shift+Y)

You should not see <!-- HTML COMMENT --> in the body of the e-mail.

Thanks for confirming the bug (indeed it seems to happen only with e-mails from Outlook and similar products ;-) ) and taking care of it.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=6870939875be19d29a4dd68153792065d0966205&tochange=819f6281ca932a8aa27efae50aa7546f692f5cf7
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73752931e273091185e1e4b5231c28beed657cc8&tochange=a30dc237c3a600a5231f2974fc2b85dfb5513414

Thanks so much Alice. So that broke in April 2017 and no one has complained in almost two years :-(

There are some editor bugs in that range:
ac3d3acf9a9e Makoto Kato — Bug 1359008 - Don't use nsIDOM* in TextEditRules's member. r=masayuki
e05f84ea2a33 Aryeh Gregor — Bug 1355792 - Consider invisible nodes to be editable; r=masayuki

Sorry, Masayuki-san, I know you're busy, but here's another bug :-(

Np, solving regressions and crash bugs of editor is my job. Looks like that this is a regression of bug 1355792. Perhaps, invisible elements were not restyled, but now, not so.

Hmm, we remove all styles from each editable node with removing style attribute and class attribute from each element node in selection. However, Chrome does not remove non-inline styles. Therefore, from point of view of web-compat, we should follow that. However, for now, we should just take back our legacy behavior (and same as Edge's behavior) only for document.execCommand("removeformat") for now.

But, according to the commit message, editor shouldn't refer layout information (i.e., frames). So, perhaps, we should check only the visibility of the element. However, looks like that there is no style context in C++ world now.

Emilio, how can we check whether an element is visible or hidden by display and/or visibility (and also opacity??) in its parents in C++ code?

nsStyleContext doesn't exist, because I renamed to ComputedStyle. You could check that using nsComputedDOMStyle::GetComputedStyle or nsComputedDOMStyle::GetComputedStyleNoFlush...

But why? Shouldn't instead removeformat remove <style> elements?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

nsStyleContext doesn't exist, because I renamed to ComputedStyle. You could check that using nsComputedDOMStyle::GetComputedStyle or nsComputedDOMStyle::GetComputedStyleNoFlush...

Oh, really? IIRC, display: none; isn't inherited but makes any children hidden. But GetComputedStyle* return actual value?

But why? Shouldn't instead removeformat remove <style> elements?

Yes, if data node is in inline level elements (declared by HTML 4.01, but editor still use this thought), our editor removes the element and moves its children to the place which the removed elements were.
https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/HTMLStyleEditor.cpp#791,794,820-821

Well, my point is, should it really replace the node with its contents, rather than just removing its contents as well?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(Sick, maybe slow response) from comment #11)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
Oh, really? IIRC, display: none; isn't inherited but makes any children hidden. But GetComputedStyle* return actual value?

Well, no, it won't. Using GetComputedStyle is roughly like using the getComputedStyle API. If you want to know if an element is rendered, then you do need to look at the primary frame

Yes, if data node is in inline level elements (declared by HTML 4.01, but editor still use this thought), our editor removes the element and moves its children to the place which the removed elements were.
https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/HTMLStyleEditor.cpp#791,794,820-821

Well, my point is, should it really replace the node with its contents, rather than just removing its contents as well? I'm not familiar with editor, but that's what I would naively expect.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

Shouldn't instead removeformat remove <style> elements?

Certainly it shouldn't promote a HTML comment to visible text content.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Well, my point is, should it really replace the node with its contents, rather than just removing its contents as well?

Although I've not verified that, <style><!-- comment --></style> creates text node rather than comment node. Then, removeformat removes the <style> element but moves the text node (<!-- comment -->) to the place which the <style> element was.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(Sick, maybe slow response) from comment #11)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
Oh, really? IIRC, display: none; isn't inherited but makes any children hidden. But GetComputedStyle* return actual value?

Well, no, it won't. Using GetComputedStyle is roughly like using the getComputedStyle API. If you want to know if an element is rendered, then you do need to look at the primary frame

Hmm, according to the regressed bug's intention, editor shouldn't check primary frames because editor may have already changed the DOM tree or style, but it may haven't been flushed yet. Because of the security reason, I don't want to flush layout during handling each edit action. Therefore, I'd like to check current style data rather than frame.

Yes, if data node is in inline level elements (declared by HTML 4.01, but editor still use this thought), our editor removes the element and moves its children to the place which the removed elements were.
https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/editor/libeditor/HTMLStyleEditor.cpp#791,794,820-821

Well, my point is, should it really replace the node with its contents, rather than just removing its contents as well? I'm not familiar with editor, but that's what I would naively expect.

Ideally, the removeformat command should remove inline styles like color, background-color, font-*, etc (but not standardized!). However, currently, our editor just removes inline container elements. We should reimplement this with new logic similar to Chrome, but I'd like to fix only the regression as far as simpler.

(In reply to Jorg K (GMT+2) from comment #13)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

Shouldn't instead removeformat remove <style> elements?

Certainly it shouldn't promote a HTML comment to visible text content.

We shouldn't touch <style> element because it may set non-inline styles.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(Sick, maybe slow response) from comment #14)

Hmm, according to the regressed bug's intention, editor shouldn't check primary frames because editor may have already changed the DOM tree or style, but it may haven't been flushed yet. Because of the security reason, I don't want to flush layout during handling each edit action. Therefore, I'd like to check current style data rather than frame.

Then you have the same problem. You don't get up-to-date styles if you don't flush, regardless of whether you poke at frames or styles.

Well, we're really wrong.

Unofficial draft of execCommand declares a whitelist of tag names:
https://w3c.github.io/editing/execCommand.html#removeformat-candidate

Although we shouldn't trust the document, but Chrome conforms to it because the whitelist does not include <del> element oddly (probably, just forgotten), but Chrome behaves so. We should follow it.

Spec issue: https://github.com/w3c/editing/issues/192

document.execCommand("removeformat") removes any elements in the range which
are editable, not <a>, not block and a container.
https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/editor/libeditor/HTMLStyleEditor.cpp#760-763

This means that it removes hidden elements like <script> and <style>,
or non-HTML elements like SVG elements. However, the unofficial document
of execCommand() lists up elements which should be handled by the command.
https://w3c.github.io/editing/execCommand.html#removeformat-candidate

Additionally, Chrome respects this list since not including <del> element
into the list does not make sense but Chrome ignores it. So, we should
respect the list.

Do you want to uplift it to 60ESR?

That is a jolly good question. If the patch can be made to apply easily, then yes. Given that the complaint came in eight months after the release of TB 60, I don't think it's the most severe issue we face. Besides, select the stuff you don't want, hit delete, is a fair work-around ;-)

Thanks for fixing it, BTW!

https://hg.mozilla.org/mozilla-central/rev/fb6392410d8c

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

… Besides, select the stuff you don't want, hit delete, is a fair work-around ;-)

Theoretically yes, but I am using an automatic procedure to answer my e-mails, as many mails can be simply answered by “Yes”, “No”, “Thanks” (simply spoken). Therefore, I wrote macros (using PhraseExpress) to quickly answer the majority of my 50 to 200 E-Mails I get daily. I agree, for ONE e-mail it would be easy, but with the large number of e-mails I answer automatically, the “delete” work-around it not that easy any more.

Thanks to all of you for discussion and fixing the issue for the next update (and, of course, I would be glad if you could include the fix much earlier, as it safes me a lot of time).

Fortunately, the patch touched only a few lines of existing code. So, I guess I can port it ESR. I'll try it.

Unfortunately, I couldn't run ./mach wpt testing/web-platform/tests/editing/run/removeformat.html in my environment with this patch, but succeeded to build it. So, I recommend you to try to run the test locally before landing into the branch.

(This changes the behavior so that shouldn't be landed ESR 60 branch for Firefox.)

Hmm, I don't build ESR at all locally, only in automation. I'll see what I can do when it comes to merging this into our branch.

https://hg.mozilla.org/releases/mozilla-esr60/rev/27ee738ebde14de9f91501650abfef8e26ffbc0b on THUNDERBIRD_60_VERBRANCH for TB 60.7 ESR. I'll check it out when this has been built.

Christian, you can test an unofficial build of TB 60.6.2 ESR here:
32bit: https://queue.taskcluster.net/v1/task/LgEi993nR1-I7EMwEREcDQ/runs/0/artifacts/public/build/install/sea/target.installer.exe
64bit: https://queue.taskcluster.net/v1/task/Hl9OmEHHSoOy3m4qvpLBlw/runs/0/artifacts/public/build/install/sea/target.installer.exe

The fix will ship officially in TB 60.7 after mid-May.

Hello Jorg K (and all others that were involved)

Fix works. Thanks. That’s how the e-mail answer looks now:

Dear Michi,

Thanks for your message.

Chris

On 2019-03-26 12:33, Michi wrote:

Lieber Christian,

Super!

Liebe Grüsse, Michi

Have a good holiday!

Christian