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

Comment 26

[John Bieling (:TbSync)]

Hi @Masayuki Nakano,

this has not yet been back-ported to 115, but 115 is affected. I finally traced bug 1857788 to be caused by this missing back-port.
I manually applied D182640 to the current mozilla-esr115 (115.0), build Thunderbird and that indeed fixed it. A very simple STR is this:

Set font to a named font (e.g., Palatino Linotype). Type a few words. Hit return.
Set font to Variable. Type a few words.
First char typed is consumed and does not appear.

Example:

1234 1234 (in Palatino)
(switch font to Variable Width, then type same string:)
234 1234

Can this be back-ported as well?

Comment 27

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

Comment on attachment 9342064 [details]
Bug 1840500 - Make HTMLEditor::ClearStyleAt() remove <br> for reuse before removing its parent from the tree r=m_kato!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: New regression of ESR 115 from ESR user's point of view.

I've not reproduced bugs in Firefox which the wrong code causes, however, according to the change, this patch could fix some edge cases even in Firefox too.

• User impact if declined: Currently, this patch is required only by Thunderbird users (bug 1857788). However, the fix potentially fixes some unknown bugs of Firefox too. Therefore, I'd like to land this in Firefox's ESR 115 branch for keeping the code management simpler.

FYI: the new path runs 10 times and the new preparation path runs twice even though the patch adds one run to reproduce the reported case.

• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch corrects the order of handling <br> element when removing empty ancestor inline elements like <b>, <i>, etc and adds new path which inserts the removed <br> if it has no parent (i.e., removed as a orphan) instead of moving the node because moving node fails if the source node has no parent. So, I mean that there is no behavior change from outside of the method point of view.

Comment 28

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

The patch is cleanly graftable.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9342064 [details]
Bug 1840500 - Make HTMLEditor::ClearStyleAt() remove <br> for reuse before removing its parent from the tree r=m_kato!

Approved for 115.6esr.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/87b7114c0d0b