Closed Bug 1787577 Opened 3 years ago Closed 1 year ago

Problem with pressing enter after mailcite

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Version:
Firefox 91
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox123 --- fixed

People

(Reporter: b1, Unassigned)

References

(
URL
)

Details

Attachments

(2 files)

test-mailquote-enter.eml
394 bytes, text/plain
Details
1787577-mailcite-div-span.patch
7.60 KB, patch
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.5112.102 Safari/537.36 Edg/104.0.1293.63

Steps to reproduce:

This problem is only visible in Thunderbird for which the Mozilla code base provides code to "split mail cites", formerly HTMLEditor::SplitMailCiteElements(), now HTMLEditor::HandleInsertParagraphInMailCiteElement(), see:
https://hg.mozilla.org/mozilla-central/rev/62f229224596#l1.96

The error also occurs in TB 91 with the old code, so this is not a regression.

Import the attached message (.eml) into a Thunderbird draft folder and edit this draft. Follow the instructions given in the message.

Actual results:

On pressing <backspace> on an empty line, the empty line isn't removed. Pressing <backspace> again removes the last character of the preceding line.

If you check the HTML source of the message, pressing <backspace> does correctly remove the <br> element, so this may not be an editor but a layout problem.

Looks a little similar to bug 1488052.

Attachment #9291867 - Attachment mime type: message/rfc822 → text/plain

I don't see the <br> getting removed with the inspector toolbox, so most likely / almost surely an editor issue.

Thanks for looking.

Yes, it is removed. Doing a plaintext answer on your mail has:

<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;"><div class="moz-cite-prefix">On 29 Aug 2022 13:59, Bugzilla@Mozilla wrote:<br></div><span style="white-space: pre-wrap; display: block; width: 98vw;">&gt; I don't see the `&lt;br&gt;` getting removed with the inspector toolbox, so most<br>&gt; likely / almost surely an editor issue.<br></span><br>2nd line<br><br></body>

After pressing backspace we have:

<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;"><div class="moz-cite-prefix">On 29 Aug 2022 13:59, Bugzilla@Mozilla wrote:<br></div><span style="white-space: pre-wrap; display: block; width: 98vw;">&gt; I don't see the `&lt;br&gt;` getting removed with the inspector toolbox, so most<br>&gt; likely / almost surely an editor issue.</span><br>2nd line<br><br></body>

The <br> is removed from inside the span. Since the span was display:block, the <br> was not significant so the display is still fine. In summary the editor gets confused in this situation.

Do you know where may go wrong in our editor code?

Severity: -- → S3
Flags: needinfo?(masayuki)

A rough code reference was given in comment #0. Also repeating: This is only triggered by TB mail cites (quotes), it was already not working in TB 91, so not regressed by the mentioned refactoring.

One possible simple solution is, Thunderbird and the editor should use <div> instead of <span style="display:block"> for mail cites. Currently, editor considers whether an element is block or inline with only the element name, i.e., not referring style via layout's frame, and once you make the <span> to <div>, you'll get the expected result.

Another solution is, editor should refer computed style with no-flush like the white-space handling. This may fix a lot of web-compat issues with Chrome and Safari, however, the impact is too big, so I don't think we should do this right now.

URL: data:text/html,<body contenteditable>...
Flags: needinfo?(masayuki)
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true

The code in question was introduced here: https://hg.mozilla.org/mozilla-central/rev/d3c63b6f02c2#l1.34
At that time, the span was already used. Now it's here and the span is also still used:
https://searchfox.org/mozilla-central/rev/39a48ce1b2b20a004dfa364d0587164f838b62b0/editor/libeditor/HTMLEditorDataTransfer.cpp#3078
We'd be happy to test a patch that replaces the span with a div.

Is this what you had in mind? It fixes the issue reported here, but triggering "Paste as Quotation" in TB creates a new <div> as expected, but two <br> elements are inserted before the closing tag, like so:

Starting with this:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=">
    <style></style>
  </head>
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">Text to insert as quotation.<br></body>
</html>

and after inserting as quotation, resulting in this:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=">
    <style></style>
  </head>
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">Text to insert as quotation.<br><div style="white-space: pre-wrap; width : 98vw;">&gt; Text to insert as quotation<br><br></div><br></body>
</html>

Can you tell where and why the additional <br> are inserted?

Attachment #9294344 - Flags: feedback?(masayuki)
Comment on attachment 9294344 [details] [diff] [review] 1787577-mailcite-div-span.patch > // Allow wrapping on spans so long lines get wrapped to the screen. > if (aPointToInsert.IsContainerHTMLElement(nsGkAtoms::body)) { <snip> > } else { >- DebugOnly<nsresult> rvIgnored = >- aSpanElement.SetAttr(kNameSpaceID_None, nsGkAtoms::style, >- u"white-space: pre-wrap;"_ns, false); >+ DebugOnly<nsresult> rvIgnored = aDivElement.SetAttr( >+ kNameSpaceID_None, nsGkAtoms::style, >+ u"white-space: pre-wrap; display: inline;"_ns, false); Hmm, in this case, we should keep using `<span>` element for avoiding editor confused at the difference between the basic style and the specified `display` style... > span[_moz_quote=true] { > color: blue; > } > >+div[_moz_quote=true] { >+ color: blue; >+} >+ Perhaps, ``` span[_moz_quote=true], div[_moz_quote=true] { color: blue; } ```
Attachment #9294344 - Flags: feedback?(masayuki)

(In reply to b1 from comment #7)

Is this what you had in mind?

Yes, it is.

It fixes the issue reported here, but triggering "Paste as Quotation" in TB creates a new <div> as expected, but two <br> elements are inserted before the closing tag, like so:

Starting with this:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=">
    <style></style>
  </head>
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">Text to insert as quotation.<br></body>
</html>

and after inserting as quotation, resulting in this:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=">
    <style></style>
  </head>
  <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">Text to insert as quotation.<br><div style="white-space: pre-wrap; width : 98vw;">&gt; Text to insert as quotation<br><br></div><br></body>
</html>

Can you tell where and why the additional <br> are inserted?

Hmm, it's caused by that the trailing <br> element in the source is treated as intended to make an empty line at end of the <body>, however, it's not because of a single <br> element at end of the <body>. I guess that the bug is in pre- or post-handling of inserting HTML fragment in HTMLEditorDataTransfer.cpp.

And the <br> after the <div> is required to make empty line at end of <body> for putting caret at end of the document.

... in this case, we should keep using <span> element ...

Agreed, but how? The code deletes the selection and then inserts a span/div. Then it checks the parent element after the insertion and sets attributes accordingly. We'd have to analyse what the parent will be before the insertion and then insert span or div. BTW, that has always been the flow, even in the original version: https://hg.mozilla.org/mozilla-central/rev/d3c63b6f02c2#l1.34

Perhaps span[_moz_quote=true], div[_moz_quote=true] {

Agreed, we added a new rule since pre also has its own rule:
https://searchfox.org/mozilla-central/rev/b94f0c157227dadd3ecf119ce271035c52ed237b/layout/style/res/html.css#173-179

I guess that the bug is in pre- or post-handling of inserting HTML fragment ...

With the excess <br> element, this solution doesn't work. Unfortunately we don't have enough knowledge to take this further, that is, re. the span/div insertion and the excess <br>.

We have looked at this bug again and bisected it. It regressed here:
https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2016-12-15&enddate=2016-12-18
certainly via bug 1288911 (https://hg.mozilla.org/mozilla-central/rev/d3c63b6f02c2).

As Masayuki detailed in comment #5, <span style="display:block"> wasn't taken into account when the bug was filed. This has changed now, see:
https://groups.google.com/a/mozilla.org/g/dev-platform/c/mTdHrYtsMPU/m/2C6NjmdQAAAJ?utm_medium=email&utm_source=footer
and bug 1858071.

We've retested the steps from comment #0 and the problem no longer exists. Masayuki, can you please close the bug.

Flags: needinfo?(masayuki)

Oh, yes. The fix of bug 1858071 should've already fixed this bug too. Thank you!

Status: NEW → RESOLVED
Closed: 1 year ago
Depends on: 1858071
Flags: needinfo?(masayuki)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: