Changes from bug 1840822 cause side-effects in Thunderbird
Categories
(Core :: DOM: Editor, defect)
Tracking
()
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
andbrElementIsBeforeBlock
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?
Updated•2 years ago
|
Comment 1•2 years ago
|
||
Set release status flags based on info from the regressing bug 1840822
Assignee | ||
Updated•2 years ago
|
Comment 2•2 years ago
|
||
: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.
Assignee | ||
Comment 3•2 years ago
|
||
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?
Assignee | ||
Updated•2 years ago
|
Comment 4•2 years ago
|
||
Set release status flags based on info from the regressing bug 1840822
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Reporter | ||
Comment 6•2 years ago
•
|
||
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 toHTMLEditor::Init
viaHTMLEditor::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).
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
Sorry for the delay to be back here. I reproduced it with mochitest. I'll post a patch.
Assignee | ||
Comment 8•2 years ago
|
||
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.
Comment 10•2 years ago
|
||
bugherder |
Reporter | ||
Comment 12•2 years ago
|
||
(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).
Description
•