Closed
Bug 1467796
Opened 6 years ago
Closed 6 years ago
Split TextEditor::InsertTextAsAction() for internal use and replacing text
Categories
(Core :: DOM: Editor, enhancement, P3)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(3 files)
Currently, InsertTextAsAction() is used for 3 cases: 1. As root of insertText EditAction. 2. As part of an EditAction. 3. As part of replacing text. For #2, we should create InsertTextInternal() or something. And for #3 (for insertReplacementText of InputEvent.inputType which is set when spellchecker corrects a word or autocomplete replaces typing text), ReplaceTextAsAction() or something.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86d7bc8fe37962f3569c8ec87ba62cccda049502
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe2ea516e66fa1c96b289d41a15307ebe889ce5
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f08e0dbe3241008bf2d15c97efde51e493ebc2f
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da50de9eeae04b05295634d014765982153b4cbc
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f3d66f007d08c45c8a771ed7fd591fc68378095
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c33e081789b0c756d287e704c2452f806ec9ed0b
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c7f54c531cf90179377ea890b3cdb894d4125a
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5e256c922c958956500b75a4c697558b1096ee3
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef056a34f4d8c4b9bdfc2c5a6bf95676a58357f
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=782815fa4e1541dad834908e767a59dd0de4a9ce
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07299a7c2de953eb796c8c9989e65429b54645f
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=109fd8adb983178e7d906d7af665d7795da9685c
Assignee | ||
Comment 13•6 years ago
|
||
My patch still have problem... I'd like to add new method to replace all value in editor for autocomplete like this: https://hg.mozilla.org/try/rev/c1f3eecb36581f9f654fe57d8727fa1a1c694f21 In this patch, ReplaceTextAsAction() is added and it shares same code with SetText() but it won't be handled in TextEditRules::WillSetText() because it should be undoable. So, patched build handles setting value with eSetValue_BySetUserInput flag to select all except the bogus node and delete selection and/or insert new value. However, oddly, this behavior is different only on Android (test_bug1368544.html): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d533ddeb5f367611d4104650c0b70735c6819e1e After calling setUserInput(""), root element of editor (i.e., anonymous-div element) loses all children on desktop, but only on Android, there is. Makoto-san, did you meet similar different behavior when you fix bug 1368544? Or do you have some ideas about this difference?
Flags: needinfo?(m_kato)
Comment 14•6 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #13) > My patch still have problem... > > I'd like to add new method to replace all value in editor for autocomplete > like this: > https://hg.mozilla.org/try/rev/c1f3eecb36581f9f654fe57d8727fa1a1c694f21 > In this patch, ReplaceTextAsAction() is added and it shares same code with > SetText() but it won't be handled in TextEditRules::WillSetText() because it > should be undoable. So, patched build handles setting value with > eSetValue_BySetUserInput flag to select all except the bogus node and delete > selection and/or insert new value. > > However, oddly, this behavior is different only on Android > (test_bug1368544.html): > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=d533ddeb5f367611d4104650c0b70735c6819e1e > After calling setUserInput(""), root element of editor (i.e., anonymous-div > element) loses all children on desktop, but only on Android, there is. > > Makoto-san, did you meet similar different behavior when you fix bug > 1368544? Or do you have some ideas about this difference? But 1368544 is spellchecker issue that spellchecker returns error since node is gone. I think that it is different issue, but it causes another new warning.
Flags: needinfo?(m_kato)
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab49f3e17eb852790b749ada77590bd533fbf872
Assignee | ||
Comment 16•6 years ago
|
||
Hmm, I tested additionally. Then, the remaining node is normal <br> element... https://treeherder.mozilla.org/#/jobs?repo=try&revision=3465fd2c7a1ce7b302f185b548ce6346612a25f5
Assignee | ||
Comment 17•6 years ago
|
||
Ah, spellchecker is disabled on Android by default: https://searchfox.org/comm-central/rev/b1e0c676a19ab12d499ab9167371708ed8a776ef/mozilla/editor/AsyncSpellCheckTestHelper.jsm#42-54 So, the start condition of the test may be different between on Android and the others. However, I'm still not sure why it could cause the difference...
Assignee | ||
Comment 18•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9809c3dbead44d633757dd74e68b814666f21ff5
Assignee | ||
Comment 19•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b5ef68b61b2cd15f8b52ffdff27f279f9bd17a
Assignee | ||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c96307aa098ee15d32b5ecc6d19dd970de8783b
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c42c8fbd530f9e8a0b95a1dd90ae10eebf16a71
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8996685 [details] Bug 1467796 - part 3: Make mozInlineSpellChecker::ReplaceWord() use TextEditor::ReplaceTextAsAction() https://reviewboard.mozilla.org/r/260760/#review267752 ::: dom/base/Selection.cpp:2132 (Diff revision 1) > ErrorResult& aRv) > { > - nsINode* rangeRoot = aRange.GetRoot(); > + // If the given range is part of another Selection, we need to clone the > + // range first. > + RefPtr<nsRange> range; > + if (aRange.IsInSelection() && aRange.GetSelection() != this) { I'm a bit lost here. Why would we ever pass a range belonging to another selection here? oh... we explicitly do want to use range from different selection, I see
Attachment #8996685 -
Flags: review?(bugs) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8996683 [details] Bug 1467796 - part 1: Split TextEditor::InsertTextAsAction() to itself and TextEditor::InsertTextAsSubAction() for internal use https://reviewboard.mozilla.org/r/260756/#review267968
Attachment #8996683 -
Flags: review?(m_kato) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8996684 [details] Bug 1467796 - part 2: Make autocomplete use new method TextEditor::ReplaceTextAsAction() which replaces all text with specified text https://reviewboard.mozilla.org/r/260758/#review267978
Attachment #8996684 -
Flags: review?(m_kato) → review+
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8996685 [details] Bug 1467796 - part 3: Make mozInlineSpellChecker::ReplaceWord() use TextEditor::ReplaceTextAsAction() https://reviewboard.mozilla.org/r/260760/#review267980
Attachment #8996685 -
Flags: review?(m_kato) → review+
Comment 29•6 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/3403701f0727 part 1: Split TextEditor::InsertTextAsAction() to itself and TextEditor::InsertTextAsSubAction() for internal use r=m_kato https://hg.mozilla.org/integration/autoland/rev/fd043d469773 part 2: Make autocomplete use new method TextEditor::ReplaceTextAsAction() which replaces all text with specified text r=m_kato https://hg.mozilla.org/integration/autoland/rev/48ffc0ee2258 part 3: Make mozInlineSpellChecker::ReplaceWord() use TextEditor::ReplaceTextAsAction() r=m_kato,smaug
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3403701f0727 https://hg.mozilla.org/mozilla-central/rev/fd043d469773 https://hg.mozilla.org/mozilla-central/rev/48ffc0ee2258
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•