Closed Bug 1503473 Opened 11 months ago Closed 11 months ago

Handle Enter key press on TextEditor as insertLineBreak rather than insertParagraph

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

Enter key press causes "insertParagraph" on HTMLEditor, and Shift + Enter key press causes "insertLineBreak" on HTMLEditor.  On the other hand, Enter key press should cause "insertLineBreak" on TextEditor but we handle it as "insertParagraph". We need to correct those terminology issue.
TextEditor::OnInputParagraphSeparator() and HTMLEditor::OnInputLineBreak() are
also used by command handlers.  Therefore, they should be renamed to
TextEditor::InsertParagraphSeparatorAsAction() and
HTMLEditor::InsertLineBreakAsAction().  Then, current
TextEditor::InsertParagraphSeparatorAsAction() should be renamed to
AsSubAction() and each caller of it should create AutoPlaceholderBatch
by themselves.
With this cleaning up, we can know when they return NS_OK with both
aCanceled is false and aHandled is false.  (Look for EditActionIgnored().)

Additionally, this patch renames HTMLEditRules::WillInsertBreak() to
WillInsertParagraphSeparator() and TextEditRules::WillInsertBreak() to
TextEditRules::WillInsertLineBreak().
When TextEditRules::WillDoAction() and HTMLEditRules::WillDoAction() didn't
return error, didn't handle it, and didn't cancel it,
TextEditor::InsertParagraphSeparatorAsSubAction() inserts a line breaker.
However, this case is only when the instance is TextEditRules and
TextEditRules::WillInsertLineBreak() prepares to insert a line break without
any errors.  So, we can move the part into TextEditRules::WillInsertLineBreak()
simply.
This patch creates new path to insert a line break in TextEditor.

Declares new EditSubAction::eInsertLineBreak and makes the path use
EditAction::eInsertLineBreak instead of EditAction::eInsertParagraphSeparator.

Unfortunately, this patch makes TextEditor::InsertLineBreakAsAction() as
a virtual method for keeping this change as small as possible.
Now, TextEditor needs only InsertLineBreak*() so that
InsertParagraphSeparator*() is necessary only in HTMLEditor.
With overriding nsIPlaintextEditor::InsertLineBreak() in HTMLEditor,
we can do it simply.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/13511cab2b41
part 1: Rename TextEditor::OnInputParagraphSeparator() and HTMLEditor::OnInputLineBreak() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ec732243e9ad
part 2: Make TextEditRules::WillInsertBreak() and HTMLEditRules::WillInsertBreak() return EditActionResult r=m_kato
https://hg.mozilla.org/integration/autoland/rev/130ba0de053f
part 3: Move inserting a line break code in TextEditor::InsertParagraphSeparatorAsSubAction() to TextEditRules::WillInsertLineBreak() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d067907793ef
part 4: Create a new path to handle Enter key press in TextEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a7f7d9f366b9
part 5: Move InsertParagraphSeparator*() into HTMLEditor r=m_kato
I think this is a candidate for a backout since it crashes Thunderbird, see:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=cffa7cb63305e24e8a6c72717e457d6fdcb9298d

Check the orange Z's and see:
PROCESS-CRASH | cloudfile | application crashed [@ mozilla::HTMLEditRules::BeforeEdit(mozilla::EditSubAction, short)]
and
PROCESS-CRASH | composition | application crashed [@ mozilla::HTMLEditRules::BeforeEdit(mozilla::EditSubAction, short)]

Note that those test directories exercise the compose window and hence the editor, but they don't do anything special. I guess TB will be pretty broken with those crashes.

In debug we see additionally:
Assertion failure: IsEditActionDataAvailable(), at /builds/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:1178
Flags: needinfo?(masayuki)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
Sorry Masayuki-san, I got this backed out.
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/75e4c3050cac
Backed out 5 changesets for crashes in Thunderbird on request of jorgk. a=backout
(In reply to Jorg K (GMT+2) from comment #10)
> Sorry Masayuki-san, I got this backed out.

No, thank you for your good catch. The assertion means that I forgot to add necessary AutoEditActionDataSetter or accidentally removed unnecessary AutoEditActionDataSetter.  This looks the former. I'll reland them with the fix soon.

(And I'm really surprised that the orange means that we don't test Shift + Enter key press on m-c...)
Flags: needinfo?(masayuki)
Ah, I'm wrong. The orange means that we don't test nsIPlaintextEditor.insertLineBreak() on m-c. I'll add automated tests in a follow up bug.
New one passed additional tests in bug 1504379 in my environment. I hope that it won't break comm-central too. (I'll re-land them if https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bd1a11dbc49fc9d9e1c4249cc07f98c714c6290 becomes green.)
Last time I interfered with M-C editor work, I caused a big stir (bug 1393140 comment 23 and below) :-( - As a consequence of that, I wrote test_bug1397412.xul. Looking at your tests, insertLineBreak() is the same as actually typing enter, right? Why don't you use synthesizeKey(), isn't that a more realistic scenario?
(In reply to Jorg K (GMT+2) from comment #15)
> Last time I interfered with M-C editor work, I caused a big stir (bug
> 1393140 comment 23 and below) :-( - As a consequence of that, I wrote
> test_bug1397412.xul. Looking at your tests, insertLineBreak() is the same as
> actually typing enter, right? Why don't you use synthesizeKey(), isn't that
> a more realistic scenario?

That's already been tested. Currently, if we need to call an XPCOM method internally, we have another public method which is non-virtual especially when it may be in a hot path or usual user input.
(FYI: I tested actual input before the previous landing since this is necessary for bug 1447239 and I tested with a patch for it since I need to check whether the result is as expected or not. So, the orange occurs only when calling nsIPlaintextEditor.insertLineBreak().)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3f5d390db0fa
part 1: Rename TextEditor::OnInputParagraphSeparator() and HTMLEditor::OnInputLineBreak() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/45bc2423e812
part 2: Make TextEditRules::WillInsertBreak() and HTMLEditRules::WillInsertBreak() return EditActionResult r=m_kato
https://hg.mozilla.org/integration/autoland/rev/952458a5da30
part 3: Move inserting a line break code in TextEditor::InsertParagraphSeparatorAsSubAction() to TextEditRules::WillInsertLineBreak() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/dffafc01a94d
part 4: Create a new path to handle Enter key press in TextEditor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bb81f74e232a
part 5: Move InsertParagraphSeparator*() into HTMLEditor r=m_kato
The crashes are gone, the remaining oranges are (sadly) expected, the remaining crash is bug 1377692.
You need to log in before you can comment on or make changes to this bug.