Bug 51164 – execCommand('styleWithCSS') is ignored when selection isn't inside a contenteditable area

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

WebKit ignores document.execCommand('styleWithCSS', false, true/false) when the document's selection isn't inside a contenteditable area. Since styleWithCSS doesn't modify DOM, WebKit should respect calls even when the selection isn't in an editable region.

[Ryosuke Niwa]

Created attachment 76735 [details]
demo

[Darin Adler]

(In reply to comment #0)
> styleWithCSS doesn't modify DOM

I didn’t know that! I thought that it added style attributes to DOM elements.

[Ryosuke Niwa]

(In reply to comment #2)
> (In reply to comment #0)
> > styleWithCSS doesn't modify DOM
> 
> I didn’t know that! I thought that it added style attributes to DOM elements.

Subsequent execCommands DO modify DOM by inserting spans and adding style attributes but styleWithCSS itself doesn't.  It's simply a flag in Editor class:
http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp?rev=73886#L982
http://trac.webkit.org/browser/trunk/WebCore/editing/Editor.h?rev=73886#L168

And it's not unusual for someone to call StyleWithCSS without putting caret inside an editable region.  For example, text editor can call it right up front to enforce the use of CSS.  We just had that bug in browser scope test (it was calling styleWithCSS before setting up selection).  And apparently, we're the only browser that requires caret/selection being inside an editable region for StyleWithCSS.

[Darin Adler]

I see. This command simply sets a flag. This should be very easy to fix!

[Julie Parent]

There is a little bit of subtlety here ... what happens if a document has 5 separate editable roots, the selection is in none of them, and you call execCommand('styleWithCSS')?  Does it effect all of them?

[Julie Parent]

Anyway, making this always enabled looks as easy as changing enabledInRichlyEditableText to enabled in

{ "StyleWithCSS", { executeStyleWithCSS, supported, enabledInRichlyEditableText, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

in WebCore/editing/EditorCommand.cpp

[Darin Adler]

(In reply to comment #5)
> There is a little bit of subtlety here ... what happens if a document has 5 separate editable roots, the selection is in none of them, and you call execCommand('styleWithCSS')?  Does it effect all of them?

We don’t have any per-editable-root settings. I presume this sets a per-document flag in WebKit now and should continue to do so. I’d imagine this is how ti works in other browsers as well.

[Ryosuke Niwa]

(In reply to comment #6)
> Anyway, making this always enabled looks as easy as changing enabledInRichlyEditableText to enabled in
> 
> { "StyleWithCSS", { executeStyleWithCSS, supported, enabledInRichlyEditableText, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

Indeed.  I'm going to post a patch in a minute.

(In reply to comment #7)
> (In reply to comment #5)
> > There is a little bit of subtlety here ... what happens if a document has 5 separate editable roots, the selection is in none of them, and you call execCommand('styleWithCSS')?  Does it effect all of them?
> 
> We don’t have any per-editable-root settings. I presume this sets a per-document flag in WebKit now and should continue to do so. I’d imagine this is how ti works in other browsers as well.

What Darin said. I think a lot of developers don't realize this because most of editing apps use an iframe + design mode to work-around Firefox's quirk to modify the editing host.

[Ryosuke Niwa]

Created attachment 76792 [details]
fixes the bug

[Ryosuke Niwa]

Thanks for the review, Darin. Landing now.

[Ryosuke Niwa]

Comment on attachment 76792 [details]
fixes the bug

Clearing flags on attachment: 76792

Committed r74204: <http://trac.webkit.org/changeset/74204>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74204 might have broken Leopard Intel Debug (Tests)