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
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-05-02 14:23 PST by
Modified: 2012-03-02 01:21 PST (History)


Attachments
Patch (24.12 KB, patch)
2012-02-29 20:12 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.00 KB, patch)
2012-03-01 14:33 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2012-03-01 15:47 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.41 KB, patch)
2012-03-01 16:09 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-05-02 14:23:36 PST
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 From 2011-05-02 14:32:00 PST -------
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 From 2011-05-02 14:41:25 PST -------
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 From 2011-05-02 14:47:58 PST -------
(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 From 2011-05-02 14:50:18 PST -------
(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 From 2011-05-02 14:51:19 PST -------
(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 From 2011-05-02 14:54:12 PST -------
(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 From 2011-05-03 07:37:47 PST -------
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 From 2011-05-03 09:43:03 PST -------
Anne, do you know the history of Opera adding this and whether they intend to propose it to the whatwg?
------- Comment #9 From 2011-05-03 09:44:09 PST -------
For reference, the Opera bug for implementing this is DSK-327033.
------- Comment #10 From 2011-05-03 14:15:09 PST -------
I know the history. :-) We haven't proposed it since we didn't know other vendors would be interested in implementing this.
------- Comment #11 From 2012-02-24 13:14:16 PST -------
https://www.w3.org/Bugs/Public/show_bug.cgi?id=15527
------- Comment #12 From 2012-02-29 20:12:21 PST -------
Created an attachment (id=129621) [details]
Patch
------- Comment #13 From 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 From 2012-02-29 20:43:37 PST -------
(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.

> 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 From 2012-02-29 21:04:28 PST -------
(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
> 
> > 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 From 2012-02-29 22:24:35 PST -------
(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 129621 [details] [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 From 2012-03-01 14:33:10 PST -------
Created an attachment (id=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 From 2012-03-01 14:38:40 PST -------
(From update of attachment 129748 [details])
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 From 2012-03-01 15:23:27 PST -------
(In reply to comment #18)
> (From update of attachment 129748 [details] [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 From 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 From 2012-03-01 15:47:39 PST -------
Created an attachment (id=129764) [details]
Patch
------- Comment #22 From 2012-03-01 15:51:13 PST -------
(From update of attachment 129764 [details])
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 From 2012-03-01 16:09:40 PST -------
Created an attachment (id=129765) [details]
Patch
------- Comment #24 From 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 From 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 From 2012-03-02 01:20:56 PST -------
(From update of attachment 129765 [details])
Clearing flags on attachment: 129765

Committed r109529: <http://trac.webkit.org/changeset/109529>
------- Comment #27 From 2012-03-02 01:21:02 PST -------
All reviewed patches have been landed.  Closing bug.