Closed Bug 1697876 Opened 3 years ago Closed 3 years ago

[Address Bar] Undoing a String which exposes the switch to tab functionality until the address bar is empty will maintain the “Redo” option in a disabled state

Categories

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

Firefox 88
defect

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- wontfix
firefox89 --- verified

People

(Reporter: emilghitta, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-context-menus] [priority:2c])

Attachments

(2 files)

Affected versions

  • Firefox 88.0a1 (BuildId:20210311093001)

Affected platforms

  • Windows 7 64bit
  • macOS 10.14
  • Ubuntu 20.04

Steps to reproduce

  1. Launch Firefox.
  2. Open a new tab.
  3. Click on Twitter (from the top sites section).
  4. Open a new tab.
  5. Click on Reddit (from the top sites section).
  6. Click on the address bar, type “twitter”.
  7. Right click inside the address bar and click undo until the address bar is empty.

Expected result

  • The redo option is enabled.

Actual result

  • The redo option is disabled.

Regression Range

  • This doesn’t seem to be a regression. This issue is reproducible with builds from 27 feb (when the redo/undo options were added in Bug 1692339)

Notes

  • For further information regarding this issue access the following link for screencast. (unfortunately it exceeds the bugzilla limit. Mozilla account needed).
  • This may be influenced by the appearance of the “switch to tab” functionality.
  • This issue seems reproducible with both Proton disabled and Proton enabled.

Emilio, based on your work in bug 1695650, I don't suppose you have an idea off-hand of what might be responsible for this?

Flags: needinfo?(emilio)
See Also: → 1692673, 1695650

The redo stack gets cleared here, with this stack:

#0  mozilla::detail::nsDequeBase::PopFront() (this=0x7f1aa12dcf10) at /home/emilio/src/moz/gecko-3/xpcom/ds/nsDeque.cpp:210
#1  0x00007f1ad6a8c4e0 in nsDeque<mozilla::TransactionItem>::PopFront() (this=0x7f1aa12dcf10) at /home/emilio/src/moz/gecko-3/obj-debug/dist/include/nsDeque.h:336
#2  nsRefPtrDeque<mozilla::TransactionItem>::PopFront() (this=0x7f1aa12dcf10) at /home/emilio/src/moz/gecko-3/obj-debug/dist/include/nsDeque.h:482
#3  mozilla::TransactionStack::PopBottom() (this=0x7f1aa12dcf10) at /home/emilio/src/moz/gecko-3/editor/txmgr/TransactionStack.cpp:42
#4  mozilla::TransactionStack::Clear() (this=this@entry=0x7f1aa12dcf10) at /home/emilio/src/moz/gecko-3/editor/txmgr/TransactionStack.cpp:59
#5  0x00007f1ad6a8b143 in mozilla::TransactionManager::EndTransaction(bool) (this=this@entry=0x7f1aa12dce00, aAllowEmpty=<optimized out>) at /home/emilio/src/moz/gecko-3/editor/txmgr/TransactionManager.cpp:743
#6  0x00007f1ad6a8ab5e in mozilla::TransactionManager::DoTransaction(nsITransaction*) (this=0x7f1aa12dce00, aTransaction=0x7f1a8bd6c830) at /home/emilio/src/moz/gecko-3/editor/txmgr/TransactionManager.cpp:82
#7  0x00007f1ad697ae07 in mozilla::EditorBase::DoTransactionInternal(nsITransaction*) (this=this@entry=0x7f1ac03c58c0, aTransaction=aTransaction@entry=0x7f1a8bd6c830) at /home/emilio/src/moz/gecko-3/editor/libeditor/EditorBase.cpp:853
#8  0x00007f1ad698bbae in mozilla::EditorBase::InsertNodeWithTransaction(nsIContent&, mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent> > const&)
    (this=this@entry=0x7f1ac03c58c0, aContentToInsert=..., aPointToInsert=...) at /home/emilio/src/moz/gecko-3/editor/libeditor/EditorBase.cpp:1554
#9  0x00007f1ad6990ceb in mozilla::EditorBase::MaybeCreatePaddingBRElementForEmptyEditor() (this=0x7f1ac03c58c0) at /home/emilio/src/moz/gecko-3/editor/libeditor/EditorBase.cpp:3242
#10 0x00007f1ad6a51bcd in mozilla::TextEditor::EnsurePaddingBRElementForEmptyEditor() (this=this@entry=0x7f1ac03c58c0) at /home/emilio/src/moz/gecko-3/editor/libeditor/TextEditor.cpp:1663
#11 0x00007f1ad6a518ae in mozilla::TextEditor::OnEndHandlingTopLevelEditSubAction() (this=0x7f1ac03c58c0) at /home/emilio/src/moz/gecko-3/editor/libeditor/TextEditSubActionHandler.cpp:155
#12 0x00007f1ad6a5859c in mozilla::EditorBase::AutoEditSubActionNotifier::~AutoEditSubActionNotifier() (this=0x7ffe6b86f540) at /home/emilio/src/moz/gecko-3/obj-debug/dist/include/mozilla/EditorBase.h:2523
#13 mozilla::TextEditor::UndoAsAction(unsigned int, nsIPrincipal*) (this=0x7f1ac03c58c0, aCount=<optimized out>, aPrincipal=<optimized out>) at /home/emilio/src/moz/gecko-3/editor/libeditor/TextEditor.cpp:1031
#14 0x00007f1ad69990bb in mozilla::UndoCommand::DoCommand(mozilla::Command, mozilla::TextEditor&, nsIPrincipal*) const (this=<optimized out>, aCommand=<optimized out>, aTextEditor=..., aPrincipal=0x7f1aa12dcf28)
    at /home/emilio/src/moz/gecko-3/editor/libeditor/EditorCommands.cpp:277
#15 0x00007f1ad699850e in mozilla::EditorCommand::DoCommand(char const*, nsISupports*) (this=0x7f1ab1979e40, aCommandName=<optimized out>, aCommandRefCon=<optimized out>)
    at /home/emilio/src/moz/gecko-3/editor/libeditor/EditorCommands.cpp:65
#16 0x00007f1ad5af9b67 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) (this=<optimized out>, aCommandName=aCommandName@entry=0x7f1aa6e84dc0 "cmd_undo", aCommandRefCon=0x7f1ac03c58c0)
    at /home/emilio/src/moz/gecko-3/dom/commandhandler/nsControllerCommandTable.cpp:138
#17 0x00007f1ad5af9a7c in nsBaseCommandController::DoCommand(char const*) (this=0x7f1aa664ff10, aCommand=0x7f1aa6e84dc0 "cmd_undo") at /home/emilio/src/moz/gecko-3/dom/commandhandler/nsBaseCommandController.cpp:114
#18 0x00007f1ad366a20a in NS_InvokeByIndex () at /home/emilio/src/moz/gecko-3/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101
#19 0x00007f1ad42801b1 in CallMethodHelper::Invoke() (this=0x7ffe6b86fa60) at /home/emilio/src/moz/gecko-3/js/xpconnect/src/XPCWrappedNative.cpp:1623

Feels a bit weird that the redo stack gets cleared by MaybeCreatePaddingBRElementForEmptyEditor... I guess we're treating the <br> insertion as a regular user insertion and at that point there's nothing to "redo"...

I thought something like this:

diff --git a/editor/libeditor/InsertNodeTransaction.cpp b/editor/libeditor/InsertNodeTransaction.cpp
index 024dbbbb84ec..95f911258724 100644
--- a/editor/libeditor/InsertNodeTransaction.cpp
+++ b/editor/libeditor/InsertNodeTransaction.cpp
@@ -147,6 +147,12 @@ NS_IMETHODIMP InsertNodeTransaction::DoTransaction() {
   return NS_OK;
 }
 
+NS_IMETHODIMP InsertNodeTransaction::GetIsTransient(bool* aIsTransient) {
+  auto* br = HTMLBRElement::FromNodeOrNull(mContentToInsert.get());
+  *aIsTransient = br && br->IsPaddingForEmptyEditor();
+  return NS_OK;
+}
+
 NS_IMETHODIMP InsertNodeTransaction::UndoTransaction() {
   if (NS_WARN_IF(!mEditorBase) || NS_WARN_IF(!mContentToInsert) ||
       NS_WARN_IF(!mPointToInsert.IsSet())) {
diff --git a/editor/libeditor/InsertNodeTransaction.h b/editor/libeditor/InsertNodeTransaction.h
index 7a843ed589f8..e7dde11ce520 100644
--- a/editor/libeditor/InsertNodeTransaction.h
+++ b/editor/libeditor/InsertNodeTransaction.h
@@ -46,6 +46,8 @@ class InsertNodeTransaction final : public EditTransactionBase {
       EditorBase& aEditorBase, nsIContent& aContentToInsert,
       const EditorDOMPointBase<PT, CT>& aPointToInsert);
 
+  NS_IMETHOD GetIsTransient(bool* aIsTransient) override;
+
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(InsertNodeTransaction,
                                            EditTransactionBase)

would fix, and while it does avoid clearing the redo stack, the redo transactions don't quite work after that... Masayuki, any thoughts? You know way more about this code than I do :)

Flags: needinfo?(emilio) → needinfo?(masayuki)
Has STR: --- → yes

I think that this is a bug of TextEditor::SetTextAsSubAction. The symptom should be caused by first transaction is created before creating a <br> element for empty editor, which should be created when TextEditor is initialized.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Address Bar → DOM: Editor
Flags: needinfo?(masayuki)
Priority: -- → P2
Product: Firefox → Core

Looking for what made the empty editor (not having the padding <br> element)...

Note: when doing "undo", empty value is set to empty string from the autocomplete

_setValue (resource:///modules/UrlbarInput.jsm#1932)
onFirstResult (resource:///modules/UrlbarInput.jsm#1234)
receiveResults (resource:///modules/UrlbarController.jsm#184)
_notifyResults (resource:///modules/UrlbarProvidersManager.jsm#641)
callback (resource:///modules/UrlbarProvidersManager.jsm#580)
constructor (resource:///modules/UrlbarUtils.jsm#1911)

Hmm, I still don't have a minimized testcase for this. I write it as this:

function shouldTreatSettingValueAsInsertText() {
  const input = document.getElementById("xul");
  const selection = input.editor.selection;
  input.value = "";
  input.editor.enableUndo(false);
  input.editor.enableUndo(true); // Reset undo history.
  input.focus();
  synthesizeKey("a");
  input.value = "abc"; // Autocomplete with setting value.
  selection.setBaseAndExtent(
    input.editor.rootElement.firstChild, 1,
    input.editor.rootElement.firstChild, 3);
  synthesizeKey("b");
  input.value = "abc"; // Autocomplete with setting value.
  selection.setBaseAndExtent(
    input.editor.rootElement.firstChild, 2,
    input.editor.rootElement.firstChild, 3);
  input.editor.undo(5);
  is(input.value, "", "All input should be undone");
  is(input.editor.rootElement.firstChild.nodeName, "br",
    "When input element becomes empty, there should be only a br element");
  input.value = "";
  input.editor.redo(1);
  is(input.value, "a", "Redo should restore the first thing");
}

However, it works... What's wrong of my understanding of the UrlbarInput behavior?

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still not recoverd perfectly) from comment #6)

However, it works... What's wrong of my understanding of the UrlbarInput behavior?

Marco, are you able to help here?

Flags: needinfo?(mak)

What is undo expected to set the value to, ideally?

I think the "issue" here is the special autofill handling we do.
autofill in the urlbar uses a "placeholder" property to avoid flickering. Basically you type "t", we autofill "t[witter.com] then you type "w", in theory we'd remove [itter.com], query for "tw", see that we can autofill and autofill "tw[itter.com]". That would flicker the appended part off and back on.
To avoid that when you type w, we immediately set the value to tw[itter.com] using the placeholder. If then the query tells us "no, I didn't mean to autofill", onFirstResult (comment 5) we restore the last userTypedValue instead. It looks like undo is actually starting a query for something, and when we get the first result we see it's not autofill and setValue to the last userTypedValue. That doesn't include the previously visited uri since it was not user typed.
This needs some brainstorming yet, I don't have a solution at hand, but maybe this will help understanding what happens. Leaving the ni for now.

(In reply to Marco Bonardo [:mak] from comment #8)

What is undo expected to set the value to, ideally?

I think that it should behave as exactly same as there is no suggestion in this case.

I think the "issue" here is the special autofill handling we do.

Exactly.

autofill in the urlbar uses a "placeholder" property to avoid flickering. Basically you type "t", we autofill "t[witter.com] then you type "w", in theory we'd remove [itter.com], query for "tw", see that we can autofill and autofill "tw[itter.com]". That would flicker the appended part off and back on.
To avoid that when you type w, we immediately set the value to tw[itter.com] using the placeholder. If then the query tells us "no, I didn't mean to autofill", onFirstResult (comment 5) we restore the last userTypedValue instead. It looks like undo is actually starting a query for something, and when we get the first result we see it's not autofill and setValue to the last userTypedValue. That doesn't include the previously visited uri since it was not user typed.
This needs some brainstorming yet, I don't have a solution at hand, but maybe this will help understanding what happens. Leaving the ni for now.

Thanks for the clarification. Once undo manager is standardized, undo history can be managed by urlbar itself, but it's not available solution for now.

I think that setting value should be treated as mergeable transaction with typing text transaction. I'll try to understand around this more...

From edit side, this is caused by an "undo" not restoring a padding <br> element when the editor becomes empty. Therefore, editor creates it with a transaction but it causes clearing undone history.

I can prevent editor to create it with transaction in this case. However, I'd like to fix the broken transaction for simpler implementation. But it makes harder to debug what's going on in editor before undoing because focus move from the URL bar avoids the broken scenario.

I'll create a patch for this tomorrow even if I don't find the right way because I don't have so much time for this bug.

Whiteboard: [proton-context-menus] → [proton-context-menus] [priority:2c]

Type "t"

I/EditorTransaction 9dfe9a0 InsertNodeTransaction::DoTransaction this={ mContentToInsert=0BD4F1A0 (br), mPointToInsert={ mParent=0BD61F60 (div.input['urlbar-scheme'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=2A510900 }
I/EditorTransaction 1e872290 PlaceholderTransaction::DoTransaction this={ mName=Typing }
D/EditorTransaction 2a06ab00 PlaceholderTransaction::Merge(aOtherTransaction=1e872290) this={ mName= } returned false due to non mergable transaction
I/EditorTransaction 225b4740 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 225b4480 DeleteRangeTransaction::DoTransaction this={ mName= } Start==============================
I/EditorTransaction 225b4480 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 1f9bdfd0 DeleteTextTransaction::DoTransaction this={ mTextNode=0BD5BC90 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mOffset=0, mLengthToDelete=60, mDeletedText="", mEditorBase=22830E00 }
I/EditorTransaction 225b4480 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 225b4480 DeleteRangeTransaction::DoTransaction this={ mName= } End==============================
I/EditorTransaction 225b4740 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 225b4a20 DeleteNodeTransaction::DoTransaction this={ mContentToDelete=0BD5BC90 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mParentNode=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mRefContent=00000000, mEditorBase=22830E00 }
I/EditorTransaction 1f9bde20 InsertNodeTransaction::DoTransaction this={ mContentToInsert=0BD7C970 (#text "t"), mPointToInsert={ mParent=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=22830E00 }

I/EditorTransaction 86e4790 PlaceholderTransaction::DoTransaction this={ mName= }
D/EditorTransaction 1e872290 PlaceholderTransaction::Merge(aOtherTransaction=86e4790) this={ mName=Typing } returned false due to non mergable placeholder transaction
I/EditorTransaction 2264b2c0 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 2264b040 DeleteRangeTransaction::DoTransaction this={ mName= } Start==============================
I/EditorTransaction 2264b040 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 22651f40 DeleteNodeTransaction::DoTransaction this={ mContentToDelete=0BD7C970 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mParentNode=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mRefContent=00000000, mEditorBase=22830E00 }
I/EditorTransaction 2264b040 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 2264b040 DeleteRangeTransaction::DoTransaction this={ mName= } End==============================
I/EditorTransaction 2264b2c0 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 1fe39d30 InsertNodeTransaction::DoTransaction this={ mContentToInsert=0BD7C920 (#text "treeherder.mozilla.org/"), mPointToInsert={ mParent=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=22830E00 }

Type "r"

I/EditorTransaction 20639650 PlaceholderTransaction::DoTransaction this={ mName=Typing }
D/EditorTransaction 86e4790 PlaceholderTransaction::Merge(aOtherTransaction=20639650) this={ mName= } returned false due to non mergable transaction
I/EditorTransaction 268be040 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 27ee95e0 DeleteRangeTransaction::DoTransaction this={ mName= } Start==============================
I/EditorTransaction 27ee95e0 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 22558ee0 DeleteTextTransaction::DoTransaction this={ mTextNode=0BD7C920 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mOffset=1, mLengthToDelete=22, mDeletedText="", mEditorBase=22830E00 }
I/EditorTransaction 27ee95e0 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 27ee95e0 DeleteRangeTransaction::DoTransaction this={ mName= } End==============================
I/EditorTransaction 268be040 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 22558f10 InsertTextTransaction::DoTransaction this={ mTextNode=0BD7C920 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mOffset=1, mStringToInsert="r", mEditorBase=22830E00 }

I/EditorTransaction 288dc040 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 288dc260 DeleteRangeTransaction::DoTransaction this={ mName= } Start==============================
I/EditorTransaction 288dc260 EditAggregateTransaction::DoTransaction this={ mName=, mChildren=1 } Start==============================
I/EditorTransaction 288dc1c0 DeleteNodeTransaction::DoTransaction this={ mContentToDelete=0BD7C920 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mParentNode=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mRefContent=00000000, mEditorBase=22830E00 }
I/EditorTransaction 288dc260 EditAggregateTransaction::DoTransaction this={ mName= } End================================
I/EditorTransaction 288dc260 DeleteRangeTransaction::DoTransaction this={ mName= } End==============================
I/EditorTransaction 288dc040 EditAggregateTransaction::DoTransaction this={ mName= } End================================
D/EditorTransaction 20639650 PlaceholderTransaction::Merge(aOtherTransaction=288dc040) this={ mName=Typing } returned false due to non placeholder transaction
I/EditorTransaction 1fe73dc0 InsertNodeTransaction::DoTransaction this={ mContentToInsert=0BD87740 (#text "treeherder.mozilla.org/"), mPointToInsert={ mParent=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=22830E00 }

Undo

I/EditorTransaction 1fe73dc0 InsertNodeTransaction::UndoTransaction this={ mContentToInsert=0BD87740 (#text "treeherder.mozilla.org/"), mPointToInsert={ mParent=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=22830E00 }
I/EditorTransaction 225b8070 InsertNodeTransaction::DoTransaction this={ mContentToInsert=0BD8DE20 (br), mPointToInsert={ mParent=0BD628D0 (div.input['urlbar-input'].moz-input-box.hbox['urlbar-input-container'].hbox['urlbar'].toolbaritem['urlbar-container'].hbox['nav-bar-customization-target'].toolbar['nav-bar'].toolbox['navigator-toolbox'].box.body.html['main-window'].), mChild=00000000, mOffset=0, mIsChildInitialized=true }, mEditorBase=22830E00 }
I/EditorTransaction 288dc1c0 Merge(aOtherTransaction=225b8070) returned false

Edit: The previous analysis is wrong.

This log is easier to understand than the previous one. According to this log, the last insertion occurred by autofilling. However, oddly, there is no placeholder transaction at this time. Why??

Ah, typing "r" and setting value belong to same placeholder. I guess that it's caused by the setter is in input event listener or something which is run synchronously. But it's not invalid scenario. So, the undo should undo everything under it...

Okay, I got it. I'll post patches.

Currently, it dispatches input event with holding mPlaceholderTransaction
and mPlaceholerBatch as 1. If input event listener sets value in XUL
document, the setting value transaction will break the existing placeholder
transaction. Then, only the last one will be undone, and it causes inserting
new padding br element with transaction and clears redo history.

This patch makes it forget them before dispatching input event.

Note that I tried to create automated test for this, but I cannot reproduce
this with simple testcase. I guess something other things also required to
reproduce this bug.

Depends on D110714

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/bc3c88ef4ea5
part 1: Add logging code to transaction classes for making easier to debug r=m_kato
https://hg.mozilla.org/integration/autoland/rev/55557d6df483
part 2: Make `EditorBase::EndPlaceholderTransaction()` release placeholder transaction and related stuff before dispatching `input` event r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

This feels like a bit of a scary change to take late in a Beta cycle. Any objections to wontfixing this for 88 and letting the fix ride 89?

Flags: needinfo?(masayuki)

Absolutely. We shouldn't uplift it.

Flags: needinfo?(masayuki)

The issue is verified fixed using the latest Fx89.0a1 on WIndows 10 and Ubuntu 18.04. The 'redo' button is available after the address bar is empty. As a side note, the address bar is no longer emptied on the first undo action.

Status: RESOLVED → VERIFIED
Flags: needinfo?(mak)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: