Bug 59961

Summary:

Implement DefaultParagraphSeparator execCommand, to let authors choose the default block element

Product:

WebKit

Reporter:

David Bloom <bloom>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

adele, annevk, ayg, darin, ehsan, enrica, kalman, leviw, ojan, pf, rniwa, webkit.review.bot, zcorpan

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

David Bloom

Reported 2011-05-02 14:23:36 PDT

Opera silently included a new execCommand in Opera 11.10, "opera-defaultBlock". It lets authors set the default block element for rich text editors on the page to either 'p' or 'div'. If other browsers adopt a similar execCommand, it could make it much easier for authors to get consistent rich text editing behavior that they want (without resorting to a hacks involving listening to keyboard events and mangling the DOM when the user presses [return]). Note that this only affects the default block, which is usually not used. For example, pressing [return] inside of <body contentEditable><p>foo[caret]</p></body> or <body contentEditable><div>foo[caret]</div></body> will always create another <p> or <div> respectively, regardless of the default block. For example, if the default block is set to <p>... <body contentEditable>foo[caret]</body> --> pressing [return] creates a <p> (default block is used) <body><div contentEditable>foo[caret]</div></body> --> pressing [return] creates a <p> (default block is used) <body contentEditable><div>foo[caret]</div></body> --> pressing [return] creates a <div> (default block is ignored) <body contentEditable><ol><li>foo</li><li>bar[caret]</li></ol></body> --> pressing [return] creates an <li> (default block is ignored) <bdoy contentEditable><ol><li>foo</li><li>[caret]</li></ol></body> --> pressing [return] creates a <p> (default block is used) Implementing this in WebKit could be as simple as adding a webkit-defaultBlock command that lets authors specify whether to create a 'p' or a 'div' here: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=85256#L831

Attachments
Patch (24.12 KB, patch) 2012-02-29 20:12 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Patch (18.00 KB, patch) 2012-03-01 14:33 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Patch (9.37 KB, patch) 2012-03-01 15:47 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Patch (17.41 KB, patch) 2012-03-01 16:09 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 2011-05-02 14:32:00 PDT

This is a great addition to the list of execCommands. It's one of the bits of contentEditable that rich-text libraries go through many contortions to get cross-browser compat. It's also an area where the "right" answer depends on the web page's needs.

Ryosuke Niwa

Comment 2 2011-05-02 14:41:25 PDT

It is, unfortunately, not that simple. There are many places in our code base where the paragraph separator is assumed to be a div. We'll have to go through editing code to find all those places and make sure to update them. What is the syntax like? execCommand("defaultBlock", false, "p"/"div")? Also, does Opera's implementation accept arbitrarily element other than p and div? I don't want to have to test this function with every single block element if possible.

David Bloom

Comment 3 2011-05-02 14:47:58 PDT

(In reply to comment #2) > It is, unfortunately, not that simple. There are many places in our code base where the paragraph separator is assumed to be a div. We'll have to go through editing code to find all those places and make sure to update them. It is probably worth doing this anyway, since developers already are using workarounds to get <p> in many cases. > > What is the syntax like? execCommand("defaultBlock", false, "p"/"div")? Yep. > > Also, does Opera's implementation accept arbitrarily element other than p and div? I don't want to have to test this function with every single block element if possible. It only accepts "p" or "div" (case-insensitive). All other values are no-ops. It also supports queryCommandValue, which will return either "p" or "div" (in lowercase).

Levi Weintraub

Comment 4 2011-05-02 14:50:18 PDT

(In reply to comment #3) > It only accepts "p" or "div" (case-insensitive). All other values are no-ops. > It also supports queryCommandValue, which will return either "p" or "div" (in lowercase). What's the rationale for only allowing two of the available block elements?

Ryosuke Niwa

Comment 5 2011-05-02 14:51:19 PDT

(In reply to comment #3) > > Also, does Opera's implementation accept arbitrarily element other than p and div? I don't want to have to test this function with every single block element if possible. > It only accepts "p" or "div" (case-insensitive). All other values are no-ops. > It also supports queryCommandValue, which will return either "p" or "div" (in lowercase). Okay, that sounds reasonable. Has this ever been brought up in whatwg? I'd like to know how much traction this feature gets before implementing it.

Ryosuke Niwa

Comment 6 2011-05-02 14:54:12 PDT

(In reply to comment #4) > (In reply to comment #3) > > It only accepts "p" or "div" (case-insensitive). All other values are no-ops. > > It also supports queryCommandValue, which will return either "p" or "div" (in lowercase). > > What's the rationale for only allowing two of the available block elements? Historically, browsers only supposed p, div, and br as a paragraph separator. If we allowed some other elements, then websites have to deal with them and some block elements (e.g. ul, blockquote, etc...) also have some special rules (e.g. h1 can't be nested) and special style (ul > li will have a list marker). So I think limiting to those two elements is a good thing.

Simon Pieters (:zcorpan)

Comment 7 2011-05-03 07:37:47 PDT

If you're interested in implementing this I suggest you bring it up in whatwg or htmlwg. AFAIK it has not been brought up yet.

Ojan Vafai

Comment 8 2011-05-03 09:43:03 PDT

Anne, do you know the history of Opera adding this and whether they intend to propose it to the whatwg?

David Bloom

Comment 9 2011-05-03 09:44:09 PDT

For reference, the Opera bug for implementing this is DSK-327033.

Simon Pieters (:zcorpan)

Comment 10 2011-05-03 14:15:09 PDT

I know the history. :-) We haven't proposed it since we didn't know other vendors would be interested in implementing this.

Pablo Flouret

Comment 11 2012-02-24 13:14:16 PST

https://www.w3.org/Bugs/Public/show_bug.cgi?id=15527

Pablo Flouret

Comment 12 2012-02-29 20:12:21 PST

Created attachment 129621 [details] Patch

Pablo Flouret

Comment 13 2012-02-29 20:14:42 PST

createDefaultParagraphElement() was used in a lot of code-paths and i've only tested a few relatively obvious things, so if anything else particularly important or interesting should be tested, let me know.

Ryosuke Niwa

Comment 14 2012-02-29 20:43:37 PST

Comment on attachment 129621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129621&action=review > Source/WebCore/editing/Editor.cpp:3028 > +PassRefPtr<HTMLElement> Editor::createDefaultParagraphSeparatorElement(Document* document) const > +{ > + if (defaultParagraphSeparatorIsDiv()) > + return HTMLDivElement::create(document); > + return HTMLParagraphElement::create(document); We don't want to have this function on Editor. We should just add this to CompositeEditCommand instead. > Source/WebCore/editing/Editor.h:407 > + bool m_defaultParagraphSeparatorIsDiv; // Should be P otherwise. Instead of saying that it should be p otherwise, please use enum.

Pablo Flouret

Comment 15 2012-02-29 21:04:28 PST

(In reply to comment #14) > (From update of attachment 129621 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129621&action=review > > > Source/WebCore/editing/Editor.cpp:3028 > > +PassRefPtr<HTMLElement> Editor::createDefaultParagraphSeparatorElement(Document* document) const > > +{ > > + if (defaultParagraphSeparatorIsDiv()) > > + return HTMLDivElement::create(document); > > + return HTMLParagraphElement::create(document); > > We don't want to have this function on Editor. We should just add this to CompositeEditCommand instead. > What about the calls in createFragmentFrom{Text,Nodes}? I guess i could go back to the helper function and get to the editor from the document.

Ryosuke Niwa

Comment 16 2012-02-29 22:24:35 PST

(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 129621 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129621&action=review > > We don't want to have this function on Editor. We should just add this to CompositeEditCommand instead. > > > > What about the calls in createFragmentFrom{Text,Nodes}? I guess i could go back to the helper function and get to the editor from the document. Ugh... that's a good point :( But let's also add one on CompositeEditCommand (can just be an inline function) so that we don't have to repeat document()->frame()->editor()-> everywhere.

Pablo Flouret

Comment 17 2012-03-01 14:33:10 PST

Created attachment 129748 [details] Patch Looking back on the change i think it looks cleaner to keep the helper and do the editor check there, if you'd still like to have it in CompositeEditCommand as well, i can add it.

Ryosuke Niwa

Comment 18 2012-03-01 14:38:40 PST

Comment on attachment 129748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129748&action=review > Source/WebCore/editing/Editor.cpp:3026 > +void Editor::setDefaultParagraphSeparator(EditorParagraphSeparator separator) > +{ > + ASSERT(separator == EditorParagraphSeparatorDiv || separator == EditorParagraphSeparatorP); > + m_defaultParagraphSeparator = separator; > +} I don't think we need an assertion here since compiler enforces the value in enum by default. Also, it's probably better to keep it inline. > Source/WebCore/editing/Editor.h:83 > +enum EditorParagraphSeparator { EditorParagraphSeparatorDiv, EditorParagraphSeparatorP }; Should be EditorParagraphSeparatorIsDiv and EditorParagraphSeparatorIsP > Source/WebCore/editing/htmlediting.cpp:852 > + if (document->frame()->editor()->defaultParagraphSeparator() == EditorParagraphSeparatorP) > + return HTMLParagraphElement::create(document); > return HTMLDivElement::create(document); We should use switch statement here instead so that we'll get a compilation error when a new value is added to enum and forgot to add a case here. > Source/WebCore/page/Frame.cpp:311 > + > + // Reset the editor's default paragraph separator to the default value (a div). > + m_editor.setDefaultParagraphSeparator(EditorParagraphSeparatorDiv); What!? We shouldn't have to do this.

Pablo Flouret

Comment 19 2012-03-01 15:23:27 PST

(In reply to comment #18) > (From update of attachment 129748 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129748&action=review > > > Source/WebCore/page/Frame.cpp:311 > > + > > + // Reset the editor's default paragraph separator to the default value (a div). > > + m_editor.setDefaultParagraphSeparator(EditorParagraphSeparatorDiv); > > What!? We shouldn't have to do this. I wasn't too happy about that one either, but (for instance) if i refresh the page or navigate away the editor is reused, and consequently the separator is not reset, which i think it should be.

Ryosuke Niwa

Comment 20 2012-03-01 15:34:02 PST

(In reply to comment #19) > I wasn't too happy about that one either, but (for instance) if i refresh the page or navigate away the editor is reused, and consequently the separator is not reset, which i think it should be. Okay, it seems like we have a bug here. But we should fix it in a separate patch since we have the same problem for styleWithCSS, etc...

Pablo Flouret

Comment 21 2012-03-01 15:47:39 PST

Created attachment 129764 [details] Patch

Ryosuke Niwa

Comment 22 2012-03-01 15:51:13 PST

Comment on attachment 129764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129764&action=review Where did test go? > Source/WebCore/editing/EditorCommand.cpp:1395 > + return "div"; Instead of hard-coding string here, can we use divTag.localName()?

Pablo Flouret

Comment 23 2012-03-01 16:09:40 PST

Created attachment 129765 [details] Patch

Pablo Flouret

Comment 24 2012-03-01 16:10:50 PST

(In reply to comment #20) > (In reply to comment #19) > > I wasn't too happy about that one either, but (for instance) if i refresh the page or navigate away the editor is reused, and consequently the separator is not reset, which i think it should be. > > Okay, it seems like we have a bug here. But we should fix it in a separate patch since we have the same problem for styleWithCSS, etc... Filed bug 80065.

Ryosuke Niwa

Comment 25 2012-03-01 16:15:01 PST

(In reply to comment #24) > (In reply to comment #20) > > (In reply to comment #19) > > > I wasn't too happy about that one either, but (for instance) if i refresh the page or navigate away the editor is reused, and consequently the separator is not reset, which i think it should be. > > > > Okay, it seems like we have a bug here. But we should fix it in a separate patch since we have the same problem for styleWithCSS, etc... > > Filed bug 80065. Thanks.

WebKit Review Bot

Comment 26 2012-03-02 01:20:56 PST

Comment on attachment 129765 [details] Patch Clearing flags on attachment: 129765 Committed r109529: <http://trac.webkit.org/changeset/109529>

WebKit Review Bot

Comment 27 2012-03-02 01:21:02 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.