Bug 59961 - Implement DefaultParagraphSeparator execCommand, to let authors choose the default block element
: Implement DefaultParagraphSeparator execCommand, to let authors choose the de...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-02 14:23 PDT by David Bloom
Modified: 2012-03-02 01:21 PST (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Bloom 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
Comment 1 Ojan Vafai 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.
Comment 2 Ryosuke Niwa 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.
Comment 3 David Bloom 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).
Comment 4 Levi Weintraub 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?
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 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.
Comment 7 Simon Pieters 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.
Comment 8 Ojan Vafai 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?
Comment 9 David Bloom 2011-05-03 09:44:09 PDT
For reference, the Opera bug for implementing this is DSK-327033.
Comment 10 Simon Pieters 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.
Comment 12 Pablo Flouret 2012-02-29 20:12:21 PST
Created attachment 129621 [details]
Patch
Comment 13 Pablo Flouret 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.
Comment 14 Ryosuke Niwa 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.
Comment 15 Pablo Flouret 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.
Comment 16 Ryosuke Niwa 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.
Comment 17 Pablo Flouret 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.
Comment 18 Ryosuke Niwa 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.
Comment 19 Pablo Flouret 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.
Comment 20 Ryosuke Niwa 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...
Comment 21 Pablo Flouret 2012-03-01 15:47:39 PST
Created attachment 129764 [details]
Patch
Comment 22 Ryosuke Niwa 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()?
Comment 23 Pablo Flouret 2012-03-01 16:09:40 PST
Created attachment 129765 [details]
Patch
Comment 24 Pablo Flouret 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.
Comment 25 Ryosuke Niwa 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.
Comment 26 WebKit Review Bot 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>
Comment 27 WebKit Review Bot 2012-03-02 01:21:02 PST
All reviewed patches have been landed.  Closing bug.