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)
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•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 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•7 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•7 years ago
|
||
Assignee | ||
Comment 16•7 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•7 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•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Assignee | ||
Comment 20•7 years ago
|
||
Assignee | ||
Comment 21•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 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•7 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•7 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•7 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•7 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•7 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: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•