Bug 36683 – For compatibility, execCommand should support deprecated 'useCSS' alias for 'styleWithCSS'

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

WebKit should support execCommand('useCSS', false, 'true'/'false') to be compatible with websites made for old versions of Firefox.  It's the same command as 'styleWithCSS'.  See the end of https://developer.mozilla.org/en/Midas.  We could argue that we shouldn't support it since it's already deprecated but there seems to be some websites that rely on this command.  For example, http://www.mozilla.org/editor/midasdemo/ is such a website.

Production step,
1. Go to http://www.mozilla.org/editor/midasdemo/
2. Check "Use CSS")
2. Execute any command

Expected: Styles are added by CSS.
Actual: Styles are added by presentational tags.

[Aryeh Gregor]

The editing spec currently requires support, but I'd be happy to drop it if Gecko is willing to.

http://aryeh.name/spec/editing/editing.html#the-usecss-command

[Kiyoto Tamura]

Created attachment 105861 [details]
the modified version of the test page.

[Kiyoto Tamura]

While I was working on this bug to get myself up to speed, I noticed something bizarre. I am using the Nightly Version 5.0.5 (6533.21.1, r94213) on Mac 10.6.7.

BEHAVIOR: styleWithCSS does not work properly

STEPS TO REPRODUCE:
1. Download the test page suggested by rniwa (http://www-archive.mozilla.org/editor/midasdemo/) and modify L403 to 

document.getElementById('edit').contentWindow.document.execCommand("styleWithCSS", false, source);

Essentially, modify the page so that it uses "styleWithCSS" as opposed to "useCSS". Note that the boolean argument needs to be flipped. I attached this modified version.

2. Load the modified version of the page (It would be missing images and such)

3. Although "useCSS" is checked, it does not work as expected. "View HTML Source" shows presentational tags. Now, if you uncheck "useCSS" and check it back, everything works as expected. CSS styles are now used instead of presentational tags.

[Kiyoto Tamura]

(In reply to comment #3)
This turned out to stem from the fact that the Mozilla page assumed that styleWithCSS/useCSS to be on by default.

> While I was working on this bug to get myself up to speed, I noticed something bizarre. I am using the Nightly Version 5.0.5 (6533.21.1, r94213) on Mac 10.6.7.
> 
> BEHAVIOR: styleWithCSS does not work properly
> 
> STEPS TO REPRODUCE:
> 1. Download the test page suggested by rniwa (http://www-archive.mozilla.org/editor/midasdemo/) and modify L403 to 
> 
> document.getElementById('edit').contentWindow.document.execCommand("styleWithCSS", false, source);
> 
> Essentially, modify the page so that it uses "styleWithCSS" as opposed to "useCSS". Note that the boolean argument needs to be flipped. I attached this modified version.
> 
> 2. Load the modified version of the page (It would be missing images and such)
> 
> 3. Although "useCSS" is checked, it does not work as expected. "View HTML Source" shows presentational tags. Now, if you uncheck "useCSS" and check it back, everything works as expected. CSS styles are now used instead of presentational tags.

[Kiyoto Tamura]

Created attachment 105881 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 105881 [details]
Patch

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

> Source/WebCore/editing/EditorCommand.cpp:1029
> +    if (value != "false" && value != "true")

I think we should do case-insensitive comparison here according to Aryeh's spec (http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command).  We should probably change executeStyleWithCSS as well.

[Kiyoto Tamura]

Created attachment 105916 [details]
Patch

[Aryeh Gregor]

What the spec says, and what Gecko does, is to interpret a case-insensitive match for "false" as "false", and anything else as "true".  So "false", "FALSE", and "fAlSe" all count as false, while "", "1", "0", " false", "true", "amsi993", "\0", etc. all count as true.  It doesn't make a big difference, since authors will usually pass boolean true or false, which will become the string "true" or "false".  But WebKit may as well match Gecko here.  Gecko's behavior is simpler here -- it can only do two things (enable/disable), not three (enable/disable/nothing).  Also, Gecko's behavior is probably more compatible.

[Darin Adler]

Comment on attachment 105916 [details]
Patch

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

Thanks for contributing a patch.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We require tests for patches that fix bugs like this one.

> Source/WebCore/editing/EditorCommand.cpp:1020
> +    const String lowercasedValue = value.lower();

The right way to do a case-insensitive comparison is to call equalIgnoringCase, not to call lower on the string. 

Since this patch is fixing a bug where we were case-sensitive before and now need to ignore case:

    1) The change log or bug title needs to mention that.
    2) The patch needs to include tests covering the correct behavior.

> Source/WebCore/editing/EditorCommand.cpp:1024
> +    frame->editor()->setShouldStyleWithCSS(lowercasedValue == "true" ? true : false);

The use of "? true : false" here is not good style. That’s a no-op and should be omitted.

This code should be written so it does two string comparisons, rather than repeating one a second time.

> Source/WebCore/editing/EditorCommand.cpp:1034
> +    frame->editor()->setShouldStyleWithCSS(lowercasedValue == "false" ? true : false);

A good way to write this is !equalIgnoringCase(value, "false"). That use of ? : is not helpful.

> Source/WebCore/editing/EditorCommand.cpp:1537
>          { "StyleWithCSS", { executeStyleWithCSS, supported, enabled, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
> +        { "UseCSS", { executeUseCSS, supported, enabled, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },
>          { "Subscript", { executeSubscript, supported, enabledInRichlyEditableText, stateSubscript, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

This list is alphabetical, so UseCSS should be put in the correct alphabetical location.

[Darin Adler]

(In reply to comment #8)
> What the spec says, and what Gecko does, is to interpret a case-insensitive match for "false" as "false", and anything else as "true".  So "false", "FALSE", and "fAlSe" all count as false, while "", "1", "0", " false", "true", "amsi993", "\0", etc. all count as true.  It doesn't make a big difference, since authors will usually pass boolean true or false, which will become the string "true" or "false".  But WebKit may as well match Gecko here.  Gecko's behavior is simpler here -- it can only do two things (enable/disable), not three (enable/disable/nothing).  Also, Gecko's behavior is probably more compatible.

I think this makes sense. For future browser engine developers, can we make sure there’s a spec that describes the Mozilla behavior?

[Darin Adler]

(In reply to comment #10)
> I think this makes sense. For future browser engine developers, can we make sure there’s a spec that describes the Mozilla behavior?

What I meant to say is “Aryeh, does your spec cover this too?”

The reason I ask is that when developing Safari we found that many website developers had made mistakes, and so behavior when passing crazy incorrect values was almost as important as normal behavior for real-world compatibility.

[Aryeh Gregor]

This is indeed specced:

http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command
http://aryeh.name/spec/editing/editing.html#the-usecss-command

If you click the "View comments" boxes on the right, you'll see the research that led to the current spec.  Only Gecko and WebKit support styleWithCSS at all, and I went with the Gecko behavior in this case because it's simpler and also older.  (In other cases I went with more WebKit-like behavior, e.g., the entire way inline formatting works.)

[Kiyoto Tamura]

Created attachment 107001 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 107001 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Please explain what kind of change you're making here.  See other change log entries for examples.

> LayoutTests/ChangeLog:7
> +        Adding useCSS support for execCommand and changing the behavior of styleWithCSS in accordance
> +        to http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command

This description should appear in the change log for WebCore.

> LayoutTests/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

"Reviewed by" should appear before the description.

> LayoutTests/ChangeLog:13
> +        * editing/execCommand/script-tests/toggle-text-decorations.js:
> +        (testSingleToggle):
> +        * editing/execCommand/toggle-text-decorations-expected.txt:

I don't think modifying these tests is a good way to test this behavior change.  These tests are testing execCommand('underline'/'strikeThrough') behaviors, not styleWithCSS or useCSS.

If I were you, I would write a test that does:
1. queryCommandState('styleWithCSS') returns true/fase after calling execCommand('useCSS'...) with false/true
2. the result of execCommand('bold' or whatever command) contains span/b after calling execCommand('useCSS') with false/true

r- because of this.

[Kiyoto Tamura]

Created attachment 107017 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 107017 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        the boolean false or the case-insensitive string "false". This is per http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command

Nit: You should probably line break after "per".

> Source/WebCore/editing/EditorCommand.cpp:1027
> +    frame->editor()->setShouldStyleWithCSS(equalIgnoringCase(value, "false"));
>      return true;

Per Aryeh's spec, it might be a good idea to log a console message saying that this command is deprecated.

> Source/WebCore/editing/EditorCommand.cpp:1540
> +        { "UseCSS", { executeUseCSS, supported, enabled, stateStyleWithCSS, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

Are you sure queryCommandState("useCSS") should behave like queryCommandState("styleWithCSS")?

> LayoutTests/editing/execCommand/style-with-css.html:10
> +<script src="script-tests/style-with-css.js"></script>

You can just include the script here instead.

> LayoutTests/editing/execCommand/use-css.html:10
> +<script src="script-tests/use-css.js"></script>

Ditto.

[Kiyoto Tamura]

Thanks for the prompt response. I will address the feedback related to tests/ChangeLog. As far as 

> 
> Are you sure queryCommandState("useCSS") should behave like queryCommandState("styleWithCSS")?
> 

goes,  I am pretty sure this yields a correct behavior. My understanding (per http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command) is that useCSS is essentially styleWithCSS except for its interpretation of the third argument passed into document.execCommand.

[Ryosuke Niwa]

(In reply to comment #17)
> I am pretty sure this yields a correct behavior. My understanding (per http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command) is that useCSS is essentially styleWithCSS except for its interpretation of the third argument passed into document.execCommand.

Interesting. Firefox doesn't support queryCommandState or queryCommandValue for UseCSS. It might be better for us not to return any state/value for this command.

Aryeh, any opinions on this?

[Aryeh Gregor]

Gecko doesn't support state for either styleWithCSS or useCSS, so there's no compatibility reason to support queryCommandState("useCSS").  The spec doesn't define any indeterm/state/value for useCSS, which means that queryCommand(Indeterm|State|Value)("useCSS") is supposed to throw INVALID_ACCESS_ERR.  This basically matches Gecko, which also throws an exception, but of a nonstandard type (NS_ERROR_FAILURE).

I added a note to make this clearer:

http://aryeh.name/gitweb.cgi?p=editing;a=commitdiff;h=7928c688
http://aryeh.name/spec/editing/editing.html#the-usecss-command

[Ryosuke Niwa]

(In reply to comment #19)
> Gecko doesn't support state for either styleWithCSS or useCSS, so there's no compatibility reason to support queryCommandState("useCSS").  The spec doesn't define any indeterm/state/value for useCSS, which means that queryCommand(Indeterm|State|Value)("useCSS") is supposed to throw INVALID_ACCESS_ERR.  This basically matches Gecko, which also throws an exception, but of a nonstandard type (NS_ERROR_FAILURE).

Thanks for the clarification.

However, since we throw any exceptions in queryCommand*, we probably should just return false here (i.e. just give use stateNone instead of stateStyleWithCSS).

[Kiyoto Tamura]

Created attachment 107131 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 107131 [details]
Patch

Could you also add tests for queryCommandValue/queryCommandState with styleWithCSS and useCSS?

[Kiyoto Tamura]

(In reply to comment #22)
> (From update of attachment 107131 [details])
> Could you also add tests for queryCommandValue/queryCommandState with styleWithCSS and useCSS?

Sure. What are we expected to see when queryCommandValue/State is called for useCSS? (I am not 100% sure what the expected behavior should be).

[Ryosuke Niwa]

(In reply to comment #23)
> Sure. What are we expected to see when queryCommandValue/State is called for useCSS? (I am not 100% sure what the expected behavior should be).

They should return 'false' and false respectively. I know Aryeh's spec says we should throw an exception but we don't do that anywhere else so that behavioral change should be made in a separate patch if we ever decide to do.

[Ryosuke Niwa]

Comment on attachment 107131 [details]
Patch

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

Let's add tests for queryCommandState/queryCommandValue and then land this patch.

> LayoutTests/editing/execCommand/style-with-css.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Can we use new style? (i.e. <!DOCTYPE html>)

> LayoutTests/editing/execCommand/style-with-css.html:21
> +    {
> +        testPassed('styleWithCSS changed the state successfully');
> +    } else {
> +        testFailed('styleWithCSS failed with the argument ' + styleArg);
> +    }

No curly brackets around single line statements.

> LayoutTests/editing/execCommand/use-css.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Ditto.

> LayoutTests/editing/execCommand/use-css.html:25
> +    {
> +        testPassed('useCSS changed the state successfully');
> +    } else {
> +        testFailed('useCSS failed with the argument ' + styleArg);
> +    }

Ditto.

> LayoutTests/editing/execCommand/use-css.html:38
> +    if (testContainer.innerHTML === expectedContents) {
> +        testPassed("one " + toggleCommand + " command converted " + initialContents + " to " + expectedContents);
> +    } else {
> +        testFailed("one " + toggleCommand + " command converted " + initialContents + " to " + testContainer.innerHTML + ", expected " + expectedContents);
> +    }

Ditto.

[Kiyoto Tamura]

> Let's add tests for queryCommandState/queryCommandValue and then land this patch.

Just for 'useCSS' or also for 'styleWithCSS'?

(In reply to comment #25)
> (From update of attachment 107131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107131&action=review
> 
> Let's add tests for queryCommandState/queryCommandValue and then land this patch.
> 
> > LayoutTests/editing/execCommand/style-with-css.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Can we use new style? (i.e. <!DOCTYPE html>)
> 
> > LayoutTests/editing/execCommand/style-with-css.html:21
> > +    {
> > +        testPassed('styleWithCSS changed the state successfully');
> > +    } else {
> > +        testFailed('styleWithCSS failed with the argument ' + styleArg);
> > +    }
> 
> No curly brackets around single line statements.
> 
> > LayoutTests/editing/execCommand/use-css.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Ditto.
> 
> > LayoutTests/editing/execCommand/use-css.html:25
> > +    {
> > +        testPassed('useCSS changed the state successfully');
> > +    } else {
> > +        testFailed('useCSS failed with the argument ' + styleArg);
> > +    }
> 
> Ditto.
> 
> > LayoutTests/editing/execCommand/use-css.html:38
> > +    if (testContainer.innerHTML === expectedContents) {
> > +        testPassed("one " + toggleCommand + " command converted " + initialContents + " to " + expectedContents);
> > +    } else {
> > +        testFailed("one " + toggleCommand + " command converted " + initialContents + " to " + testContainer.innerHTML + ", expected " + expectedContents);
> > +    }
> 
> Ditto.

[Ryosuke Niwa]

(In reply to comment #26)
> > Let's add tests for queryCommandState/queryCommandValue and then land this patch.
> 
> Just for 'useCSS' or also for 'styleWithCSS'?

Definitely for useCSS but it'll be great if you could also add tests for styleWithCSS.

[Aryeh Gregor]

(In reply to comment #24)
> They should return 'false' and false respectively. I know Aryeh's spec says we should throw an exception but we don't do that anywhere else so that behavioral change should be made in a separate patch if we ever decide to do.

Sounds reasonable.

[Kiyoto Tamura]

Created attachment 107237 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 107237 [details]
Patch

Thanks for adding the test!

[Ryosuke Niwa]

Perhaps you want to ask for cq+ ?

[Kiyoto Tamura]

What is cq+? Commit Queue?
(In reply to comment #31)
> Perhaps you want to ask for cq+ ?

[Ryosuke Niwa]

(In reply to comment #32)
> What is cq+? Commit Queue?
> (In reply to comment #31)
> > Perhaps you want to ask for cq+ ?

Yes. The commit queue can your land your patch if some committer sets cq+.  Alternatively, you can ask some committer to land your patch on behalf.

[Darin Adler]

When submitting a patch, if you want it to be landed by the commit-queue, you can indicate it by setting the commit-queue flag to ?. That lets a committer know that it should be set to + if it’s good to land.

[Darin Adler]

Comment on attachment 107237 [details]
Patch

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

> LayoutTests/editing/execCommand/use-css.html:42
> +testUseCSS(false, true);
> +testUseCSS('false', true);
> +testUseCSS('FALSE', true);
> +testUseCSS(true, false);
> +testUseCSS('random string', false);

Other cases to test would be the empty string and the string " false " (which apparently means true).

> LayoutTests/editing/execCommand/use-css.html:47
> +if (document.queryCommandState('useCSS') === false)
> +    testPassed("queryCommandState('useCSS') returns false");
> +else
> +    testFailed("queryCommandState('useCSS') should return boolean false");

I think the shouldBe macro does a better job of this; I’m not sure that === false will fail for every value other than false.

[Kiyoto Tamura]

(In reply to comment #35)
> Other cases to test would be the empty string and the string " false " (which apparently means true).

We can certainly test that.

> 
> > LayoutTests/editing/execCommand/use-css.html:47
> > +if (document.queryCommandState('useCSS') === false)
> > +    testPassed("queryCommandState('useCSS') returns false");
> > +else
> > +    testFailed("queryCommandState('useCSS') should return boolean false");
> 
> I think the shouldBe macro does a better job of this; I’m not sure that === false will fail for every value other than false.

Do you know which shouldBe macro it is? I was looking at LayoutTest/fast/js/resources/js-test-pre.js and the shouldBe function seemed to require that both arguments be strings. Here, we are trying to test that it is the boolean true/false as opposed to strings. The (===) is to avoid JS's notorious type coercions.

[WebKit Review Bot]

Comment on attachment 107237 [details]
Patch

Clearing flags on attachment: 107237

Committed r95050: <http://trac.webkit.org/changeset/95050>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(In reply to comment #36)
> Do you know which shouldBe macro it is? I was looking at LayoutTest/fast/js/resources/js-test-pre.js and the shouldBe function seemed to require that both arguments be strings. Here, we are trying to test that it is the boolean true/false as opposed to strings. The (===) is to avoid JS's notorious type coercions.

shouldBe evaluates both strings and then compare so shouldBe('true', '1 == 1') will PASS.

[Darin Adler]

(In reply to comment #36)
> Do you know which shouldBe macro it is?

shouldBe("document.queryCommandState('useCSS')", "false");