Don't create transaction when script sets input.value

RESOLVED FIXED in Firefox 55

Status

()

Core
Editor
P1
normal
RESOLVED FIXED
2 months ago
21 days ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

55 Branch
mozilla55
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

2 months ago
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.
(Assignee)

Comment 1

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a60b26d6158e33ac31bed2ea47049360d3a66ec1
(Assignee)

Comment 2

2 months ago
devtools/client/inspector/computed/test/browser_computed_search-filter_context-menu.js checks cmdUndo after setting input.value...
(Assignee)

Comment 3

a month ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a month ago
mozreview-review
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 9

a month ago
mozreview-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 10

a month ago
mozreview-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 11

a month ago
mozreview-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+
(Assignee)

Comment 12

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 13

a month ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a month ago
mozreview-review
Comment on attachment 8868933 [details]
Bug 1358025 - Part 2. Add SetText Transaction API.

https://reviewboard.mozilla.org/r/140592/#review146360
Attachment #8868933 - Flags: review+

Comment 21

a month ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 23

a month ago
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

Comment 24

29 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44c15a67ed2a
https://hg.mozilla.org/mozilla-central/rev/bbe1b297cc89
https://hg.mozilla.org/mozilla-central/rev/f5c9b93e31d3
https://hg.mozilla.org/mozilla-central/rev/97275ae04155
https://hg.mozilla.org/mozilla-central/rev/029d3143f52d
Status: NEW → RESOLVED
Last Resolved: 29 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1368544

Updated

21 days ago
Depends on: 1369592
You need to log in before you can comment on or make changes to this bug.