Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3cc74586c7f90d6da8f3638d50066a54a6b0467

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 6 years ago

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().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 6 years ago

Now, TextEditor needs only InsertLineBreak*() so that
InsertParagraphSeparator*() is necessary only in HTMLEditor.
With overriding nsIPlaintextEditor::InsertLineBreak() in HTMLEditor,
we can do it simply.

	Pulsebot
	Comment 7 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 8 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/13511cab2b41
https://hg.mozilla.org/mozilla-central/rev/ec732243e9ad
https://hg.mozilla.org/mozilla-central/rev/130ba0de053f
https://hg.mozilla.org/mozilla-central/rev/d067907793ef
https://hg.mozilla.org/mozilla-central/rev/a7f7d9f366b9

	Jorg K (CEST = GMT+2)
	Comment 9 • 6 years ago

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

	Jorg K (CEST = GMT+2)
	Comment 10 • 6 years ago

Sorry Masayuki-san, I got this backed out.

	Pulsebot
	Comment 11 • 6 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 6 years ago

(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...)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 6 years ago

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.)

	Jorg K (CEST = GMT+2)
	Comment 15 • 6 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 6 years ago

(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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 17 • 6 years ago

(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().)

	Pulsebot
	Comment 18 • 6 years ago

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

	Jorg K (CEST = GMT+2)
	Comment 19 • 6 years ago

TB try based on this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ebf1c93da861bb06050571df9765f2d6e862eab6

	Jorg K (CEST = GMT+2)
	Comment 20 • 6 years ago

The crashes are gone, the remaining oranges are (sadly) expected, the remaining crash is bug 1377692.

	Natalia Csoregi [:nataliaCs]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3f5d390db0fa
https://hg.mozilla.org/mozilla-central/rev/45bc2423e812
https://hg.mozilla.org/mozilla-central/rev/952458a5da30
https://hg.mozilla.org/mozilla-central/rev/dffafc01a94d
https://hg.mozilla.org/mozilla-central/rev/bb81f74e232a