Closed
Bug 748308
Opened 13 years ago
Closed 8 years ago
Support execCommand("insertlinebreak") and execCommand("insertparagraph")
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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")
Assignee | ||
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
> 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)
Assignee | ||
Comment 4•8 years ago
|
||
(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)
Comment 5•8 years ago
|
||
(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)
Assignee | ||
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
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+
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afaae749f78d
Support insertParagraph and insertLineBreak per spec/Blink/WebKit; r=masayuki
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.
Description
•