| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
David Bloom
2011-05-02 14:23:36 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. 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.
(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). (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? (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. (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. 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. Anne, do you know the history of Opera adding this and whether they intend to propose it to the whatwg? For reference, the Opera bug for implementing this is DSK-327033. I know the history. :-) We haven't proposed it since we didn't know other vendors would be interested in implementing this. Created attachment 129621 [details]
Patch
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 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. (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. (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. 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 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. (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. (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... Created attachment 129764 [details]
Patch
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()? Created attachment 129765 [details]
Patch
(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. (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 on attachment 129765 [details] Patch Clearing flags on attachment: 129765 Committed r109529: <http://trac.webkit.org/changeset/109529> All reviewed patches have been landed. Closing bug. |