Closed Bug 1503473 Opened 6 years ago Closed 6 years ago

Handle Enter key press on TextEditor as insertLineBreak rather than insertParagraph

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

Bug 1503473 - part 1: Rename TextEditor::OnInputParagraphSeparator() and HTMLEditor::OnInputLineBreak()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1503473 - part 2: Make TextEditRules::WillInsertBreak() and HTMLEditRules::WillInsertBreak() return EditActionResult
47 bytes, text/x-phabricator-request
Details | Review
Bug 1503473 - part 3: Move inserting a line break code in TextEditor::InsertParagraphSeparatorAsSubAction() to TextEditRules::WillInsertLineBreak()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1503473 - part 4: Create a new path to handle Enter key press in TextEditor
47 bytes, text/x-phabricator-request
Details | Review
Bug 1503473 - part 5: Move InsertParagraphSeparator*() into HTMLEditor
47 bytes, text/x-phabricator-request
Details | Review
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
status-firefox65: fixed → ---
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.

Attachment

General

Creator:
Created:
Updated:
Size: