Bug 36683 - For compatibility, execCommand should support deprecated 'useCSS' alias for 'styleWithCSS'
Summary: For compatibility, execCommand should support deprecated 'useCSS' alias for '...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P3 Trivial
Assignee: Nobody
URL:
Keywords: EasyFix, NeedsReduction
Depends on:
Blocks:
 
Reported: 2010-03-26 16:08 PDT by Ryosuke Niwa
Modified: 2011-09-13 16:18 PDT (History)
9 users (show)

See Also:


Attachments
the modified version of the test page. (13.57 KB, text/html)
2011-08-31 16:57 PDT, Kiyoto Tamura
no flags Details
Patch (2.69 KB, patch)
2011-08-31 19:00 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff
Patch (3.04 KB, patch)
2011-09-01 00:50 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff
Patch (10.13 KB, patch)
2011-09-10 22:52 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2011-09-11 20:53 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff
Patch (8.74 KB, patch)
2011-09-12 19:40 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff
Patch (10.42 KB, patch)
2011-09-13 14:59 PDT, Kiyoto Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-03-26 16:08:39 PDT
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.
Comment 1 Aryeh Gregor 2011-08-18 11:34:38 PDT
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
Comment 2 Kiyoto Tamura 2011-08-31 16:57:48 PDT
Created attachment 105861 [details]
the modified version of the test page.
Comment 3 Kiyoto Tamura 2011-08-31 17:02:55 PDT
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.
Comment 4 Kiyoto Tamura 2011-08-31 17:18:50 PDT
(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.
Comment 5 Kiyoto Tamura 2011-08-31 19:00:15 PDT
Created attachment 105881 [details]
Patch
Comment 6 Ryosuke Niwa 2011-08-31 19:44:35 PDT
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.
Comment 7 Kiyoto Tamura 2011-09-01 00:50:01 PDT
Created attachment 105916 [details]
Patch
Comment 8 Aryeh Gregor 2011-09-01 08:03:13 PDT
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.
Comment 9 Darin Adler 2011-09-01 08:43:49 PDT
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.
Comment 10 Darin Adler 2011-09-01 08:44:57 PDT
(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?
Comment 11 Darin Adler 2011-09-01 08:46:16 PDT
(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.
Comment 12 Aryeh Gregor 2011-09-01 10:05:25 PDT
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.)
Comment 13 Kiyoto Tamura 2011-09-10 22:52:50 PDT
Created attachment 107001 [details]
Patch
Comment 14 Ryosuke Niwa 2011-09-10 23:14:15 PDT
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.
Comment 15 Kiyoto Tamura 2011-09-11 20:53:04 PDT
Created attachment 107017 [details]
Patch
Comment 16 Ryosuke Niwa 2011-09-11 22:06:38 PDT
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.
Comment 17 Kiyoto Tamura 2011-09-11 22:48:15 PDT
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.
Comment 18 Ryosuke Niwa 2011-09-11 22:58:56 PDT
(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?
Comment 19 Aryeh Gregor 2011-09-12 13:50:01 PDT
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
Comment 20 Ryosuke Niwa 2011-09-12 14:17:58 PDT
(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).
Comment 21 Kiyoto Tamura 2011-09-12 19:40:06 PDT
Created attachment 107131 [details]
Patch
Comment 22 Ryosuke Niwa 2011-09-12 20:07:52 PDT
Comment on attachment 107131 [details]
Patch

Could you also add tests for queryCommandValue/queryCommandState with styleWithCSS and useCSS?
Comment 23 Kiyoto Tamura 2011-09-12 20:17:09 PDT
(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).
Comment 24 Ryosuke Niwa 2011-09-12 20:21:54 PDT
(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.
Comment 25 Ryosuke Niwa 2011-09-13 00:51:47 PDT
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.
Comment 26 Kiyoto Tamura 2011-09-13 12:57:45 PDT
> 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.
Comment 27 Ryosuke Niwa 2011-09-13 13:23:39 PDT
(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.
Comment 28 Aryeh Gregor 2011-09-13 13:39:32 PDT
(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.
Comment 29 Kiyoto Tamura 2011-09-13 14:59:19 PDT
Created attachment 107237 [details]
Patch
Comment 30 Ryosuke Niwa 2011-09-13 15:01:41 PDT
Comment on attachment 107237 [details]
Patch

Thanks for adding the test!
Comment 31 Ryosuke Niwa 2011-09-13 15:02:03 PDT
Perhaps you want to ask for cq+ ?
Comment 32 Kiyoto Tamura 2011-09-13 15:03:36 PDT
What is cq+? Commit Queue?
(In reply to comment #31)
> Perhaps you want to ask for cq+ ?
Comment 33 Ryosuke Niwa 2011-09-13 15:04:44 PDT
(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.
Comment 34 Darin Adler 2011-09-13 15:06:41 PDT
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.
Comment 35 Darin Adler 2011-09-13 15:09:18 PDT
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.
Comment 36 Kiyoto Tamura 2011-09-13 15:19:52 PDT
(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.
Comment 37 WebKit Review Bot 2011-09-13 15:30:29 PDT
Comment on attachment 107237 [details]
Patch

Clearing flags on attachment: 107237

Committed r95050: <http://trac.webkit.org/changeset/95050>
Comment 38 WebKit Review Bot 2011-09-13 15:30:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Ryosuke Niwa 2011-09-13 15:41:46 PDT
(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.
Comment 40 Darin Adler 2011-09-13 16:18:05 PDT
(In reply to comment #36)
> Do you know which shouldBe macro it is?

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