Closed Bug 1849763 Opened 2 years ago Closed 2 years ago

Changes from bug 1840822 cause side-effects in Thunderbird

Categories

(Core :: DOM: Editor, defect)

defect

Tracking

()

RESOLVED FIXED
120 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 --- unaffected
firefox116 --- unaffected
firefox117 --- unaffected
firefox118 --- wontfix
firefox119 --- wontfix
firefox120 --- fixed

People

(Reporter: babolivier, Assigned: masayuki)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

As part of resolving bug 1840822, the following subset of the change introduced side effects in Thunderbird's plain text composer:

--- a/editor/libeditor/HTMLEditSubActionHandler.cpp
+++ b/editor/libeditor/HTMLEditSubActionHandler.cpp
@@ -2180,18 +2199,33 @@ HTMLEditor::InsertParagraphSeparatorAsSu
   return EditActionResult::HandledResult();
 }
 
 Result<CreateElementResult, nsresult> HTMLEditor::HandleInsertBRElement(
     const EditorDOMPoint& aPointToBreak, const Element& aEditingHost) {
   MOZ_ASSERT(aPointToBreak.IsSet());
   MOZ_ASSERT(IsEditActionDataAvailable());
 
-  bool brElementIsAfterBlock = false, brElementIsBeforeBlock = false;
   const bool editingHostIsEmpty = HTMLEditUtils::IsEmptyNode(aEditingHost);
+  WSRunScanner wsRunScanner(&aEditingHost, aPointToBreak);
+  WSScanResult backwardScanResult =
+      wsRunScanner.ScanPreviousVisibleNodeOrBlockBoundaryFrom(aPointToBreak);
+  if (MOZ_UNLIKELY(backwardScanResult.Failed())) {
+    NS_WARNING(
+        "WSRunScanner::ScanPreviousVisibleNodeOrBlockBoundaryFrom() failed");
+    return Err(NS_ERROR_FAILURE);
+  }
+  const bool brElementIsAfterBlock = backwardScanResult.ReachedBlockBoundary();
+  WSScanResult forwardScanResult =
+      wsRunScanner.ScanNextVisibleNodeOrBlockBoundaryFrom(aPointToBreak);
+  if (MOZ_UNLIKELY(forwardScanResult.Failed())) {
+    NS_WARNING("WSRunScanner::ScanNextVisibleNodeOrBlockBoundaryFrom() failed");
+    return Err(NS_ERROR_FAILURE);
+  }
+  const bool brElementIsBeforeBlock = forwardScanResult.ReachedBlockBoundary();
 
   // First, insert a <br> element.
   RefPtr<Element> brElement;
   if (IsPlaintextMailComposer()) {
     Result<CreateElementResult, nsresult> insertBRElementResult =
         InsertBRElement(WithTransaction::Yes, aPointToBreak);
     if (MOZ_UNLIKELY(insertBRElementResult.isErr())) {
       NS_WARNING("HTMLEditor::InsertBRElement(WithTransaction::Yes) failed");
@@ -2201,33 +2235,16 @@ Result<CreateElementResult, nsresult> HT
         insertBRElementResult.unwrap();
     // We'll return with suggesting new caret position and nobody refers
     // selection after here.  So we don't need to update selection here.
     unwrappedInsertBRElementResult.IgnoreCaretPointSuggestion();
     MOZ_ASSERT(unwrappedInsertBRElementResult.GetNewNode());
     brElement = unwrappedInsertBRElementResult.UnwrapNewNode();
   } else {
     EditorDOMPoint pointToBreak(aPointToBreak);
-    WSRunScanner wsRunScanner(&aEditingHost, pointToBreak);
-    WSScanResult backwardScanResult =
-        wsRunScanner.ScanPreviousVisibleNodeOrBlockBoundaryFrom(pointToBreak);
-    if (MOZ_UNLIKELY(backwardScanResult.Failed())) {
-      NS_WARNING(
-          "WSRunScanner::ScanPreviousVisibleNodeOrBlockBoundaryFrom() failed");
-      return Err(NS_ERROR_FAILURE);
-    }
-    brElementIsAfterBlock = backwardScanResult.ReachedBlockBoundary();
-    WSScanResult forwardScanResult =
-        wsRunScanner.ScanNextVisibleNodeOrBlockBoundaryFrom(pointToBreak);
-    if (MOZ_UNLIKELY(forwardScanResult.Failed())) {
-      NS_WARNING(
-          "WSRunScanner::ScanNextVisibleNodeOrBlockBoundaryFrom() failed");
-      return Err(NS_ERROR_FAILURE);
-    }
-    brElementIsBeforeBlock = forwardScanResult.ReachedBlockBoundary();
     // If the container of the break is a link, we need to split it and
     // insert new <br> between the split links.
     RefPtr<Element> linkNode =
         HTMLEditor::GetLinkElement(pointToBreak.GetContainer());
     if (linkNode) {
       Result<SplitNodeResult, nsresult> splitLinkNodeResult =
           SplitNodeDeepWithTransaction(
               *linkNode, pointToBreak,

This is causing permanent test failures in Thunderbird's CI. More context from my comment in the aforementioned bug:

The main reason is that setting brElementIsAfterBlock and brElementIsBeforeBlock to something other than false in this context means this code is evaluated, which messes with the placement of signatures (as it resets the cursor back to the beginning of the text area).

Some other tests are also failing from not expecting the extra <br> element because doubling it does not make sense in the plain text composer.

The specific area of comm-central related to our test failures (which I forgot to include in my comment in the aforementioned bug, apologies) is https://searchfox.org/comm-central/rev/45b4e45a73c60c081cbd7f42204e9905901ead2b/mailnews/compose/src/nsMsgCompose.cpp#704-707.

As far as I can tell this logic for setting brElementIsAfterBlock and brElementIsBeforeBlock (or bAfterBlock/bBeforeBlock, as they were called in previous incarnations) have always been in this else clause that is only evaluated when not in the plain text email editor - at least that's what I can tell from looking at searchfox's records for the past ~20y.

I have attached a mochitest for Thunderbird that demonstrates what behaviour Thunderbird expects from the plain text composer. This test passes when the diff I've included above is reverted.

:masayuki, continuing our conversation from the previous bug, could you provide some context regarding the technical reasoning why this change is necessary, to help me figure out the next steps from here?

Flags: needinfo?(masayuki)
Keywords: regression
Regressed by: 1840822

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

Attachment #9349902 - Attachment mime type: application/x-javascript → text/plain
Flags: needinfo?(masayuki)

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

For more information, please visit BugBot documentation.

Flags: needinfo?(jjaschke)

The new behavior is required for making the inserted line break visible when inserting one around a block boundary.

E.g., insertLineBreak (Shift + Enter) and insertParagraph (Enter) in specific situations require to insert 2 <br> elements in the following cases:

  • <div>abc[]</div>
  • <div>abc[]<div>def</div></div>

However, we don't need the extra <br> in the following cases:

  • <div><div>abc</div>{}</div>
  • <div>{}<div>abc</div></div>

So, the brElementIsAfterBlock && brElementIsBeforeBlock check confirms the latter cases.

HTMLEditor::InsertLineBreak() is an equivalent of insertParagraph and it is a stateful API. Therefore, could you provide the content in <body> before and after the call gets unexpected result?

Flags: needinfo?(brendan)

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

:farre could you set a severity on this?

Flags: needinfo?(afarre)
Severity: -- → S3
Flags: needinfo?(afarre)
See Also: → 1851138

Apologies for the delay, I've been unavailable for a couple of weeks.

The new behavior is required for making the inserted line break visible when inserting one around a block boundary.

Thanks for the explanation - this makes sense to me.

Therefore, could you provide the content in <body> before and after the call gets unexpected result?

Considering Test signature as the configured plaintext signature, here's the content of the <body>:

  • before the call: <br> (which looks to me to be added to HTMLEditor::Init via HTMLEditor::MaybeCreatePaddingBRElementForEmptyEditor)
  • after the call: <div class="moz-signature">-- <br>Test signature<br></div><br><br>

For what it's worth, here's the content of <body> if I revert the diff I've included in my first comment: <br><div class="moz-signature">-- <br>Test signature<br></div><br> (which is the desired result).

Flags: needinfo?(brendan)

Sorry for the delay to be back here. I reproduced it with mochitest. I'll post a patch.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

If HTMLEditor::HandleInsertBRElement inserts an invisible <br> element
after a <br> element which is requested, new caret position should be
at the invisible <br> element.

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/05889275ffc3 Make `HTMLEditor::HandleInsertBRElement` suggest caret position as at the invisible <br> if inserted r=jjaschke
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch
Duplicate of this bug: 1851138

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

Sorry for the delay to be back here. I reproduced it with mochitest. I'll post a patch.

Sorry I missed this notification - thanks a lot for taking a stab at it! I can confirm your patch fixes our failing tests (minus a small side-effect in another test that I think should be on Thunderbird to address).

See Also: 1851138
No longer duplicate of this bug: 1851138
Regressions: 1935938
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: