Closed Bug 1876913 Opened 4 months ago Closed 4 months ago

"Plaintext" editing problems following deletion in Firefox and Thunderbird

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
124 Branch
Tracking Flags:
Tracking Status
thunderbird_esr115 --- unaffected
firefox-esr115 --- unaffected
firefox122 --- disabled
firefox123 --- wontfix
firefox124 --- verified
firefox125 --- verified

People

(Reporter: fo, Assigned: masayuki)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files)

plaintextmail.html
840 bytes, text/html
Details
plaintextmail.eml
674 bytes, text/plain
Details
Bug 1876913 - part 1: Port `test_bug1406726.html` to WPT r=m_kato!,#dom-core
48 bytes, text/x-phabricator-request
Details | Review
Bug 1876913 - part 2: Make `HTMLEditor::FormatBlockContainerWithTransaction` refer the computed style when considering the wrapping part r=m_kato!,#dom-core
48 bytes, text/x-phabricator-request
Details | Review
1876913.png
20.65 KB, image/png
Details
Attached file plaintextmail.html —

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

Steps to reproduce:

In Firefox 115 ESR or 112 Release open the attached HTML file.

Position the caret at the very beginning of the document, select the first three lines until after the colon (":") on the third line. Delete the selection. Hit <enter>.

Actual results:

An empty line was created at the top and also another erroneous empty line after the original first line:

<empty line>
> You may refuse any item on delivery and our driver will take it away. To 
<empty line>
> help keep you and our drivers safe, you must tell the driver that you do

Expected results:

Only one empty line created at the top:

<empty line>
> You may refuse any item on delivery and our driver will take it away. To 
> help keep you and our drivers safe, you must tell the driver that you do
Attached file plaintextmail.eml —

In Thunderbird plaintext editing mode the behaviour is different, but equally erroneous.

Import the attached message as draft, edit the draft (in plaintext mode). Select from the top to after the colon, delete selection. At that point in time, unlike that happened in Firefox, one empty line remains:

<empty line>
> You may refuse any item on delivery and our driver will take it away. To 
> help keep you and our drivers safe, you must tell the driver that you do

With the caret still at the top, press delete again. Result:

> You may refuse any item on delivery and our driver will take it away.
To
> help keep you and our drivers safe, you must tell the driver that you do

The first line (which wraps onto the second now) is no longer blue, since it was moved out of the <span class=moz_quote>.

This behaviour is only present in TB 122 beta, not TB 115 ESR. Mozregression led to bug 1851951.

Keywords: regression
Regressed by: 1851951

Corrections: I was using TB 123 beta, where pref editor.block_inline_check.use_computed_style is true. When using FF 122 and setting the pref to true, the behaviour is identical to TB 123 beta with the attached HTML file. So no need to test in Thunderbird.

:masayuki, since you are the author of the regressor, bug 1851951, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Flags: needinfo?(masayuki)
Version: Firefox 115 → Firefox 123

Well, it's odd behavior, so this is obviously a bug.

On the other hand, I'm not sure how to fix this. Chrome wraps the first <br> into new <div> at the deletion. I think that this is reasonable for the further editing. However, following this could break some other special editing feature of Gecko...

Assignee: nobody → masayuki
Severity: -- → S3
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(masayuki)

Yes, on complete deletion of the first three lines, Chrome produces:

<body contenteditable="true" style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
<div class="moz-cite-prefix"><span style="color: blue;">&gt; You may refuse any item on delivery and our driver will take it away. To </span><br></div>
<span class="moz_quote" style="white-space: pre-wrap; display: block; width: 98vw;">&gt; help keep you and our drivers safe, you must tell the driver that you do <br>&gt; not wish to keep the product before the goods are handed to you so that <br>&gt; they can remove them from the totes. You'll be refunded for any returned <br>&gt; items within 3 to 5 working days.<br></span><br>
</body>

so the first line gets moved into a <div> while losing its class. While that looks good, the <div> will cause problems with TB's further "plaintext" processing where a quote is expected to be wrapped into a <span class="moz_quote">. It's not possible to just delete the <br>s and the <div>?

I think that the plaintext mode bug mentioned in comment 2 is a different one. Please file another bug if my patches cannot fix it. Checking it requires to write a mochitest. (I'll post patches soon.)

This bug is caused by that we're still using the traditional behavior for
hack in InsertParagraphSeparatorAsSubAction to wrap current line with the
default paragraph separator here:
https://searchfox.org/mozilla-central/rev/07de26de179b62f9663e3cafff21874b55acf026/editor/libeditor/HTMLEditSubActionHandler.cpp#2021-2031

If we make it use the format block mode, it starts to refer the computed style
to consider whether block or inline for the blocked <span>, but that changes
other behaviors too and test_bug1406726.html starts failing and the
expectations are really different from results in Chrome. For the further
work, this patch ports the tests into WPT and align the expectations to Chrome.

In bug 1851951, I considered that the method should keep the traditional
behavior for the backward compatibility because of mainly used by handling
a block level command, formatBlock, but I think that computing the replacing
part should be considered with the computed style of the elements. That's
compatible with the other browsers.

Depends on D199844

Thanks for addressing this. So what will the result be with attachment 9376784 [details] when deleting the first three lines, the <br><br><div> ... </div>?

I don't touch the deletion. So, the result is same as current Nightly, i.e., <br><span .... Then, you type Enter, you'll get <div><br></div><div><br></div><span ....

Attached image 1876913.png —

I don't touch the deletion. So, the result is same as current Nightly ...

The bug as I reported is that after deleting the first three lines, the result is rather surprising. Do you intend to address this? The screenshots were taken with current Nightly.

I handled the bug of insertParagraph command in the comment #0, not the deletion in the plaintext mail editor mode (mentioned in comment #1). They must be different bug, just you found them with a testcase.

(We should not handle 2 or more issues in a bug.)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/29a6fd68ebb0
part 1: Port `test_bug1406726.html` to WPT r=m_kato
https://hg.mozilla.org/integration/autoland/rev/505443c66b74
part 2: Make `HTMLEditor::FormatBlockContainerWithTransaction` refer the computed style when considering the wrapping part r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44290 for changes under testing/web-platform/tests

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

I handled the bug of insertParagraph command in the comment #0, not the deletion in the plaintext mail editor mode (mentioned in comment #1). They must be different bug, just you found them with a testcase.

Comment #0 was accidentally referring to FF 122 which shows a different editing issue not present in 123 or 124 after bug 1851951.

I filed bug 1877513 for the deletion issue.

https://hg.mozilla.org/mozilla-central/rev/29a6fd68ebb0
https://hg.mozilla.org/mozilla-central/rev/505443c66b74

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
status-firefox124: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 124 Branch
Upstream PR merged by moz-wptsync-bot
Flags: qe-verify+

I was able to reproduce the issue one time on Win10x64 using the attached html and FF 122.0 with the pref mentioned enabled.
Could not reproduce on Win10x64 using latest 125.0a1 and 124.0b4.
Francesco do you still have the issue? I am asking you because I only reproduce it once. Thank you.

Flags: needinfo?(fo)

I'm not sure what you're asking. With pref editor.block_inline_check.use_computed_style set to true, FF 122 behaves like FF 123, which is the current release. The surprising delete behaviour is still present, see bug 1877513.

Flags: needinfo?(fo)

(In reply to Francesco from comment #19)

I'm not sure what you're asking. With pref editor.block_inline_check.use_computed_style set to true, FF 122 behaves like FF 123, which is the current release. The surprising delete behaviour is still present, see bug 1877513.

Right, just that Masayuki Nakano made the fix for this, which is available in Beta 124 see link https://archive.mozilla.org/pub/firefox/candidates/124.0b5-candidates/, Can you please confirm initial issue is not reproducing in 124? Thank you.

Flags: needinfo?(fo)

I don't know what was fixed in this bug. The issue I reported wasn't fixed, it was carried forward to bug 1877513.

See comment #12:

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

I handled the bug of insertParagraph command in the comment #0,
not the deletion in the plaintext mail editor mode (mentioned in comment #1).

I didn't report a bug in insertParagraph, and I don't know what that is about. I reported a deletion bug. Sorry, I can't help you.

Flags: needinfo?(fo)

Masayuki Nakano, can you please help me with what can I verify as fixed in this bug? Thank you.

Flags: needinfo?(masayuki)

I fixed the first reported issue "An empty line was created at the top and also another erroneous empty line after the original first line:" in this bug.

  1. Type Enter here
  2. Then, empty line was inserted after "abc", but it shouldn't.

(FYI for Francesco: it's hard to fix bug 1877513, there is no test about the case, and I need to change beforeinput.getTargetRanges() behavior to conform to the spec, therefore, the amount of the change is simply big. I still need some more days to fix it.)

Flags: needinfo?(masayuki)

Apologies, yes, this issue: "An empty line was created at the top and also another erroneous empty line after the original first line".

I tested in FF 115 and 123, and the issue it present. In FF 124 beta and Nightly (125) it's not present.

Status: RESOLVED → VERIFIED

Reproduced the steps provided by Masayuki Nakano on Win10x64 using FF build 122.0.
Verified as fixed o Win10x64 / Mac 12.6/Ubuntu 23.10 using FF builds 124.0b5 and 125.0a1(20240228212327). Thank you.

status-firefox124: fixed → verified
status-firefox125: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: