"Remove All Text Styles" (cmd_removeStyles) places HTML comment into the document
Categories
(Core :: DOM: Editor, defect, P3)
Tracking
()
People
(Reporter: christian, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(3 files)
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
Comment 1•6 years ago
|
||
Are you really using TB 66 beta?
Reporter | ||
Comment 2•6 years ago
|
||
I apologize – no. It’s 606.1. I accidentally used the Firefox version.
Comment 3•6 years ago
|
||
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 ;-)
Comment 4•6 years ago
|
||
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.
Reporter | ||
Comment 5•6 years ago
|
||
Thanks for confirming the bug (indeed it seems to happen only with e-mails from Outlook and similar products ;-) ) and taking care of it.
Comment 6•6 years ago
|
||
Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=6870939875be19d29a4dd68153792065d0966205&tochange=819f6281ca932a8aa27efae50aa7546f692f5cf7
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73752931e273091185e1e4b5231c28beed657cc8&tochange=a30dc237c3a600a5231f2974fc2b85dfb5513414
Comment 7•6 years ago
|
||
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 :-(
Assignee | ||
Comment 8•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 9•6 years ago
|
||
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?
Comment 10•6 years ago
|
||
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?
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
nsStyleContext doesn't exist, because I renamed to
ComputedStyle
. You could check that usingnsComputedDOMStyle::GetComputedStyle
ornsComputedDOMStyle::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
Comment 12•6 years ago
|
||
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. ButGetComputedStyle*
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.
Comment 13•6 years ago
|
||
(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.
Assignee | ||
Comment 14•6 years ago
|
||
(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. ButGetComputedStyle*
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-821Well, 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.
Assignee | ||
Comment 15•6 years ago
|
||
(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.
Comment 16•6 years ago
|
||
(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.
Assignee | ||
Comment 17•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 18•6 years ago
|
||
Spec issue: https://github.com/w3c/editing/issues/192
Assignee | ||
Comment 19•6 years ago
|
||
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.
Comment 20•6 years ago
|
||
Comment 22•6 years ago
|
||
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!
Comment 23•6 years ago
|
||
bugherder |
Reporter | ||
Comment 24•6 years ago
|
||
(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).
Assignee | ||
Comment 25•6 years ago
|
||
Fortunately, the patch touched only a few lines of existing code. So, I guess I can port it ESR. I'll try it.
Assignee | ||
Comment 26•6 years ago
|
||
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.)
Comment 27•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 30•6 years ago
|
||
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.
Comment 31•6 years ago
|
||
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.
Reporter | ||
Comment 32•6 years ago
|
||
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
Description
•