Closed Bug 1358025 Opened 7 years ago Closed 7 years ago

Don't create transaction when script sets input.value

Categories

(Core :: DOM: Editor, enhancement, P1)

Product:
Core
Component:
DOM: Editor
Version:
55 Branch
Platform:
Unspecified
All
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1358025 - Part 1. Use setUserInput instead of input.value setter.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1358025 - Part 2. Add SetText Transaction API.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1358025 - Part 3. Use nsIEditor.setText when input.value setter isn't user interaction.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1358025 - Part 4. Disable undo when input.value setter isn't user interaction.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1358025 - Part 5. Add undo transaction test on input element.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
When investigating bug 1346723, most time spends selectAll and creating/committing 2 transactions (DeleteSelection and InsertText).

Now we supports undo even if input.value is set by script, not user input, but other browser (Chrome, Edge and Safari) doesn't support undo in this situation.

To improve input.value performance, we might not have to create transactions via not eSetValue_BySetUserInput.
devtools/client/inspector/computed/test/browser_computed_search-filter_context-menu.js checks cmdUndo after setting input.value...
When I change devtools/client/inspector/test/browser_inspector_search-filter_context-menu.js to synthesizeKey, it causes the following error.  This test isn't valid since searchBox.value doesn't generate query.  When emulating user input, it generates query, then this error occurs.  We should wait suggestion result... Ugh... 

49 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_search-filter_context-menu.js | A promise chain failed to handle a rejection:  - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1212 - Error: Connection closed, pending request to server1.conn0.child1/domwalker30, type getSuggestionsForQuery failed
Comment on attachment 8868933 [details]
Bug 1358025 - Part 2. Add SetText Transaction API.

https://reviewboard.mozilla.org/r/140592/#review143978

I agree with this patch. However, I'd like to check new patch. Therefore, marking r- for now.

::: editor/libeditor/EditorBase.cpp:2657
(Diff revision 1)
> +  if (NS_WARN_IF(!transaction)) {
> +    return NS_ERROR_FAILURE;
> +  }

We should be able to get rid of OOM check for CreatetxnFor*(). However, it's okay since IIRC, other callers also check it.

::: editor/libeditor/EditorBase.cpp:2666
(Diff revision 1)
> +  // Let listeners know what's up
> +  {
> +    AutoActionListenerArray listeners(mActionListeners);
> +    for (auto& listener : listeners) {
> +      listener->WillDeleteText(
> +        static_cast<nsIDOMCharacterData*>(aCharData.AsDOMNode()), 0,
> +        length);
> +      listener->WillInsertText(
> +        static_cast<nsIDOMCharacterData*>(aCharData.AsDOMNode()), 0,
> +        aString);
> +    }
> +  }
> +
> +  nsresult rv = DoTransaction(transaction);
> +
> +  // Let listeners know what happened
> +  {
> +    AutoActionListenerArray listeners(mActionListeners);
> +    for (auto& listener : listeners) {
> +      listener->DidDeleteText(
> +        static_cast<nsIDOMCharacterData*>(aCharData.AsDOMNode()), 0,
> +        length, rv);
> +      listener->DidInsertText(
> +        static_cast<nsIDOMCharacterData*>(aCharData.AsDOMNode()), 0,
> +        aString, rv);
> +    }
> +  }

Do we need to notify listeners of deleting text even if old value is empty string? It's okay if the cost getting old value is expensive, though.

Similarly, do we need to notify listeners of inserting text even if aCharData is empty?

::: editor/libeditor/TextEditRules.cpp:797
(Diff revision 1)
> +  if (!IsPlaintextEditor() || textEditor->IsIMEComposing() ||
> +      IsPasswordEditor() || aMaxLength != -1) {

I don't understand why we cannot handle this for password editor. Could add comment to explain the reason?

::: editor/libeditor/TextEditRules.cpp:807
(Diff revision 1)
> +
> +  WillInsert(aSelection, aCancel);
> +  // we want to ignore result of WillInsert()
> +  *aCancel = false;
> +
> +  // TextEditRules::WillSetText handles that child node is one text node only.

Perhaps, "handles only when there is only one node and it's a text node"?

And although I don't know the actual behavior, if there is no children, why this cannot handle the action? If it's intentional behavior, I'd like you to add the explanation here.

::: editor/libeditor/TextEditor.cpp:805
(Diff revision 1)
> +  }
> +  if (!handled) {
> +    // We want to select trailing BR node to remove all nodes to replace all,
> +    // but TextEditor::SelectEntireDocument doesn't select that BR node.
> +    if (rules->DocumentIsEmpty()) {
> +      // if it's empty don't select entire doc - that would select

nit: perhaps, needs "," after "empty"?
Attachment #8868933 - Flags: review?(masayuki) → review-
Comment on attachment 8868934 [details]
Bug 1358025 - Part 3. Use nsIEditor.setText when input.value setter isn't user interaction.

https://reviewboard.mozilla.org/r/140594/#review143992

::: commit-message-642db:3
(Diff revision 1)
> +Bug 1358025 - Part 3. Use nsIEditor.setText when input.value setter isn't user interaction. r?masayuki
> +
> +When not using eSetValue_BySetUserInput, we should use SetText transaction instead for fast path.  For old compatibility, when input.value setter is by user interaction, I keep original way.  Because the original way doesn't replace all text when some string matches.

s/old compatibility/backward compatibility
Attachment #8868934 - Flags: review?(masayuki) → review+
Comment on attachment 8868935 [details]
Bug 1358025 - Part 4. Disable undo when input.value setter isn't user interaction.

https://reviewboard.mozilla.org/r/140596/#review143998

::: dom/html/nsTextEditorState.cpp:2661
(Diff revision 1)
> +            AutoDisableUndo disableUndo(mEditor);
> +
>              plaintextEditor->SetText(newValue);

Is this same behavior as other browsers?

I'd like to know if setting input.value discards all undo transactions before set.

And I'd like you to add automated test for this case. I think that if the behavior is exactly same as all other browsers, it could be in wpt test.  Otherwise, should be under mozilla of wpt test? 

Perhaps, there are 5 cases:
1. empty value to empty value
2. empty value to non-empty value
3. non-empty value to non-empty value (same value)
4. non-empty value to non-empty value (different value)
5. non-empty value to empty value

Could you add the automated tests to additional patch?
Attachment #8868935 - Flags: review?(masayuki) → review+
Comment on attachment 8868932 [details]
Bug 1358025 - Part 1. Use setUserInput instead of input.value setter.

https://reviewboard.mozilla.org/r/140590/#review144080
Attachment #8868932 - Flags: review?(pbrosset) → review+
Comment on attachment 8868933 [details]
Bug 1358025 - Part 2. Add SetText Transaction API.

https://reviewboard.mozilla.org/r/140592/#review143978

> Do we need to notify listeners of deleting text even if old value is empty string? It's okay if the cost getting old value is expensive, though.
> 
> Similarly, do we need to notify listeners of inserting text even if aCharData is empty?

I see.  When test is passed if we don't send notification with empty string.  I will remove this.

> I don't understand why we cannot handle this for password editor. Could add comment to explain the reason?

Password has to mask to "*" on text node, and we create timer on this SetText if echo is turned on.  So I don't want to write same logic (even if it is new function) since it becomes complex code.

If we have to improve this for password editor, I can support it on SetText.

> Perhaps, "handles only when there is only one node and it's a text node"?
> 
> And although I don't know the actual behavior, if there is no children, why this cannot handle the action? If it's intentional behavior, I'd like you to add the explanation here.

I want to be more simply code, so I don't want to add code for emtpy document.  If empty, this fast path won't get more improvement since selectall is nothging and delete selection doesn't remove nodes.

If empty situation is botleneck, I will add it.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> Comment on attachment 8868935 [details]
> Bug 1358025 - Part 4. Disable undo when input.value setter isn't user
> interaction.
> 
> https://reviewboard.mozilla.org/r/140596/#review143998
> 
> ::: dom/html/nsTextEditorState.cpp:2661
> (Diff revision 1)
> > +            AutoDisableUndo disableUndo(mEditor);
> > +
> >              plaintextEditor->SetText(newValue);
> 
> Is this same behavior as other browsers?
> 
> I'd like to know if setting input.value discards all undo transactions
> before set.
> 
> And I'd like you to add automated test for this case. I think that if the
> behavior is exactly same as all other browsers, it could be in wpt test. 
> Otherwise, should be under mozilla of wpt test? 

Is undo management standardized?  https://www.w3.org/wiki/Web_Editing_APIs/UndoManager_Problem_Descriptions and https://html.spec.whatwg.org/multipage/forms.html are nothing for form.  Please point me for this spec.

I can add this to mochitest.
(In reply to Makoto Kato [:m_kato] from comment #13)
> Is undo management standardized? 

Not sure, but perhaps, not yet.

> I can add this to mochitest.

If you add it to wpt under mozilla, it's easier to run on other browsers but won't be upstreamed.
https://searchfox.org/mozilla-central/source/testing/web-platform/mozilla/README

So, only when we need to use EventUtils.js, we need to keep writing as mochitests.
Comment on attachment 8871212 [details]
Bug 1358025 - Part 5. Add undo transaction test on input element.

https://reviewboard.mozilla.org/r/142700/#review146362

Excellent!

::: editor/libeditor/tests/test_bug1358025.html:15
(Diff revision 2)
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1358025">Mozilla Bug 1358025</a>
> +<div id="display">
> +  <input type="text" id="edit">
> +</div>
> +<div id="content" style="display: none">
> + 

nit: whitespace in empty line.
Attachment #8871212 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/44c15a67ed2a
Part 1. Use setUserInput instead of input.value setter. r=pbro
https://hg.mozilla.org/integration/autoland/rev/bbe1b297cc89
Part 2. Add SetText Transaction API. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/f5c9b93e31d3
Part 3. Use nsIEditor.setText when input.value setter isn't user interaction. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/97275ae04155
Part 4. Disable undo when input.value setter isn't user interaction. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/029d3143f52d
Part 5. Add undo transaction test on input element. r=masayuki
Depends on: 1368544
Depends on: 1369592
Depends on: 1376147
Depends on: 1385514
Depends on: 1385525
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: