Closed Bug 1467796 Opened 7 years ago Closed 7 years ago

Split TextEditor::InsertTextAsAction() for internal use and replacing text

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

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.
Priority: -- → P3
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)
(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)
Hmm, I tested additionally. Then, the remaining node is normal <br> element... https://treeherder.mozilla.org/#/jobs?repo=try&revision=3465fd2c7a1ce7b302f185b548ce6346612a25f5
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...
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 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 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 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+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: