Closed Bug 1840500 Opened 10 months ago Closed 9 months ago

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

Categories

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

Desktop
All
defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
thunderbird_esr91 --- unaffected
thunderbird_esr102 --- unaffected
thunderbird_esr115 --- fixed
firefox-esr102 --- unaffected
firefox-esr115 --- fixed
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 --- wontfix
firefox117 --- fixed

People

(Reporter: steve, Assigned: masayuki)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(3 files)

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

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.

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.

Component: Untriaged → Message Compose Window

(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.

Flags: needinfo?(alice0775)

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

Flags: needinfo?(alice0775)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Regressed by: 1780140
Hardware: Unspecified → Desktop

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

Flags: needinfo?(masayuki)

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.

Flags: needinfo?(masayuki)

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.)

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.

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

Flags: needinfo?(masayuki)

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

Severity: -- → S4
Component: Message Compose Window → DOM: Editor
Flags: needinfo?(masayuki)
Priority: -- → P5
Product: Thunderbird → Core
Version: Thunderbird 115 → unspecified
Summary: Copy/Paste in message compose pasting incorrectly → Setting font name and pasting rich text does not respect the font name in Thunderbird

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

Severity: S4 → --
Priority: P5 → --
Summary: Setting font name and pasting rich text does not respect the font name in Thunderbird → Copy/Paste in message compose pasting incorrectly

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

(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.

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.

Summary: Copy/Paste in message compose pasting incorrectly → It may fail typing first character after changing font name in empty paragraph

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.

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.

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".

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

Assignee: nobody → masayuki
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P4

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.

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

:masayuki any update on landing this?

Flags: needinfo?(masayuki)

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

Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com:
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
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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?

Flags: needinfo?(masayuki)
Blocks: 1857788

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.
Flags: needinfo?(masayuki)
Attachment #9342064 - Flags: approval-mozilla-esr115?

The patch is cleanly graftable.

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.

Attachment #9342064 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: