1840500 - It may fail typing first character after changing font name in empty paragraph

Status: RESOLVED FIXED

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

Description

[Steve Edmonds]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

Copied and pasted some monospace text within a variable with text message.

Actual results:

Incorrect pasting, sometimes nothing sometimes a backspace

Expected results:

Text pasted correctly as in TB102

Comment 1

[Steve Edmonds]

Attachment: Re: [tb-beta] Re: Noob question: re update channel & missing messages - Steve Edmonds <[e-mail]> - 2023-06-27 1002.eml

Message to test copy and paste.

Comment 2

[Steve Edmonds]

This using openSUSE/KDE

Using the attached file, in TB1150b4 File>open>open saved message. There should be some monospace lines within the variable spaced text.
Go Message>Edit as new message.
Highlight the 2 lines
└thunderbird
└thunderbird-102.12.0.tar.bz2

Place the cursor at the end of ....bz2 and press ENTER
Paste.

For me this seems to paste just a backspace. Other results are sometimes produced.

Comment 3

[Steve Edmonds]

Another way to see the issue.
Place the cursor at the end of ....bz2 and press ENTER
Change the font to variable width.
Press any letter.

For me the cursor is backspaced, with line deletion and no letter, to the end of ....bz2.

Both the above work as expected in TB102.

Comment 4

[Francesco]

(I saw this on the TB beta mailing list.)

This is much easier to reproduce. Here are the steps:

Open a compose window, set the style to "Fixed with". Type "aaa" and hit <enter>. The cursor is now on the second line. Now change the style to "Variable Width". Type a letter. The letter isn't shown. The second letter typed is shown. So after switching to "Variable Width", the next typed letter is swallowed up.

Alice, can you find the regression? This is likely a Core:Editor bug and should be moved to that component.

Comment 5

[Alice0775 White]

I can reproduce the issue on Daily116.0a1 Ubuntu20.04 and Windows10 as well.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=5f76a64dd9d51900ac5a4cad55c8f13496e69fba&tochange=e345670ea92ac98e3c7b9fa7e5d16df0e172f148
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e99353cf3334caf457ee247724f8623e5c38fb6&tochange=2d32ee89304872df06dcad6c605447205d816380

Suspect:
fdeb206a0fd4683c3087515332930b778684f25b Masayuki Nakano — Bug 1780140 - Make HTMLEditor::ClearStyleAt clean up new empty inline elements which are not contain new text r=m_kato

Comment 6

[Francesco]

Many thanks, Alice. Please move the bug to Core:Editor.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

From point of view of the editor module, the new behavior is by design. Pasting plaintext respects the temporary style, but pasting formatted text intentionally ignores the temporary style. This matches with Chrome's behavior. (You can paste it as the specified font style with Ctrl + Shift + V.)

The dropdown executes cmd_fontFace with monospace, this is an equivalent of document.execCommand("fontName", false, "monospace").

On the other hand, I'm surprised at the regression point. It seems that specifying "clear selection" in the paste handler didn't (doesn't?) work as expected.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I guess around here, HTMLEditor::CreateStyleForInsertText should be called if HTMLEditor::InsertFromTransferableAtSelection wants to respect the preserved style. So, if it's a mail editor, it sets new argument like PreservedStyle::Apply or PreservedStyle::Discard.

(Unfortunately, I don't have much time to work on this.)

Comment 9

[Alice0775 White]

STR from comment#4,

1. Open a compose window w/ html mode
2. Set the style to "Fixed with".
3. Type "aaa" and hit <enter>.
4. Change the style to "Variable Width".
5. Type a letter.

Actual results:
The letter isn't shown.

Comment 10

[Francesco]

Please move the bug to Core:Editor so it will be triaged and prioritised. It won't receive any attention in product Thunderbird.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug is not valid for Firefox. Therefore, as a bug of Core, this does not have normal priority.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, sorry, I'm confused at different bug. I need to revisit later.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1780140

Comment 14

[Francesco]

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

This bug is not valid for Firefox. Therefore, as a bug of Core, this does not have normal priority.

Hmm, surely with some effort the STR in comment #9 (from comment #4) can be executed via JS in Firefox using document.execCommand("fontName", ... and produce the same failure.

Furthermore, as I understand it, Bugzilla classifies by code module, so a bug in the editor code goes into Core:Editor, even if it's only (easily) reproducible in TB. If you consider the upstream/downstream model, FF being upstream and TB being downstream, this is an upstream bug.

As the STR show, it's not related to pasting, it can be reproduced by typing a single character after <enter> and font change.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I reproduced it in Thunderbird Daily, but...

(In reply to Francesco from comment #14)

Hmm, surely with some effort the STR in comment #9 (from comment #4) can be executed via JS in Firefox using document.execCommand("fontName", ... and produce the same failure.

I've not reproduced this in Firefox yet. How did you reproduce this in Firefox?

Furthermore, as I understand it, Bugzilla classifies by code module, so a bug in the editor code goes into Core:Editor, even if it's only (easily) reproducible in TB. If you consider the upstream/downstream model, FF being upstream and TB being downstream, this is an upstream bug.

The editor code is complicated. Some features are used only in Thunderbird, and chrome script in it also does something special. "Core" is managed by Mozilla directly, but cannot work Thunderbird aggressively. Therefore, simply moving to core component, the bug may be not handled in proper priority for Thunderbird users.

As the STR show, it's not related to pasting, it can be reproduced by typing a single character after <enter> and font change.

Yeah, and I was completely confused with different bug.

Comment 16

[Francesco]

Attachment: contenteditable-with-paragraph2.htm

I was thinking about something like this. To make it clear, it does not reproduce the problem.

document.execCommand("fontName", null, ""); does not set the font to "variable", that is "no font". What would be a way to clear the font?

Maybe using SpecialPowers.doCommand(window, "cmd_fontFace", ""); could simulate it better.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, good point.

If in execCommand case, it causes to call DoCommand here.
https://searchfox.org/mozilla-central/rev/3b707c8fd7e978eebf24279ee51ccf07895cfbcb/dom/base/Document.cpp#5537,5539

But with SpecialPowers.doCommand(window, "cmd_fontFace", ""), it must reach here.
https://searchfox.org/comm-central/rev/16c0316670744afe8e8bd146b7a714f0dcc6da00/mail/components/compose/content/ComposerCommands.js#471-472

The former must reach here.
https://searchfox.org/mozilla-central/rev/3b707c8fd7e978eebf24279ee51ccf07895cfbcb/editor/libeditor/HTMLEditorCommands.cpp#476-478

But the latter should reach here.
https://searchfox.org/mozilla-central/rev/3b707c8fd7e978eebf24279ee51ccf07895cfbcb/editor/libeditor/HTMLEditorCommands.cpp#492

According to Chrome, the behavior of Document.execCommand is compatible with them. But from the point of view of the cmd_fontFace, it's not good. I'll try to write a test.

Comment 18

[Francesco]

Note that the failure is not observed when changing the font from monospace to Arial. It's only observed with switching to Variable Width, that is "no font".

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Now, I managed to reproduce this with cmd_fontFace with empty string.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1840500 - Make `HTMLEditor::ClearStyleAt()` remove `<br>` for reuse before removing its parent from the tree r=m_kato!

Once a parent node is removed from the tree, the node may become not editable
because editable state "inherits" from ancestor nodes.
HTMLEditor::ClearStyleAt cleans up unnecessary parent inline elements first,
but if <br> element which should be reused in the block parent is contained
the inline elements, HTMLEditUtils::IsRemovableNode() will start returning
false for the <br> element because of in a document fragment which do not
contain an element whose contenteditable is true. Then,
MoveNodeWithTransaction fails to move it because of unremovable.

Therefore, this patch makes the method move <br> element before removing
inline elements and use InsertNodeWithTransaction instead if <br> element
is an orphan node.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1780140

Comment 22

[Dianna Smith [:diannaS]]

:masayuki any update on landing this?

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I was on a short vacation, landing it now.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2932851a8379
Make `HTMLEditor::ClearStyleAt()` remove `<br>` for reuse before removing its parent from the tree r=m_kato

Comment 25

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/2932851a8379