Support execCommand("insertlinebreak") and execCommand("insertparagraph")

RESOLVED FIXED in Firefox 51

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

WebKit supports execCommand("insertlinebreak") to mean the same as the user hitting Ctrl-Enter or such, i.e., insert a <br>.  This seems useful and I specced it.
Due to bug 761993, I'm expanding this to include insertparagraph support.  I expect them to be similar enough that there's no point in a separate bug.
Summary: Support execCommand("insertlinebreak") → Support execCommand("insertlinebreak") and execCommand("insertparagraph")
Masayuki, what do you think about changing insertparagraph to match Blink (and presumably WebKit)?  They treat it the same as hitting enter, which is very useful for cross-browser tests (wpt doesn't yet support synthesizeKey equivalent).  We treat it as a synonym for formatblock "p", which is redundant.  Edge seems to actually insert a new <p> at the cursor, which doesn't seem very useful and can be emulated with insertHTML.  It's possible there's Gecko-only content that relies on current behavior, however, which needs to be switched to the cross-browser compatible formatblock command.

We also don't seem to support insertlinebreak at all, while Blink and Edge both seem to insert a <br>, so that seems like a no-brainer.  It should work the same as Shift-Enter.
Flags: needinfo?(masayuki)
> what do you think about changing insertparagraph to match Blink (and presumably WebKit)?

I think that making better compatibility with Blink is what we should do in these days (except when the behavior does not make sense).


I wrote a testcase briefly:
https://jsfiddle.net/d_toybox/k00cf3z5/

According to this, Edge's behavior is really broken. We should use Chrome's behavior, basically.

> They treat it the same as hitting enter,

Looks like not so. "insertlinebreak" should insert a <br> element. On the other hand, "insertparagraph" makes following siblings move into new <p> (or same as its parent) element.

> which is very useful for cross-browser tests (wpt doesn't
> yet support synthesizeKey equivalent).  We treat it as a synonym for formatblock "p", which is
> redundant.

"cmd_paragraphState" is also odd when I test in the first editor whose line breaker is <br>. So, emulating "Enter" key or, perhaps, just calling nsIPlaintextEditor.insertLineBreak() is better.
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> I think that making better compatibility with Blink is what we should do in
> these days (except when the behavior does not make sense).

Why Blink more than Edge or WebKit?  Are we more likely to go down the same code paths?

> Looks like not so. "insertlinebreak" should insert a <br> element. On the
> other hand, "insertparagraph" makes following siblings move into new <p> (or
> same as its parent) element.

The first behavior is the same as hitting "Shift-Enter", and the second is the same as hitting "Enter".
Flags: needinfo?(masayuki)
(In reply to :Aryeh Gregor (working until September 2) from comment #4)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> > I think that making better compatibility with Blink is what we should do in
> > these days (except when the behavior does not make sense).
> 
> Why Blink more than Edge or WebKit?

Due to current market share (including Android's Chrome). If we can behave similar to Chrome, most complicated web applications would work even if their developers haven't tested with Firefox. So, not technical reason.

> Are we more likely to go down the same code paths?

I think that we don't need to create Blink clone, but we don't have complain about Blink's implementation about non-standardized issue and we don't kill backward compatibility with our old behavior, we should take similar (or exactly same) behavior as far as possible.

> > Looks like not so. "insertlinebreak" should insert a <br> element. On the
> > other hand, "insertparagraph" makes following siblings move into new <p> (or
> > same as its parent) element.
> 
> The first behavior is the same as hitting "Shift-Enter", and the second is
> the same as hitting "Enter".

Ah, I compared with current behavior of "insertparagraph" (cmd_paragraphState).
Flags: needinfo?(masayuki)
Created attachment 8783518 [details] [diff] [review]
0001-Bug-748308-Support-insertParagraph-and-insertLineBre.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eaf56b6f5d5&exclusion_profile=false
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #8783518 - Flags: review?(masayuki)
Comment on attachment 8783518 [details] [diff] [review]
0001-Bug-748308-Support-insertParagraph-and-insertLineBre.patch

Really excellent!

>@@ -2932,17 +2932,18 @@ static const struct MidasCommand gMidasCommandTable[] = {
>   { "justifyleft",   "cmd_align",       "left", true,  false },
>   { "justifyright",  "cmd_align",      "right", true,  false },
>   { "justifycenter", "cmd_align",     "center", true,  false },
>   { "justifyfull",   "cmd_align",    "justify", true,  false },
>   { "removeformat",  "cmd_removeStyles",    "", true,  false },
>   { "unlink",        "cmd_removeLinks",     "", true,  false },
>   { "insertorderedlist",   "cmd_ol",        "", true,  false },
>   { "insertunorderedlist", "cmd_ul",        "", true,  false },
>-  { "insertparagraph", "cmd_paragraphState", "p", true,  false },
>+  { "insertparagraph", "cmd_insertParagraph", "", true,  false },
>+  { "insertlinebreak", "cmd_insertLineBreak", "", true,  false },

I don't know if somebody manages cmd_* names, though, I guess nobody does it and these names must be fine.

> /******************************************************************************
>+ * mozilla::InsertParagraphCommand
>+ ******************************************************************************/
>+
>+NS_IMETHODIMP
>+InsertParagraphCommand::IsCommandEnabled(const char* aCommandName,
>+                                         nsISupports* aCommandRefCon,
>+                                         bool* aIsEnabled)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsEnabled);

Could you use NS_WARN_IF like:

> if (NS_WARN_IF(!aIsEnabled)) {
>   return NS_ERROR_INVALID_ARG;
> }

>+NS_IMETHODIMP
>+InsertParagraphCommand::DoCommand(const char* aCommandName,
>+                                  nsISupports* aCommandRefCon)
>+{
>+  nsCOMPtr<nsIPlaintextEditor> editor = do_QueryInterface(aCommandRefCon);
>+  NS_ENSURE_TRUE(editor, NS_ERROR_NOT_IMPLEMENTED);

Same, please use NS_WARN_IF().

>+
>+  TextEditor* textEditor = static_cast<TextEditor*>(editor.get());

Not scope of this bug, we should create AsEditorBase(), AsTextEditor() and AsHTMLEditor() for safer code.

>+
>+  return textEditor->TypedText(EmptyString(), TextEditor::eTypedBreak);
>+}
>+
>+NS_IMETHODIMP
>+InsertParagraphCommand::GetCommandStateParams(const char* aCommandName,
>+                                              nsICommandParams* aParams,
>+                                              nsISupports* aCommandRefCon)
>+{
>+  NS_ENSURE_ARG_POINTER(aParams);

Please use NS_WARN_IF().

>+NS_IMETHODIMP
>+InsertLineBreakCommand::IsCommandEnabled(const char* aCommandName,
>+                                         nsISupports* aCommandRefCon,
>+                                         bool* aIsEnabled)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsEnabled);

Please use NS_WARN_IF().

>+  nsCOMPtr<nsIEditor> editor = do_QueryInterface(aCommandRefCon);
>+  if (editor) {
>+    return editor->GetIsSelectionEditable(aIsEnabled);
>+  }
>+
>+  *aIsEnabled = false;
>+  return NS_ERROR_NOT_IMPLEMENTED;

Hmm, looks like that you should use NS_WARN_IF for the error case:

> if (NS_WARN_IF(!editor)) {
>   *aIsEnabled = false;
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> return editor->GetIsSelectionEditable(aIsEnabled);

>+}
>+
>+NS_IMETHODIMP
>+InsertLineBreakCommand::DoCommand(const char* aCommandName,
>+                                  nsISupports* aCommandRefCon)
>+{
>+  nsCOMPtr<nsIPlaintextEditor> editor = do_QueryInterface(aCommandRefCon);
>+  NS_ENSURE_TRUE(editor, NS_ERROR_NOT_IMPLEMENTED);

Please use NS_WARN_IF().

>+
>+  TextEditor* textEditor = static_cast<TextEditor*>(editor.get());
>+
>+  return textEditor->TypedText(EmptyString(), TextEditor::eTypedBR);
>+}

>+NS_IMETHODIMP
>+InsertLineBreakCommand::GetCommandStateParams(const char* aCommandName,
>+                                              nsICommandParams* aParams,
>+                                              nsISupports* aCommandRefCon)
>+{
>+  NS_ENSURE_ARG_POINTER(aParams);

Please use NS_WARN_IF().
Attachment #8783518 - Flags: review?(masayuki) → review+

Comment 8

a year ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afaae749f78d
Support insertParagraph and insertLineBreak per spec/Blink/WebKit; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/afaae749f78d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.