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

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 0001-Bug-748308-Support-insertParagraph-and-insertLineBre.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eaf56b6f5d5&exclusion_profile=false

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afaae749f78d
Support insertParagraph and insertLineBreak per spec/Blink/WebKit; r=masayuki

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afaae749f78d