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)

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(3 files)

Bug 1467796 - part 1: Split TextEditor::InsertTextAsAction() to itself and TextEditor::InsertTextAsSubAction() for internal use
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1467796 - part 2: Make autocomplete use new method TextEditor::ReplaceTextAsAction() which replaces all text with specified text
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1467796 - part 3: Make mozInlineSpellChecker::ReplaceWord() use TextEditor::ReplaceTextAsAction()
59 bytes, text/x-review-board-request
smaug
: review+
m_kato
: review+
Details 
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: