1297414 - Generate <p>/<div> for newlines, not <br> (defaultParagraphSeparator)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

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

(This was originally filed as bug 748303, but it got taken over by unrelated discussion.  I'm copying the relevant bits here for reference.)

If you have an empty editable div and type in "foo\nbar", IE/Opera generate
  <p>foo</p><p>bar</p>,
WebKit generates
  foo<div>bar</div>,
and Gecko generates
  foo<br>bar.
Likewise, if you have
  foo<blockquote>bar</blockquote>baz
and outdent, Gecko generates <br>s to separate the lines, IE/Opera generates <p>, WebKit <div>.  After lots of discussion, the spec requires <p>, with <div> allowable optionally using the defaultParagraphSeparator command.  Gecko should update to match the spec.

This should have no big web-compat impact, because it will just be changing to match IE, but there will probably be problems with Thunderbird and/or Composer -- maybe they'll want to set defaultParagraphSeparator = div to make sure there's no extra whitespace.

I think this is one of the most annoying browser differences in contenteditable, particularly since it hits people who aren't even using execCommand() (i.e., most editors).  Recent WebKit still defaults to div but supports defaultParagraphSeparator; if Gecko output <p> instead of <br>, the latest version of every browser could be made to output <p>, which would be a nice compat win for authors.

(richtext2 requires the non-Gecko behavior here for outdent.)

(:Ehsan Akhgari from bug 748303 comment #1)
> Hmm, I thought we fixed this a while ago.  Note that we would probably have
> to keep the cmd_insertBrOnReturn command working.
> 
> There's also a pref which implements a node used by Thunderbird and Composer
> which lets you enter a BR the first time you press Enter, and replace it
> with a new paragraph element if you press Enter immediately after.  I can't
> seem to remember the name of the pref, and I can't find it now, but Fabien
> knows about it.

Masayuki, what do you think?  Do you think we could get away from switching the default on the web to <p>, or will we have to stick to <br> by default and make <p> opt-in?  I suspect we might hit browser sniffing here, but maybe it's worth it.  We can certainly support defaultParagraphSeparator, at least, and just add our own non-standard "br" value as the default, so authors can choose p/div if they want: https://dvcs.w3.org/hg/editing/raw-file/329ddf94242a/editing.html#the-defaultparagraphseparator-command

Comment 1

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

According to your comment, "After lots of discussion, the spec requires <p>, with <div> allowable optionally using the defaultParagraphSeparator command.  Gecko should update to match the spec.", and personally, I like using <p> for default line breaker in contenteditable, yes, I think that we should change the default behavior of Enter key to use <p>.

If Thunderbird still needs to use <br>, Thunderbird should change the behavior with cmd_insertBrOnReturn.

I wonder, what should occur when user presses Enter key in |<div contenteditable>foo<br>bar</div>|? Even if web page author expected to use <br> for a line breaker, we'd change the behavior? In such web apps, we're now inserting <br>. Therefore, if we would start to insert <p> in them, they would include two styles line breakers. This must be bad when this bug is fixed while they edit such data across sessions.

Comment 2

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #1)
> According to your comment, "After lots of discussion, the spec requires <p>,
> with <div> allowable optionally using the defaultParagraphSeparator command.
> Gecko should update to match the spec.", and personally, I like using <p>
> for default line breaker in contenteditable, yes, I think that we should
> change the default behavior of Enter key to use <p>.

I thought about it, and realized that it's a problem that <p> has a margin by default.  Indeed, in a simple contenteditable page like

data:text/html,<!doctype html><div contenteditable><br></div>

if you type some text with line breaks, in IE11 it will produce <p>s and there will be margins, unlike all other browsers.  In WebKit/Blink last I tested, they generate <div> and didn't want to change.  I notice Edge has also switched to <div>, so if we're going to change the default it also needs to be to <div> to match other browsers.  I think this is lame, because <div> is not supposed to be used for that, but I don't think it's worth trying to be different.

Supporting <div> here at all will require changing behavior elsewhere to treat it more like a paragraph.  Specifically, right now if you hit Enter in a div, it will insert a <br> always, not duplicate the <div>, but for a <p> it will duplicate it.  This behavior is probably correct in theory, but if we have to use <div> as a separator to match all other UAs, we need to change it also.

> If Thunderbird still needs to use <br>, Thunderbird should change the
> behavior with cmd_insertBrOnReturn.

This doesn't replicate current behavior, because it will behave differently if you press Enter inside a <p>.

> I wonder, what should occur when user presses Enter key in |<div
> contenteditable>foo<br>bar</div>|? Even if web page author expected to use
> <br> for a line breaker, we'd change the behavior? In such web apps, we're
> now inserting <br>. Therefore, if we would start to insert <p> in them, they
> would include two styles line breakers. This must be bad when this bug is
> fixed while they edit such data across sessions.

This is already a problem if they edit from different browsers, or if two users edit the same content using different browsers.  I don't think there's any good way around it, because we have no way of distinguishing a <br> that was inserted because the user pressed Enter (should change) and one they deliberately inserted using Shift-Enter (should remain a <br>, e.g., in list items).  Also, then you have to figure out what br's are invisible, and I don't think we do that reliably.

So I think we should switch to <div> as default to match all other UAs, although IMO <p> is actually better.  This will require some changes in line break handling in <div>.  Generally I don't think it's a good idea to spend resources making significant editor changes these days, because so few sites use most of it (AFAIK) and we're likely to break things, but this is a pretty major issue (relevant every time the user hits Enter) and all the other browsers have converged.  The current functionality of outputting <br> will be accessible via a non-standard "br" value for defaultParagraphSeparator, in case that's the easiest way for any sites we break to keep working.  Do you agree?

Comment 3

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

I don't know what I did wrong, but it seems Edge does still produce <p>.  So the question is, do we want to match Edge's default, or Blink/WebKit?  When this was discussed a few years ago, I was told that WebKit (no Blink yet IIRC) would not change its default, because there was a ton of WebKit-specific content that they didn't want to break.  The Blink/WebKit behavior is likely to be more compatible, I think, because it has no margin by default, so it looks like our current output.  But Edge is definitely more correct.

Whatever you choose, I'll update the spec/tests to match (unless someone disagrees).  If we go with <div>, Edge is outnumbered 3-to-1.  If we go with <p>, it's an even split and <p> is more semantically correct and what the spec already said.

Comment 4

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

I put a lot of work into this, but on consideration, I don't think it's worth the effort to finish it off and get it working, unless you disagree.  Practically speaking, I don't think any major sites use contenteditable without overriding the behavior for hitting Enter etc., and the sites that do use it already deal with our current behavior somehow, and in fact might break if we change.  So basically this type of thing would just be for Thunderbird.

So maybe we should mark this type of bug from now on as WONTFIX.  People should be using JS libraries, not the built-in functionality.  Instead, we should work on bugs to do with caret positioning or other things that real-world editing apps are interested in.

Comment 5

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

Masayuki, ping?

Comment 6

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

(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #2)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #1)
> > According to your comment, "After lots of discussion, the spec requires <p>,
> > with <div> allowable optionally using the defaultParagraphSeparator command.
> > Gecko should update to match the spec.", and personally, I like using <p>
> > for default line breaker in contenteditable, yes, I think that we should
> > change the default behavior of Enter key to use <p>.
> 
> I thought about it, and realized that it's a problem that <p> has a margin
> by default.  Indeed, in a simple contenteditable page like
> 
> data:text/html,<!doctype html><div contenteditable><br></div>
> 
> if you type some text with line breaks, in IE11 it will produce <p>s and
> there will be margins, unlike all other browsers.  In WebKit/Blink last I
> tested, they generate <div> and didn't want to change.  I notice Edge has
> also switched to <div>, so if we're going to change the default it also
> needs to be to <div> to match other browsers.  I think this is lame, because
> <div> is not supposed to be used for that, but I don't think it's worth
> trying to be different.
> 
> Supporting <div> here at all will require changing behavior elsewhere to
> treat it more like a paragraph.  Specifically, right now if you hit Enter in
> a div, it will insert a <br> always, not duplicate the <div>, but for a <p>
> it will duplicate it.  This behavior is probably correct in theory, but if
> we have to use <div> as a separator to match all other UAs, we need to
> change it also.

Looks like that Chromium uses <div> as the default separator (I've not found what changes the default separator from the source code yet). And when Enter key is pressed neither at editing root, at end of a block nor in a heading element, it duplicates same block element, perhaps, blockquote, etc?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp?sq=package:chromium&dr=CSs&l=244
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/commands/InsertParagraphSeparatorCommand.cpp?l=116&cl=GROK&gsn=shouldUseDefaultParagraphElement

> > If Thunderbird still needs to use <br>, Thunderbird should change the
> > behavior with cmd_insertBrOnReturn.
> 
> This doesn't replicate current behavior, because it will behave differently
> if you press Enter inside a <p>.

In composing window of Tb, user can select which block element should be around the caret. If I choose "Body Text", pressing Enter key causes a <br> element. So, this bug fix would change this behavior.

> > I wonder, what should occur when user presses Enter key in |<div
> > contenteditable>foo<br>bar</div>|? Even if web page author expected to use
> > <br> for a line breaker, we'd change the behavior? In such web apps, we're
> > now inserting <br>. Therefore, if we would start to insert <p> in them, they
> > would include two styles line breakers. This must be bad when this bug is
> > fixed while they edit such data across sessions.
> 
> This is already a problem if they edit from different browsers, or if two
> users edit the same content using different browsers.

Oh, indeed!

> I don't think there's
> any good way around it, because we have no way of distinguishing a <br> that
> was inserted because the user pressed Enter (should change) and one they
> deliberately inserted using Shift-Enter (should remain a <br>, e.g., in list
> items).  Also, then you have to figure out what br's are invisible, and I
> don't think we do that reliably.
> 
> So I think we should switch to <div> as default to match all other UAs,
> although IMO <p> is actually better.

I agree.

> This will require some changes in line
> break handling in <div>.  Generally I don't think it's a good idea to spend
> resources making significant editor changes these days, because so few sites
> use most of it (AFAIK) and we're likely to break things, but this is a
> pretty major issue (relevant every time the user hits Enter) and all the
> other browsers have converged.

One of important things is, contenteditable is used by complicated web apps such as Facebook, Twitter, Gmail, Google Docs, Outlook, etc which are very important web services for most users. As you know, our market share is not big, so, I think that we should make compatibility with other browsers better as far as possible since if Gecko works well without tests, our users won't be banned from any web apps. On the other hand, existing web apps which support Firefox might be broken if our behavior will be changed. So, when we change the behavior, we need to be careful and may need to contact them before release.

> The current functionality of outputting <br>
> will be accessible via a non-standard "br" value for
> defaultParagraphSeparator, in case that's the easiest way for any sites we
> break to keep working.  Do you agree?

Did you mean that when web apps want same behavior as other browsers, they should initialize defaultParagraphSeparator by themselves? If so, I don't think that it's good idea. As I said above, the default behavior should be same as other browsers and it might be better to support current behavior with setting defaultParagraphSeparator to "br".

(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #3)
> I don't know what I did wrong, but it seems Edge does still produce <p>.  So
> the question is, do we want to match Edge's default, or Blink/WebKit?  When
> this was discussed a few years ago, I was told that WebKit (no Blink yet
> IIRC) would not change its default, because there was a ton of
> WebKit-specific content that they didn't want to break.  The Blink/WebKit
> behavior is likely to be more compatible, I think, because it has no margin
> by default, so it looks like our current output.  But Edge is definitely
> more correct.

I think that basically, we should prefer the compatibility with Chromium as far as possible due to current market share. Although, as you said, in semantics, using <p> is really better. However, <p> has default margin that may break existing layout. Additionally, <p> cannot have another block level element. So, I feel that using <div> is not so bad approach.

> Whatever you choose, I'll update the spec/tests to match (unless someone
> disagrees).  If we go with <div>, Edge is outnumbered 3-to-1.  If we go with
> <p>, it's an even split and <p> is more semantically correct and what the
> spec already said.

I think that if all spec editors agree with using <p> as default, we should follow it, otherwise, we should use <div> for compatibility with Chromium.

(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #4)
> I put a lot of work into this, but on consideration, I don't think it's
> worth the effort to finish it off and get it working, unless you disagree. 
> Practically speaking, I don't think any major sites use contenteditable
> without overriding the behavior for hitting Enter etc., and the sites that
> do use it already deal with our current behavior somehow, and in fact might
> break if we change.  So basically this type of thing would just be for
> Thunderbird.

Yes, if we would break existing web apps, we need to give up or contact them after landing but before release.

Ideally, contenteditable should work similarly in all browsers without overriding its behavior because it doesn't make sense to keep current chaos. And I think that such overriding code makes current editor slower and reducing battery in any web apps. So, if it's possible, I really hope that the behavior should be standardized.

> So maybe we should mark this type of bug from now on as WONTFIX.  People
> should be using JS libraries, not the built-in functionality.  Instead, we
> should work on bugs to do with caret positioning or other things that
> real-world editing apps are interested in.

I think that we should not mark this as WONTFIX for now even if we cannot fix, instead, leaving open is enough.

The prioritizing what bugs are important is up to you.

Comment 7

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> One of important things is, contenteditable is used by complicated web apps
> such as Facebook, Twitter, Gmail, Google Docs, Outlook, etc which are very
> important web services for most users.

I'm under the impression that all of these sites override our behavior in handling user input, and don't use execCommand.  I think some might not even use our caret handling etc.

> Ideally, contenteditable should work similarly in all browsers without
> overriding its behavior because it doesn't make sense to keep current chaos.
> And I think that such overriding code makes current editor slower and
> reducing battery in any web apps. So, if it's possible, I really hope that
> the behavior should be standardized.

I think this will never happen.  But if you want to keep accepting patches for this type of thing, that's up to you.  If anyone is interested in taking up this particular bug, I can post the patches, but I don't think I'll work on them any more.

Comment 8

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

(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #7)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> > Ideally, contenteditable should work similarly in all browsers without
> > overriding its behavior because it doesn't make sense to keep current chaos.
> > And I think that such overriding code makes current editor slower and
> > reducing battery in any web apps. So, if it's possible, I really hope that
> > the behavior should be standardized.
> 
> I think this will never happen.  But if you want to keep accepting patches
> for this type of thing, that's up to you.  If anyone is interested in taking
> up this particular bug, I can post the patches, but I don't think I'll work
> on them any more.

That's fine. I just think that we shouldn't close such difficult bug as WONTFIX only with a few people's discussion.

Comment 9

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

Attachment: Bug 1297414 - Fix incorrect text expectation;

These expectations were just wrong.  I don't know how they got here.  I
think Chrome matches the tests and not the spec, fortunately.

Review commit: https://reviewboard.mozilla.org/r/125084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125084/

Comment 10

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

Attachment: Bug 1297414 - Send eTypedBR in plaintext mode too;

This is what test_htmleditor_keyevent_handling.html expects.  It
historically made no difference in the case tested there, because we
inserted a <br> either way, but now it will make a difference.  I don't
know what an HTML editor in plaintext mode even means, so I don't know
if this change is actually the intended behavior.

Review commit: https://reviewboard.mozilla.org/r/125086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125086/

Comment 11

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

Attachment: Bug 1297414 - Support execCommand("defaultParagraphSeparator");

Later commits will add the actual functionality.

Review commit: https://reviewboard.mozilla.org/r/125088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125088/

Comment 12

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

Attachment: Bug 1297414 - Treat <div> like <p> for break insertion;

I don't personally agree with this behavior (although I did spec it some
years ago), but it's the behavior of all other UAs, so we should do it
anyway.

Review commit: https://reviewboard.mozilla.org/r/125090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125090/

Comment 13

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

Attachment: Bug 1297414 - formatBlock should move non-editable nodes;

Otherwise if you formatBlock on "a<span
contenteditable=false>b</span>c", it will look like "acb".  It's fine to
move non-editable nodes to a different parent, it's only a problem to
move the child of a non-editable node (even if that child is itself
editable).

On the way, I fixed an exception in insert*List if there was an
uneditable inline node around.  I don't intend to fix all the todo's in
the test, but now it should have better coverage, at least.

Review commit: https://reviewboard.mozilla.org/r/125092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125092/

Comment 14

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

Attachment: Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

*aHandled was previously always set to true in WillMakeBasicBlock, which
was probably a bug but is not addressed in this commit.

Review commit: https://reviewboard.mozilla.org/r/125094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125094/

Comment 15

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

Attachment: Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

Review commit: https://reviewboard.mozilla.org/r/125096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125096/

Comment 16

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

Attachment: Bug 1297414 - Change default paragraph separator to <div>;

This matches Blink/WebKit, and is more similar to Edge than before, but
may cause compat problems for Gecko-only code or code paths.  Sites can
revert to old behavior with:
document.execCommand("defaultparagraphseparator", false, "br").

This regresses test_bug430392.html on one test ("adding returns") and I
don't know why.  The test involves embedded non-editable content and we
already have a lot of todos in that file, so I think it's tolerable.

Review commit: https://reviewboard.mozilla.org/r/125098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125098/

Comment 17

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

Comment on attachment 8852940 [details]
Bug 1297414 - Fix incorrect text expectation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125084/diff/1-2/

Comment 18

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

Comment on attachment 8852941 [details]
Bug 1297414 - Send eTypedBR in plaintext mode too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125086/diff/1-2/

Comment 19

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125088/diff/1-2/

Comment 20

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

Comment on attachment 8852943 [details]
Bug 1297414 - Treat <div> like <p> for break insertion;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125090/diff/1-2/

Comment 21

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125092/diff/1-2/

Comment 22

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125094/diff/1-2/

Comment 23

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

Comment on attachment 8852946 [details]
Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125096/diff/1-2/

Comment 24

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125098/diff/1-2/

Comment 25

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

Comment on attachment 8852940 [details]
Bug 1297414 - Fix incorrect text expectation;

https://reviewboard.mozilla.org/r/125084/#review127888

This is really hard to understand, I think that this and editing/include/tests.js should be rewritten with JSON style.

Comment 26

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

Comment on attachment 8852941 [details]
Bug 1297414 - Send eTypedBR in plaintext mode too;

https://reviewboard.mozilla.org/r/125086/#review127890

::: commit-message-ce879:5
(Diff revision 2)
> +inserted a <br> either way, but now it will make a difference.  I don't
> +know what an HTML editor in plaintext mode even means, so I don't know
> +if this change is actually the intended behavior.

It's used in e.g., Thunderbird's composer for plaintext email.

I.e., editing text is like as plaintext editor. Without text styles, just break each line at pressing Enter without margin, etc. However, it needs styled text for some reasons, e.g., quoted text.

Comment 27

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

https://reviewboard.mozilla.org/r/125088/#review127892

::: dom/html/nsHTMLDocument.cpp:3000
(Diff revision 2)
>  static const struct MidasCommand gMidasCommandTable[] = {
>    { "bold",          "cmd_bold",            "", true,  false },
>    { "italic",        "cmd_italic",          "", true,  false },
>    { "underline",     "cmd_underline",       "", true,  false },
>    { "strikethrough", "cmd_strikethrough",   "", true,  false },
>    { "subscript",     "cmd_subscript",       "", true,  false },
>    { "superscript",   "cmd_superscript",     "", true,  false },
>    { "cut",           "cmd_cut",             "", true,  false },
>    { "copy",          "cmd_copy",            "", true,  false },
>    { "paste",         "cmd_paste",           "", true,  false },
>    { "delete",        "cmd_deleteCharBackward", "", true,  false },
>    { "forwarddelete", "cmd_deleteCharForward", "", true,  false },
>    { "selectall",     "cmd_selectAll",       "", true,  false },
>    { "undo",          "cmd_undo",            "", true,  false },
>    { "redo",          "cmd_redo",            "", true,  false },
>    { "indent",        "cmd_indent",          "", true,  false },
>    { "outdent",       "cmd_outdent",         "", true,  false },
>    { "backcolor",     "cmd_highlight",       "", false, false },
>    { "forecolor",     "cmd_fontColor",       "", false, false },
>    { "hilitecolor",   "cmd_highlight",       "", false, false },
>    { "fontname",      "cmd_fontFace",        "", false, false },
>    { "fontsize",      "cmd_fontSize",        "", false, false },
>    { "increasefontsize", "cmd_increaseFont", "", false, false },
>    { "decreasefontsize", "cmd_decreaseFont", "", false, false },
>    { "inserthorizontalrule", "cmd_insertHR", "", true,  false },
>    { "createlink",    "cmd_insertLinkNoUI",  "", false, false },
>    { "insertimage",   "cmd_insertImageNoUI", "", false, false },
>    { "inserthtml",    "cmd_insertHTML",      "", false, false },
>    { "inserttext",    "cmd_insertText",      "", false, false },
>    { "gethtml",       "cmd_getContents",     "", false, false },
>    { "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_insertParagraph", "", true,  false },
>    { "insertlinebreak", "cmd_insertLineBreak", "", true,  false },
>    { "formatblock",   "cmd_paragraphState",  "", false, false },
>    { "heading",       "cmd_paragraphState",  "", false, false },
>    { "styleWithCSS",  "cmd_setDocumentUseCSS", "", false, true },
>    { "contentReadOnly", "cmd_setDocumentReadOnly", "", false, true },
>    { "insertBrOnReturn", "cmd_insertBrOnReturn", "", false, true },
> +  { "defaultParagraphSeparator", "cmd_defaultParagraphSeparator", "", false, false },

Not a scope of this bug, I think that we should sort this into A to Z order because we need to check if there is same command already with "Find" or something.

::: editor/composer/moz.build:28
(Diff revision 2)
> +LOCAL_INCLUDES += [
> +    '/dom/base',
> +]

I don't understand this change. Why do you need this in this patch? You added new include in this patch is only "mozilla/HTMLEditor.h". It shouldn't require this.

::: editor/composer/nsComposerDocumentCommands.cpp:31
(Diff revision 2)
>  #include "nsISupportsUtils.h"           // for NS_IF_ADDREF
>  #include "nsIURI.h"                     // for nsIURI
>  #include "nsPresContext.h"              // for nsPresContext
>  #include "nscore.h"                     // for NS_IMETHODIMP, nsresult, etc
>  
> +using namespace mozilla;

Sigh, I should move renaming classes to mozilla name space, later.

::: editor/composer/nsComposerDocumentCommands.cpp:293
(Diff revision 2)
> +      // Mozilla extension for backwards compatibility
> +      htmlEditor->SetDefaultParagraphSeparator(ParagraphSeparator::br);
> +      return NS_OK;
> +    }
> +
> +    MOZ_ASSERT_UNREACHABLE("ExecCommand() passed an invalid value");

Why do we need to crash in debug build?

::: editor/libeditor/HTMLEditor.h:64
(Diff revision 2)
>  } // namespace dom
>  namespace widget {
>  struct IMEState;
>  } // namespace widget
>  
> +enum class ParagraphSeparator { div, p, br };

Hmm, this doesn't conform to our coding rules, but eDiv, eP, eBr are ugly, and this is enum class. So, they need "ParagraphSeparator::" prefix. Must be fine...

::: editor/libeditor/HTMLEditor.h:378
(Diff revision 2)
>      return attrCount > 1 ||
>             (1 == attrCount &&
>              !aElement->GetAttrNameAt(0)->Equals(nsGkAtoms::mozdirty));
>    }
>  
> +  ParagraphSeparator GetDefaultParagraphSeparator() {

nit: Put "{" to the next line.

And why don't you make this method |const|?

::: editor/libeditor/HTMLEditor.h:381
(Diff revision 2)
>    }
>  
> +  ParagraphSeparator GetDefaultParagraphSeparator() {
> +    return mDefaultParagraphSeparator;
> +  }
> +  void SetDefaultParagraphSeparator(ParagraphSeparator aSep) {

nit: put the |{| to the next line.

Comment 28

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

Comment on attachment 8852943 [details]
Bug 1297414 - Treat <div> like <p> for break insertion;

https://reviewboard.mozilla.org/r/125090/#review127898

::: editor/libeditor/HTMLEditRules.cpp:1559
(Diff revision 2)
> -  } else if (blockParent->IsHTMLElement(nsGkAtoms::p)) {
> +  } else if (blockParent->IsHTMLElement(nsGkAtoms::p) ||
> +             blockParent->IsHTMLElement(nsGkAtoms::div)) {

Why don't you use |IsAnyOfHTMLElements()|?

Comment 29

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review127900

I feel odd to move non-editable nodes to the last of editing host, though.

Which spec documents this behavior? Anyway, I'd be happy if you separate this fix to another bug for easier to check regression point. Or, do we need to land this with the other patches?

::: editor/libeditor/tests/test_bug430392.html:57
(Diff revision 2)
> +    ["formatblock", "p"],
> +  ]
> +  .forEach(item => {
> +      var cmd = Array.isArray(item) ? item[0] : item;
> +      var param = Array.isArray(item) ? item[1] : "";
> +      tests.push([cmd, () => { document.execCommand(cmd, false, param) }]);

Where is the expected result?

Comment 30

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

https://reviewboard.mozilla.org/r/125094/#review127906

::: editor/libeditor/HTMLEditRules.cpp:1536
(Diff revision 2)
> +      // The nodes that can contain p and div are the same.  If the editing
> +      // host is a <p> or similar, we have to just insert a newline.  Special
> +      // case for lists, which can contain block elements as grandchildren.
> +      // TODO Any other exceptions?  We need a "can contain as descendant"
> +      // function.

If it's <pre>, it seems that we shouldn't use neither <p>, <div> nor <br> for line breaker because user might expect raw line breaker, i.e., \n, in such case.

How about table, tbody, thead, tfoot, tr?

::: editor/libeditor/HTMLEditRules.cpp:1542
(Diff revision 2)
> +       !host->IsHTMLElement(nsGkAtoms::ol) &&
> +       !host->IsHTMLElement(nsGkAtoms::ul) &&
> +       !host->IsHTMLElement(nsGkAtoms::dl)) ||

nit: Why don't you use IsAnyOfHTMLElements()?

::: editor/libeditor/HTMLEditRules.cpp:1555
(Diff revision 2)
> +    nsresult rv = MakeBasicBlock(aSelection,
> +                                 separator == ParagraphSeparator::div
> +                                 ? *nsGkAtoms::div : *nsGkAtoms::p);

Might be better to add a method to return element atom for ParagraphSeparator, but up to you.

::: editor/libeditor/HTMLEditRules.cpp:1560
(Diff revision 2)
> +    nsresult rv = MakeBasicBlock(aSelection,
> +                                 separator == ParagraphSeparator::div
> +                                 ? *nsGkAtoms::div : *nsGkAtoms::p);
> +    // We warn on failure, but don't handle it, because it might be harmless.
> +    // Instead we just check that a new block was actually created.
> +    Unused << NS_WARN_IF(NS_FAILED(rv));

Why don't you use NS_WARNING_ASSERTION()?

::: editor/libeditor/HTMLEditRules.cpp:1565
(Diff revision 2)
> +    if (NS_WARN_IF(blockParent == host)) {
> +      // Didn't create a new block for some reason, fall back to <br>
> +      rv = StandardBreakImpl(node, offset, aSelection);

Well, if selection is, e.g., [host, 1]-[host, 1], and typing Enter, should we not insert <div> or <p> in this case?

::: editor/libeditor/HTMLEditRules.cpp:3528
(Diff revision 2)
>    // We want to ignore result of WillInsert()
>    *aCancel = false;
> -  *aHandled = false;
> +  *aHandled = true;
> +
> +  nsresult rv = MakeBasicBlock(aSelection, blockType);
> +  Unused << NS_WARN_IF(NS_FAILED(rv));

Why don't you use NS_WARNING_ASSERTION()?

Comment 31

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

Comment on attachment 8852946 [details]
Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

https://reviewboard.mozilla.org/r/125096/#review127910

::: editor/libeditor/HTMLEditRules.cpp:6306
(Diff revision 2)
> +nsIAtom&
> +HTMLEditRules::DefaultParagraphSeparator()
> +{
> +  MOZ_ASSERT(mHTMLEditor);
> +  if (!mHTMLEditor) {
> +    return *nsGkAtoms::div;
> +  }
> +  switch (mHTMLEditor->GetDefaultParagraphSeparator()) {
> +    default:
> +      MOZ_FALLTHROUGH_ASSERT("Unexpected paragraph separator!");
> +
> +    case ParagraphSeparator::div:
> +      return *nsGkAtoms::div;
> +
> +    case ParagraphSeparator::p:
> +      return *nsGkAtoms::p;
> +
> +    case ParagraphSeparator::br:
> +      return *nsGkAtoms::p;
> +  }
> +}

So, like I mentioned in previous patch, there should be static nsIAtom& ParagraphSeparatorElement(ParagraphSeparator&) or something. Then, this method can use it.

Comment 32

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

https://reviewboard.mozilla.org/r/125098/#review127916

::: commit-message-fd4f7:3
(Diff revision 2)
> +This matches Blink/WebKit, and is more similar to Edge than before, but
> +may cause compat problems for Gecko-only code or code paths.  Sites can
> +revert to old behavior with:
> +document.execCommand("defaultparagraphseparator", false, "br").

I think that you should post "Intent to change" mail to dev-platform.

And it might be better to land this on different day from another bug for easier to check regression point. Either is okay to me.

::: editor/libeditor/tests/test_bug430392.html:40
(Diff revision 2)
> -  }], ["adding shift-returns", () => {
> +  }, " A; B ; C "],
> +  ["adding shift-returns", () => {

Oh, I misunderstood something when the expected result is undefined.

Comment 33

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

So, it might be better to file two bugs, one blocks this bug and is for landing "Bug 1297414 - formatBlock should move non-editable nodes;", the other depends on this bug and is for landing the last patch.

Comment 34

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

https://reviewboard.mozilla.org/r/125088/#review127892

> I don't understand this change. Why do you need this in this patch? You added new include in this patch is only "mozilla/HTMLEditor.h". It shouldn't require this.

0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/editor/composer/nsComposerDocumentCommands.cpp:7:
 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/HTMLEditor.h:12:
 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/TextEditor.h:9:
 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/EditorBase.h:15:
 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/dom/Text.h:10:
 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/nsGenericDOMDataNode.h:24:
 0:15.39 /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/dom/ShadowRoot.h:17:10: fatal error: 'nsDocument.h' file not found
 0:15.39 #include "nsDocument.h"
 0:15.39          ^
 0:15.97 1 error generated.

I added a comment to the moz.build explaining why we need it.

> Why do we need to crash in debug build?

The code should be unreachable, because nsHTMLDocument::ExecCommand already validates the parameter.  If we get here, it's a logic error and should be debugged.  Unless you're concerned that these methods might be called by callers other than ExecCommand?

> nit: Put "{" to the next line.
> 
> And why don't you make this method |const|?

I thought our style was that inline methods have the curly brace on the same line, but it seems not.

I don't usually bother marking methods "const" in classes like HTMLEditor where I don't know of us using any const instances.  Do we have any "const HTMLEditor" in the code?  But if you want I can.

Comment 35

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

Comment on attachment 8852940 [details]
Bug 1297414 - Fix incorrect text expectation;

https://reviewboard.mozilla.org/r/125084/#review127888

I filed bug 1352813.

Comment 36

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review127900

We need to land this with the other patches to avoid test suite regressions.  It would be possible to avoid the regressions by switching back to defaultParagraphSeparator "br" for the affected test, but I preferred to fix the actual bug.

The nodes are not supposed to be moved to the end of the editing host.  The point is that we're creating a new wrapper, and the old code would refuse to move the non-editable nodes from the old wrapper to the new wrapper.  So it would move the editable nodes to the front (to put them in the new parent) and leave the non-editable nodes alone (at the end).  This is wrong -- non-editable nodes with editable parent can be moved to a different parent, so they should be moved as well, preserving the order of the nodes.

This bug triggered failures in tests because formerly, we would just insert a <br> and not change the parents.  Now we'll add a <div> parent, so behavior will regress unless this bug is fixed first.

I'll update the commit message with more info.

> Where is the expected result?

As it says in the comment, "callback() is called and we test that the textContent didn't change."  An expected result is only provided for expected failures, where the textContent currently does change.  Do you want me to do anything to make this clearer in the test?

Comment 37

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

https://reviewboard.mozilla.org/r/125094/#review127906

> If it's <pre>, it seems that we shouldn't use neither <p>, <div> nor <br> for line breaker because user might expect raw line breaker, i.e., \n, in such case.
> 
> How about table, tbody, thead, tfoot, tr?

It looks like we currently insert <br> in <pre>, and this patch shouldn't change that.  I don't know if this is the best behavior, but it seems out of scope for this bug.

table/tbody/thead/tfoot/tr should be exceptions as well, you're right, in case someone makes one of them editable.  I'll update the patch.  (I usually don't worry too much about editable elements that aren't divs, because our code is so broken in those cases anyway . . .)

> Why don't you use NS_WARNING_ASSERTION()?

Because that requires a message and I don't want to give one.  The "Unused << " is ugly, though.  I filed bug 1352816 to allow NS_WARNING_ASSERTION(NS_FAILED(rv)) with no further description.  For now, I think this is okay, because I get 171 hits in the tree for "Unused << NS_WARN_IF", and 340 for "NS_WARNING_ASSERTION(".  But if you want I can change it -- let me know.

> Well, if selection is, e.g., [host, 1]-[host, 1], and typing Enter, should we not insert <div> or <p> in this case?

I don't understand.  Could you elaborate, with an example of the exact markup you're thinking of and actual/expected results?

Comment 38

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

https://reviewboard.mozilla.org/r/125098/#review127916

> I think that you should post "Intent to change" mail to dev-platform.
> 
> And it might be better to land this on different day from another bug for easier to check regression point. Either is okay to me.

I posted an intent to change to dev-platform.  If you want, I can push this final commit in a separate push from the others, to ease mozregression.

Comment 39

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125088/diff/2-3/

Comment 40

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

Comment on attachment 8852943 [details]
Bug 1297414 - Treat <div> like <p> for break insertion;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125090/diff/2-3/

Comment 41

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125092/diff/2-3/

Comment 42

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125094/diff/2-3/

Comment 43

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

Comment on attachment 8852946 [details]
Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125096/diff/2-3/

Comment 44

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125098/diff/2-3/

Comment 45

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

(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #34)
> > Why do we need to crash in debug build?
> 
> The code should be unreachable, because nsHTMLDocument::ExecCommand already
> validates the parameter.  If we get here, it's a logic error and should be
> debugged.  Unless you're concerned that these methods might be called by
> callers other than ExecCommand?

I don't see any way in MozReview to flag something as being blocked on you, so I'm marking the bug needinfo to make sure you respond to this point (in addition to the ones you didn't give r+ to).

Comment 46

[Jorg K (CEST = GMT+2)]

Hmm, I came to this bug rather late after the dev-platform announcement. I hope Aryeh will be around to fix any issues we might see after this lands.

I have a few comments, and sorry for not reading everything in great detail.

1) Breaking <p>foobar</p> will still result in <p>foo</p><p>bar</p>, I suppose.
2) Serialising foo<br>bar to text will give foo\nbar, just as <div>foo</div><div>bar</div>
   will, I hope(?).
3) You're aware of HTMLEditRules::SplitMailCites(). I put some tricks in there recently:
   http://searchfox.org/mozilla-central/rev/fd99701cafa324669d950ced902d08a7450cd48b/editor/libeditor/HTMLEditRules.cpp#1728
   But that's covered by a test.
4) In Thunderbird we use
   editor.returnInParagraphCreatesNewParagraph = Services.prefs.getBoolPref("editor.CR_creates_new_p");
   so the nsIHTMLEditor::returnInParagraphCreatesNewParagraph.
   I don't see this mentioned here, so I assume that will just remain working as it was.
5) TB currently doesn't use cmd_insertBrOnReturn. How do I need to interpret comment #2. If we were to
   use this to maintain insertion <br>, would a paragraph than not be split any more, see point 1)?
6) So |document.execCommand("defaultparagraphseparator", false, "br")| is new? I also see:
   htmlEditor->SetDefaultParagraphSeparator(ParagraphSeparator::br);
   Is that also an interface accessible by JS like 4)?
7) Finally, we have bug 1271835 on file where people want to insert paragraphs instead of
   <br> when splitting blockquotes. Could that be achieved with a combination of the settings here?

Comment 47

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

(In reply to Jorg K (GMT+2) from comment #46)
> 1) Breaking <p>foobar</p> will still result in <p>foo</p><p>bar</p>, I
> suppose.

It should, yes.

> 2) Serialising foo<br>bar to text will give foo\nbar, just as
> <div>foo</div><div>bar</div>
>    will, I hope(?).

I didn't change serialization.

> 3) You're aware of HTMLEditRules::SplitMailCites(). I put some tricks in
> there recently:
>   
> http://searchfox.org/mozilla-central/rev/
> fd99701cafa324669d950ced902d08a7450cd48b/editor/libeditor/HTMLEditRules.
> cpp#1728
>    But that's covered by a test.

No tests regressed due to this patch set, except one minor regression as noted in the commit message.  If SplitMailCites isn't needed for Firefox, though, IIUC I'm not supposed to spend time looking at it.

> 4) In Thunderbird we use
>    editor.returnInParagraphCreatesNewParagraph =
> Services.prefs.getBoolPref("editor.CR_creates_new_p");
>    so the nsIHTMLEditor::returnInParagraphCreatesNewParagraph.
>    I don't see this mentioned here, so I assume that will just remain
> working as it was.

I don't know, I didn't test it.

> 5) TB currently doesn't use cmd_insertBrOnReturn. How do I need to interpret
> comment #2. If we were to
>    use this to maintain insertion <br>, would a paragraph than not be split
> any more, see point 1)?

I don't know anything about cmd_insertBrOnReturn.

> 6) So |document.execCommand("defaultparagraphseparator", false, "br")| is
> new? I also see:
>    htmlEditor->SetDefaultParagraphSeparator(ParagraphSeparator::br);
>    Is that also an interface accessible by JS like 4)?

Yes to the first.  It's specced:

https://dvcs.w3.org/hg/editing/raw-file/22257987e0be/editing.html#the-defaultparagraphseparator-command

We follow the spec except that we also support "br" for backwards compatibility.  Web pages can change the separator with document.execCommand("defaultparagraphseparator", false, "br") (or "p" or "div").

I assume that SetDefaultParagraphSeparator is not separately accessible from JS.  I'm only writing this for web APIs.  If Thunderbird still wants <br>, they can get it with execCommand().

> 7) Finally, we have bug 1271835 on file where people want to insert
> paragraphs instead of
>    <br> when splitting blockquotes. Could that be achieved with a
> combination of the settings here?

I didn't understand the bug at a quick read.  It would help if you gave a set of steps to reproduce, expected behavior, and actual behavior.

Comment 48

[Jorg K (CEST = GMT+2)]

Thanks for the quick reply.

(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #47)
> > 2) Serialising foo<br>bar to text will give foo\nbar, just as
> > <div>foo</div><div>bar</div>
> >    will, I hope(?).
> I didn't change serialization.
So let's just hope it works for <div>.

> I don't know anything about cmd_insertBrOnReturn.
Well, you wrote something about it in comment #2.

> > 7) Finally, we have bug 1271835 on file where people want to insert
> > paragraphs instead of
> >    <br> when splitting blockquotes. Could that be achieved with a
> > combination of the settings here?
> I didn't understand the bug at a quick read.  It would help if you gave a
> set of steps to reproduce, expected behavior, and actual behavior.
Never mind, it's not a bug, it's an enhancement request. Basically in TB we can select whether we want to compose using paragraphs or "body text", which inserts <br>. The request is when using paragraphs, splitting a blockquote should insert a paragraph. Since that's done in HTMLEditRules::SplitMailCites(), it's not affected by the changes here. However, I could drill open that function and react to nsIHTMLEditor::GetDefaultParagraphSeparator().

We might end up calling |document.execCommand("defaultparagraphseparator", false, "br/p")| depending on whether we're in paragraph or "body text" mode when initialising our compose window.

Comment 49

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

https://reviewboard.mozilla.org/r/125088/#review127892

> 0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/editor/composer/nsComposerDocumentCommands.cpp:7:
>  0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/HTMLEditor.h:12:
>  0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/TextEditor.h:9:
>  0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/EditorBase.h:15:
>  0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/dom/Text.h:10:
>  0:15.39 In file included from /mnt/ssd/checkouts/gecko-dev/obj/dist/include/nsGenericDOMDataNode.h:24:
>  0:15.39 /mnt/ssd/checkouts/gecko-dev/obj/dist/include/mozilla/dom/ShadowRoot.h:17:10: fatal error: 'nsDocument.h' file not found
>  0:15.39 #include "nsDocument.h"
>  0:15.39          ^
>  0:15.97 1 error generated.
> 
> I added a comment to the moz.build explaining why we need it.

Okay, thanks. Although, we should fix such include hell in another bug.

> The code should be unreachable, because nsHTMLDocument::ExecCommand already validates the parameter.  If we get here, it's a logic error and should be debugged.  Unless you're concerned that these methods might be called by callers other than ExecCommand?

Although, I don't find the checker in nsHTMLDocument::ExecCommand(), commands can be executed without nsHTMLDocument::ExecCommand() from XUL or chrome script. If some XUL add-ons implement such feature, crashing only on debug build isn't good thing. So, please take same behavior as opt build here.

> I thought our style was that inline methods have the curly brace on the same line, but it seems not.
> 
> I don't usually bother marking methods "const" in classes like HTMLEditor where I don't know of us using any const instances.  Do we have any "const HTMLEditor" in the code?  But if you want I can.

If inline method can be written in one line, braces can be put in same line. However, our 80 column per line rule easily prevent that like here :-(

Currently, each editor class isn't referred from outside of editor/ due to nsI*Editor not builtin classes. However, if we would make them builtin classes, it might be worthwhile to use |const *Editor*| reference. And also, I like better explicitly to indicate if each method may or may not change the class state, so, I'd be happy if you'd add |const| for such methods.

Comment 50

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

https://reviewboard.mozilla.org/r/125094/#review127906

> It looks like we currently insert <br> in <pre>, and this patch shouldn't change that.  I don't know if this is the best behavior, but it seems out of scope for this bug.
> 
> table/tbody/thead/tfoot/tr should be exceptions as well, you're right, in case someone makes one of them editable.  I'll update the patch.  (I usually don't worry too much about editable elements that aren't divs, because our code is so broken in those cases anyway . . .)

Yep, about <pre> issue shouldn't be changed in this bug anyway.

But it might be worthwhile to post it to the WG.

> Because that requires a message and I don't want to give one.  The "Unused << " is ugly, though.  I filed bug 1352816 to allow NS_WARNING_ASSERTION(NS_FAILED(rv)) with no further description.  For now, I think this is okay, because I get 171 hits in the tree for "Unused << NS_WARN_IF", and 340 for "NS_WARNING_ASSERTION(".  But if you want I can change it -- let me know.

Okay, fine.

> I don't understand.  Could you elaborate, with an example of the exact markup you're thinking of and actual/expected results?

Ah, I was thinking about following case, but I misunderstood.

> <div contenteditalbe><p>foo</p>[]<p>bar</p></div>

The code is a fallback when it fails to create <p> or <div> here. So, fine.

Comment 51

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

https://reviewboard.mozilla.org/r/125094/#review128350

Comment 52

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review127900

> We need to land this with the other patches to avoid test suite regressions.  It would be possible to avoid the regressions
> by switching back to defaultParagraphSeparator "br" for the affected test, but I preferred to fix the actual bug.

I meant that only this patch would be landed before landing this bug. However, if it causes a lot of fix in automated tests, I'm fine to land with the other patches.

And thank you for your explanation, I'll review this again with the information.

> As it says in the comment, "callback() is called and we test that the textContent didn't change."  An expected result is only provided for expected failures, where the textContent currently does change.  Do you want me to do anything to make this clearer in the test?

Ah, I see, but I'd be happy if you make the comment clearer. (about not specifying expected result means nothing should be changed.)

Comment 53

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review128414

::: editor/libeditor/HTMLEditRules.cpp:3225
(Diff revision 3)
>      // if curNode is a Break, delete it, and quit remembering prev list item
>      if (TextEditUtils::IsBreak(curNode)) {
>        NS_ENSURE_STATE(mHTMLEditor);
>        rv = mHTMLEditor->DeleteNode(curNode);
>        NS_ENSURE_SUCCESS(rv, rv);
>        prevListItem = nullptr;
>        continue;
> -    } else if (IsEmptyInline(curNode)) {
> +    } else if (IsEmptyInline(curNode) && mHTMLEditor &&
> +               mHTMLEditor->IsEditable(curNode)) {
>        // if curNode is an empty inline container, delete it
>        NS_ENSURE_STATE(mHTMLEditor);
>        rv = mHTMLEditor->DeleteNode(curNode);
>        NS_ENSURE_SUCCESS(rv, rv);
>        continue;
>      }

I don't understand why we need to check if it's editable only before removing empty inline element but not so for <br>. Could you add to explain about this?

::: editor/libeditor/HTMLEditRules.cpp:6744
(Diff revision 3)
>  /**
>   * RemoveBlockStyle() makes the nodes have no special block type.
>   */
>  nsresult
>  HTMLEditRules::RemoveBlockStyle(nsTArray<OwningNonNull<nsINode>>& aNodeArray)

Sigh, I'm really really confused by this method what it tries to do... A lot of methods around HTML editor are unclear to me what are their expected result...

::: editor/libeditor/HTMLEditRules.cpp:6760
(Diff revision 3)
>      if (HTMLEditUtils::IsFormatNode(curNode)) {
>        // Process any partial progress saved
>        if (curBlock) {
>          nsresult rv = RemovePartOfBlock(*curBlock, *firstNode, *lastNode);
>          NS_ENSURE_SUCCESS(rv, rv);
>          firstNode = lastNode = curBlock = nullptr;
>        }
> +      if (!mHTMLEditor->IsEditable(curNode)) {
> +        continue;
> +      }

If a formart block is not editable, giving up to remove the block, sounds like make sense...

At this time, curBlock is previous common block parent which are listed in aNodeArray and all of them are inline. So, not checking if curBlock is editable might be okay...

::: editor/libeditor/HTMLEditRules.cpp:6811
(Diff revision 3)
>        curBlock = htmlEditor->GetBlockNodeParent(curNode);
>        if (curBlock && HTMLEditUtils::IsFormatNode(curBlock)) {
>          firstNode = lastNode = curNode->AsContent();
>        } else {
>          // Not a block kind that we care about.
>          curBlock = nullptr;
>        }

curBlock is stored only here. However, if it's a format block node, it will be removed with RemvoePartOfBlock() even if it's not editable. Is it okay to you?

::: editor/libeditor/HTMLEditor.cpp:3168
(Diff revision 3)
>  HTMLEditor::DeleteNode(nsIDOMNode* aNode)
>  {
>    // do nothing if the node is read-only
>    nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
> -  if (!IsModifiableNode(aNode) && !IsMozEditorBogusNode(content)) {
> +  if (NS_WARN_IF(!IsModifiableNode(aNode) && !IsMozEditorBogusNode(content))) {
>      return NS_ERROR_FAILURE;

Hmm, looks like this becomes just a spammer...

Looks like that a lot of callers don't check if it's editable. For example, HTMLEditor::ReplaceHeadContentsWithHTML() clears <head> element, but it must usually be not editable. EditorBase::RemoveContainer() uses DeleteNode() when all children is moved from a block to its parent. And HTMLEditRules::WillDeleteSelection() may remove following <br> which may be <br contenteditable="false"> (Although, I'm not sure if this is correct, https://jsfiddle.net/d_toybox/rn5z0e5L/).

Comment 54

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review128414

> Sigh, I'm really really confused by this method what it tries to do... A lot of methods around HTML editor are unclear to me what are their expected result...

I think it tries to get rid of things like <ol>, <ul>, <blockquote>, etc.

> If a formart block is not editable, giving up to remove the block, sounds like make sense...
> 
> At this time, curBlock is previous common block parent which are listed in aNodeArray and all of them are inline. So, not checking if curBlock is editable might be okay...

I think it's wrong, because you might have

  <p>Regular non-editable p <span contenteditable>editable</span></p>

or something, and it's wrong to remove the <span>.  So we should ignore non-editable blocks (treat them as no block).

> curBlock is stored only here. However, if it's a format block node, it will be removed with RemvoePartOfBlock() even if it's not editable. Is it okay to you?

No, you're right, and my change will regress some cases of non-editable content as-is, so I'll fix it.  Our handling of non-editable content is totally broken, but we still shouldn't regress it.

> Hmm, looks like this becomes just a spammer...
> 
> Looks like that a lot of callers don't check if it's editable. For example, HTMLEditor::ReplaceHeadContentsWithHTML() clears <head> element, but it must usually be not editable. EditorBase::RemoveContainer() uses DeleteNode() when all children is moved from a block to its parent. And HTMLEditRules::WillDeleteSelection() may remove following <br> which may be <br contenteditable="false"> (Although, I'm not sure if this is correct, https://jsfiddle.net/d_toybox/rn5z0e5L/).

All these should be checking for editability, or their callers should, and I think warnings are appropriate, because the code is buggy and should be fixed.  I don't think it will spam a lot, because non-editable content embedded in editable content isn't common, and we do have a lot of bugs when it does come up, so it's good to have warnings to look at when debugging so we can quickly spot what's going wrong.  Non-editable nodes need to be handled specially, not just NS_ENSURE_SUCCESS.

So I think we should keep the NS_WARN, and if we find it's too spammy, fix the callers that aren't checking for editability.  If you still disagree, though, I'll take it out of the patch.

Comment 55

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

https://reviewboard.mozilla.org/r/125088/#review127892

> Although, I don't find the checker in nsHTMLDocument::ExecCommand(), commands can be executed without nsHTMLDocument::ExecCommand() from XUL or chrome script. If some XUL add-ons implement such feature, crashing only on debug build isn't good thing. So, please take same behavior as opt build here.

Okay.  I replaced it with an NS_WARN, and a comment saying that it shouldn't be reachable from nsHTMLDocument::ExecCommand.  The check is here: https://reviewboard.mozilla.org/r/125088/diff/3#chunk1.6

Comment 56

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

https://reviewboard.mozilla.org/r/125094/#review127906

> Yep, about <pre> issue shouldn't be changed in this bug anyway.
> 
> But it might be worthwhile to post it to the WG.

What WG?  There's no maintained contenteditable spec right now.

Comment 57

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125088/diff/3-4/

Comment 58

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

Comment on attachment 8852943 [details]
Bug 1297414 - Treat <div> like <p> for break insertion;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125090/diff/3-4/

Comment 59

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125092/diff/3-4/

Comment 60

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125094/diff/3-4/

Comment 61

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

Comment on attachment 8852946 [details]
Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125096/diff/3-4/

Comment 62

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125098/diff/3-4/

Comment 63

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review128414

> All these should be checking for editability, or their callers should, and I think warnings are appropriate, because the code is buggy and should be fixed.  I don't think it will spam a lot, because non-editable content embedded in editable content isn't common, and we do have a lot of bugs when it does come up, so it's good to have warnings to look at when debugging so we can quickly spot what's going wrong.  Non-editable nodes need to be handled specially, not just NS_ENSURE_SUCCESS.
> 
> So I think we should keep the NS_WARN, and if we find it's too spammy, fix the callers that aren't checking for editability.  If you still disagree, though, I'll take it out of the patch.

Yeah, it's okay to keep it for now. But I think that if it causes a lot of warnings, we should fix the cause ASAP or just remove the warning temporarily.

Comment 64

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

https://reviewboard.mozilla.org/r/125092/#review128488

Comment 65

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

Comment on attachment 8852940 [details]
Bug 1297414 - Fix incorrect text expectation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125084/diff/2-3/

Comment 66

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

Comment on attachment 8852941 [details]
Bug 1297414 - Send eTypedBR in plaintext mode too;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125086/diff/2-3/

Comment 67

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

Comment on attachment 8852942 [details]
Bug 1297414 - Support execCommand("defaultParagraphSeparator");

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125088/diff/4-5/

Comment 68

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

Comment on attachment 8852943 [details]
Bug 1297414 - Treat <div> like <p> for break insertion;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125090/diff/4-5/

Comment 69

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

Comment on attachment 8852944 [details]
Bug 1297414 - formatBlock should move non-editable nodes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125092/diff/4-5/

Comment 70

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

Comment on attachment 8852945 [details]
Bug 1297414 - Support <div>/<p> separators in WillInsertBreak;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125094/diff/4-5/

Comment 71

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

Comment on attachment 8852946 [details]
Bug 1297414 - Support <div> separator in ReturnIn(Header|ListItem);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125096/diff/4-5/

Comment 72

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

Comment on attachment 8852947 [details]
Bug 1297414 - Change default paragraph separator to <div>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125098/diff/4-5/

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d52e9d683755
Fix incorrect text expectation; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dadefb7df55
Send eTypedBR in plaintext mode too; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b2a6f76d0f
Support execCommand("defaultParagraphSeparator"); r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f23591dd74
Treat <div> like <p> for break insertion; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d90b7d842d6
formatBlock should move non-editable nodes; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c26462fb440
Support <div>/<p> separators in WillInsertBreak; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d39d921ab42
Support <div> separator in ReturnIn(Header|ListItem); r=masayuki

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21eac8bd0645
Change default paragraph separator to <div>; r=masayuki

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d52e9d683755
https://hg.mozilla.org/mozilla-central/rev/3dadefb7df55
https://hg.mozilla.org/mozilla-central/rev/82b2a6f76d0f
https://hg.mozilla.org/mozilla-central/rev/d4f23591dd74
https://hg.mozilla.org/mozilla-central/rev/7d90b7d842d6
https://hg.mozilla.org/mozilla-central/rev/6c26462fb440
https://hg.mozilla.org/mozilla-central/rev/7d39d921ab42
https://hg.mozilla.org/mozilla-central/rev/21eac8bd0645

Comment 76

[Jorg K (CEST = GMT+2)]

Aryeh and Masuyuki-san, thanks a lot for your work here.

To restore Thunderbird to previous working order, especially "plain text" composition (which is of course also HTML composition but with |style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;"| which makes it look plain), I added
  execCommand("defaultparagraphseparator", false, "br");
in the initialisation of our compose window.

In TB we have a preference to either compose in "body text" mode or paragraph mode. We're looking into using
  execCommand("defaultparagraphseparator", false, "p");
when in paragraph mode, see attachment 8854727 [details] [diff] [review], if you're interested.

Personally I think we should not stick with the past but rather modernise, and this implementation gives us the opportunity.

Comment 77

[Jorg K (CEST = GMT+2)]

We've been playing with
  execCommand("defaultparagraphseparator", false, "p");
but after splitting a blockquote in TB-only HTMLEditRules::SplitMailCites() further <enter> strokes are ignored. One of our users has even shot a video, attachment 8854769 [details].

HTMLEditRules::SplitMailCites() when defaultparagraphseparator=="p" leaves us in a broken state, with this debug:

[6140] WARNING: 'blockParent == host', file c:/mozilla-source/comm-central/mozilla/editor/libeditor/HTMLEditRules.cpp, line 1577
[6140] WARNING: NS_ENSURE_TRUE(!disconnected) failed: file c:/mozilla-source/comm-central/mozilla/dom/base/nsRange.cpp, line 132
[6140] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80530004: file c:/mozilla-source/comm-central/mozilla/editor/libeditor/HTMLEditRules.cpp, line 7406

The HTML I have at that stage is:

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 2/13/15 6:46 AM, Bugzilla@Mozilla
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:[e-mail]%2F">
      <pre wrap="">This is one of the remaining </pre>
    </blockquote>
    <br>
    <blockquote type="cite"
      cite="mid:[e-mail]%2F">
      <pre wrap="">auto-complete regressions introduced in TB 31 and
IMHO it would be good to fix it for TB 38, hence the tracking flag.</pre>
    </blockquote>
    <p><br>
    </p>
  </body>
</html>

I've just split the blockquote which inserted the <br>.

New bug for that? Maybe hard to reproduce in FF alone.

Comment 78

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

(In reply to Jorg K (GMT+2) from comment #77)
> New bug for that? Maybe hard to reproduce in FF alone.

Could be written with mochitest(-chrome) with setting nsIEditor.flags.

# and canceling old ni?.

Comment 79

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] from comment #78)
> Could be written with mochitest(-chrome) with setting nsIEditor.flags.
Sure, but it's reproducible in FF alone, see bug 1353695.

Comment 80

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

I've assigned bug 1353695 to myself and will look at it today.

Comment 81

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/rich-text-editor-s-newline-behaviour-has-been-changed-now-generates-div-instead-of-br/

Comment 82

[Chris Mills [:cmills]]

I have had a go at documenting this.

Our original document on rich text editing in the browser was really out of date, so I've started adding more details to the following doc, so this is at least covered somewhere:
https://developer.mozilla.org/en-US/Firefox/Releases/55#HTML

I covered this particular point at:
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Editable_content#Differences_in_markup_generation

I will need some time to research what else needs to be written here; any pointers you might have would be very helpful ;-)

Finally, I added a note to the Fx55 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/55#HTML

Let me know if this looks OK, and what other details you think should be added. Thanks!

Comment 83

[Makoto Kato [:m_kato]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #82)
> I have had a go at documenting this.
> 
> Our original document on rich text editing in the browser was really out of
> date, so I've started adding more details to the following doc, so this is
> at least covered somewhere:
> https://developer.mozilla.org/en-US/Firefox/Releases/55#HTML
> 
> I covered this particular point at:
> https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/
> Editable_content#Differences_in_markup_generation
> 
> I will need some time to research what else needs to be written here; any
> pointers you might have would be very helpful ;-)
> 
> Finally, I added a note to the Fx55 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/55#HTML
> 
> Let me know if this looks OK, and what other details you think should be
> added. Thanks!

There is still no defaultParagraphSeparator entry in https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand.

Comment 84

[Makoto Kato [:m_kato]]

per comment #83.  Could you update https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand ?

Comment 85

[Chris Mills [:cmills]]

(In reply to Makoto Kato [:m_kato] (slow due to PTO, back at Jan) from comment #84)
> per comment #83.  Could you update
> https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand ?

Ooops, sorry — I missed this request. I've added it now:

https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

When did the defaultParagraphSeparator command actually start being supported? Does it need a separate line in the support table?

Comment 86

[Makoto Kato [:m_kato]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #85)
> (In reply to Makoto Kato [:m_kato] (slow due to PTO, back at Jan) from
> comment #84)
> > per comment #83.  Could you update
> > https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand ?
> 
> Ooops, sorry — I missed this request. I've added it now:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand
> 
> When did the defaultParagraphSeparator command actually start being
> supported? Does it need a separate line in the support table?

Firefox 55.  It is better to add support table.  WebKit and Blink are supported by https://bugs.webkit.org/show_bug.cgi?id=59961. And IE11+ supports it according to https://msdn.microsoft.com/en-us/library/hh801229%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396#DefaultParagraphSeparator.

Comment 87

[Chris Mills [:cmills]]

(In reply to Makoto Kato [:m_kato] from comment #86)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #85)
> > (In reply to Makoto Kato [:m_kato] (slow due to PTO, back at Jan) from
> > comment #84)
> > > per comment #83.  Could you update
> > > https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand ?
> > 
> > Ooops, sorry — I missed this request. I've added it now:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand
> > 
> > When did the defaultParagraphSeparator command actually start being
> > supported? Does it need a separate line in the support table?
> 
> Firefox 55.  It is better to add support table.  WebKit and Blink are
> supported by https://bugs.webkit.org/show_bug.cgi?id=59961. And IE11+
> supports it according to
> https://msdn.microsoft.com/en-us/library/hh801229%28v=vs.85%29.
> aspx?f=255&MSPPError=-2147217396#DefaultParagraphSeparator.

OK, added.