Bug 21305 – queryCommandValue "formatBlock" always returns false

• Component: HTML Editing
• Status: RESOLVED FIXED

[Anthony Johnston]

On Google Chrome

Official Build 2200
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/525.13 (KHTML, like Gecko) Chrome/0.2.149.30 Safari/525.13 

Calling the following always gets a false returned, it should return a value such as "p", "h1" etc..

document.queryCommandValue("formatBlock")

[Ryosuke Niwa]

I know Internet Explorer returns "normal" for anything but h1-h6, address, & p.  But that sounds rather unreasonable to me.  We should probably return other block element as well.  Here are HTML5 elements that have display:block in the default style:

p, div, layer, article, aside, footer, header, hgroup, nav, section, address, blockquote, hr, h1, h2, h3, h4, h5, h6, ul, menu, dir, ol, dd, dl, dt, pre, xmp, plaintext, listing

The following elements are also block elements but Firefox doesn't return:
form, legend, fieldset, rt (inside ruby, of course, Firefox doesn't support this anyway), frameset, frame, iframe

[Ryosuke Niwa]

The further investigation seems to indicate that Firefox doesn't return div, layer, article, etc... either. So Firefox and MSIE almost agree on everything except that:
1. When there are no matching block ancestors (none of them are h1-h6, p, address), Firefox returns empty string while MSIE returns "normal".
2. For pre, Firefox returns "pre" and MSIE returns "formatted".

[Ryosuke Niwa]

Opera on the other hand also returns blockquote, div, and possibly other block elements.  It does return pre so it's pretty clear to me at this point that we should implement what Firefox implements.

[Ryosuke Niwa]

Created attachment 67765 [details]
Patch

[Ryosuke Niwa]

Thanks for the review, Justin!

[Ryosuke Niwa]

Comment on attachment 67765 [details]
Patch

Oops, I shouldn't be adding these functions to EditorCommand.cpp.  I'll move the functions and resubmit.

[Ryosuke Niwa]

Created attachment 67864 [details]
Patch

[Ryosuke Niwa]

Sorry about my forgetting about the move.  It's identical patch so it should be easy to review.

[Roland Steiner]

(In reply to comment #1)
> The following elements are also block elements but Firefox doesn't return:
> form, legend, fieldset, rt (inside ruby, of course, Firefox doesn't support this anyway), frameset, frame, iframe

Just a quick note: <rt> is implemented as block, but mostly out of convenience. From a user's point of view I don't think it should be reported/treated as block.

[Darin Adler]

Comment on attachment 67864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67864&action=review

I’m reluctantly saying review+ here. I am not that happy with this extremely specific function being added to the Editor class with an unclear name. I’m not sure the function belongs in the Editor class at all.

> WebCore/editing/Editor.cpp:986
> +static bool isFormatBlock(const Node* node)

The term “block” is normally a rendering term, so an “is block” function should be answering a question about rendering, not DOM.

This function answers a much more specific question, and it would be better to give it a specific name.

Generally speaking, putting this specific “FormatBlock” concept into Editor is OK, but we need to give it a name that makes it clear this is “for the FormatBlock” command, and not some more fundamental concept that is useful elsewhere.

> WebCore/editing/Editor.cpp:1002
> +    if (!m_frame->selection())
> +        return String("");

This can never be 0, so this check is not needed.

> WebCore/editing/Editor.cpp:1006
> +        return String("");

I would prefer either

    return "";

Or:

    return emptyAtom;

Explicitly saying String("") doesn’t seem to add anything. I think that returning emptyAtom might be slightly more efficient, which is why I mention that option.

> WebCore/editing/Editor.cpp:1008
> +    ExceptionCode ec = 0;

There’s no need to set up and check the exception code here. The commonAncestorContainer function is guaranteed to return 0 any time an exception would be raised. The exception code is only there for the bindings’ benefit. We could even create an overloaded version without the exception code for use inside the engine. It’s really only JavaScript and other such bindings that need the exception code.

> WebCore/editing/Editor.cpp:1016
> +    return static_cast<Element*>(commonAncestor)->localName();

This cast is only safe if we have a guarantee that the common ancestor is not a document. It would be best to have a comment explaining why that is so, or we could add an “is element” check above and be a little safer. At the very least I’d like to see an assertion.

> WebCore/editing/Editor.h:128
> +    String selectionFormatBlock();

I think this function name is very confusing. The FormatBlock thing is a strange, quirky, execCommand name that is not based on normal web browser terminology. Further, a block is not a string, and a function named “block” should return a block, not a string. Editor should supply an operation with a name that could be clear to someone who’s never heard of “FormatBlock”. Or it could just have a name that explicitly refers to the FormatBlock command in its name. Can you think of a better name? I can think and see if I can come up with one, but I don’t have an immediate suggestion.

[Ryosuke Niwa]

Comment on attachment 67864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67864&action=prettypatch

>> WebCore/editing/Editor.cpp:986
>> +static bool isFormatBlock(const Node* node)
> 
> The term “block” is normally a rendering term, so an “is block” function should be answering a question about rendering, not DOM.
> 
> This function answers a much more specific question, and it would be better to give it a specific name.
> 
> Generally speaking, putting this specific “FormatBlock” concept into Editor is OK, but we need to give it a name that makes it clear this is “for the FormatBlock” command, and not some more fundamental concept that is useful elsewhere.

How about isElementForFormatBlockCommand ?

>> WebCore/editing/Editor.cpp:1002
>> +        return String("");
> 
> This can never be 0, so this check is not needed.

Huh, I want to say that I saw the same check somewhere and that's why I added it.  But I can't find it now.  Will remove.

>> WebCore/editing/Editor.cpp:1006
>> +        return String("");
> 
> I would prefer either
> 
>     return "";
> 
> Or:
> 
>     return emptyAtom;
> 
> Explicitly saying String("") doesn’t seem to add anything. I think that returning emptyAtom might be slightly more efficient, which is why I mention that option.

Will do.

>> WebCore/editing/Editor.cpp:1008
>> +    ExceptionCode ec = 0;
> 
> There’s no need to set up and check the exception code here. The commonAncestorContainer function is guaranteed to return 0 any time an exception would be raised. The exception code is only there for the bindings’ benefit. We could even create an overloaded version without the exception code for use inside the engine. It’s really only JavaScript and other such bindings that need the exception code.

Will do.

>> WebCore/editing/Editor.cpp:1016
>> +    return static_cast<Element*>(commonAncestor)->localName();
> 
> This cast is only safe if we have a guarantee that the common ancestor is not a document. It would be best to have a comment explaining why that is so, or we could add an “is element” check above and be a little safer. At the very least I’d like to see an assertion.

It is guaranteed because we skip any element that's not isFormatBlock.  But adding an assertion here might be a good idea.

>> WebCore/editing/Editor.h:128
>> +    String selectionFormatBlock();
> 
> I think this function name is very confusing. The FormatBlock thing is a strange, quirky, execCommand name that is not based on normal web browser terminology. Further, a block is not a string, and a function named “block” should return a block, not a string. Editor should supply an operation with a name that could be clear to someone who’s never heard of “FormatBlock”. Or it could just have a name that explicitly refers to the FormatBlock command in its name. Can you think of a better name? I can think and see if I can come up with one, but I don’t have an immediate suggestion.

Maybe currentFormatBlockCommandValue or selectionFormatBlockCommandValue?

[Darin Adler]

(In reply to comment #11)
> How about isElementForFormatBlockCommand ?

Yes, better name.

> It is guaranteed because we skip any element that's not isFormatBlock.  But adding an assertion here might be a good idea.

Yes, that is a guarantee! Not sure why I didn’t spot that.

> Maybe currentFormatBlockCommandValue or selectionFormatBlockCommandValue?

Either would be OK. My idea based on your function name above would valueForFormatBlockCommand.

Another possibility that I like slightly better would be to have an elementForFormatBlockCommand function and call localName at the call site in EditorCommand.cpp.

I’m still not sure these functions belong in Editor. I could see them going somewhere else. We are getting dangerously close to having a "god class".

[Ryosuke Niwa]

Created attachment 68545 [details]
fixed per darin's comments

[Ryosuke Niwa]

(In reply to comment #12)
> > Maybe currentFormatBlockCommandValue or selectionFormatBlockCommandValue?
> 
> Either would be OK. My idea based on your function name above would valueForFormatBlockCommand.
> 
> Another possibility that I like slightly better would be to have an elementForFormatBlockCommand function and call localName at the call site in EditorCommand.cpp.

I like that!

> I’m still not sure these functions belong in Editor. I could see them going somewhere else. We are getting dangerously close to having a "god class".

I was thinking that maybe we could move all functions related to SelectionController or VisibleSelection since they only need to have access to the selection.  But then SelectionController seems to be included in 1000+ files and I'm hesitant to add a new function there.  On the other hand, moving them to VisibleSelection seems odd because VisibleSelection is a more primitive object and these functions are very high level stuff in editing almost directly called by the DOM and users.

[Ryosuke Niwa]

Hi Darin,

Could you review this patch at your earliest convenience?

[Ryosuke Niwa]

Committed r68670: <http://trac.webkit.org/changeset/68670>

[Ryosuke Niwa]

Reviewer name is fixed in http://trac.webkit.org/changeset/68671.