408231 - queryCommandEnabled( "formatblock" ) throws NS_ERROR_NOT_IMPLEMENTED

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Peter Hodgkinson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: 3.0b3pre

When executing the following block of javascript the command document.queryCommandEnabled( "formatblock" ) throws a NS_ERROR_NOT_IMPLEMENTED exception. This is a change in functionality between Firefox 2 and Firefox 3. Firefox 2 returns 'true'.

<script>
        var error;
        document.designMode='on';
        try {
                alert(  document.queryCommandEnabled( "formatblock" ) );
        }
        catch( error ) {
                alert( error );
        }
</script>


This looks like it may be related Bug 404320, however this particular bug is still present in the latest nightly build.

Reproducible: Always

Steps to Reproduce:
1. Run the following javascript


<script>
        var error;
        document.designMode='on';
        try {
                alert(  document.queryCommandEnabled( "formatblock" ) );
        }
        catch( error ) {
                alert( error );
        }
</script>
Actual Results:  
A NS_ERROR_NOT_IMPLEMENTED exception is thrown

Expected Results:  
'true' is returned

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem here is that when nsHTMLDocument::ConvertToMidasInternalCommand() encounters commands which corresponds to "cmd_paragraphState" ("formatblock", "heading", "insertparagraph"), ConvertToMidasInternalCommand() assumes that its inParam parameter corresponds to whatever paragraph state that we're toggling. This is true in the nsHTMLDocument::ExecCommand() case, but in the other 4 cases (QueryCommandEnabled(), QueryCommandIndeterm(), QueryCommandState(), QueryCommandValue()) we don't care about the input parameter, and just pass in the external commandId (e.g. "formatblock", ...). ConvertToMidasInternalCommand() has a special case for commands corresponding to "cmd_paragraphState". It checks to see if inParam matches the the name of the blocks being worked with, and if not it returns failure. The four cases where we just pass on the external commandId as inParam will fail, as none of {"formatblock", "heading", "insertparagraph"} correspond to block names {p, div, ...}. Hence this bug.

The fix is to split ConvertToMidasInternalCommand() into two methods, one that just retrieves the internal command name, and one that also checks the parameter. ExecCommand() will then check inParam, and QueryCommandEnabled(), QueryCommandIndeterm(), QueryCommandState() and QueryCommandValue() won't, as they don't need to.

PeterV, does that sound reasonable? Mind if I take this one?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Patch + mochitest. -W patch to follow.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 -w

Patch + mochitest. -W patch to follow.

* Added nsHTMLDocument::ConvertToMidasInternalCommand(inCmd,outCmd) which just retrieves the internal name of the inCmd.
* ConvertToMidasInternalCommand defers to ConvertToMidasInternalCommandInner, which can ignore the parameters passed, so as to implement ConvertToMidasInternalCommand(inCmd,outCmd).
* Changed some of the QueryCommand...() functions to use the appropriate ConvertToMidasInternalCommand().

This patch changes the behaviour of QueryCommandValue() slightly from FireFox 2.

queryCommandValue("createlink") FF2="createlink" PatchV1=""
queryCommandValue("decreasefontsize") FF2="decreasefontsize" PatchV1=""
queryCommandValue("increasefontsize") FF2="increasefontsize" PatchV1=""
queryCommandValue("inserthtml") FF2="inserthtml" PatchV1=""
queryCommandValue("insertimage") FF2="insertimage" PatchV1=""

I think these values make more sense. If not, I can work harder to make the patched values match FF2.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Test - see if the values of queryCommand*() functions match patch v1

This pages tests whether the values of the queryCommand*() functions match those of patch v1. So you can run this in FF2 to see how it's different. Based on the mochitest for patch v1.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 293735 [details] [diff] [review]
Patch v1 -w

>Index: content/html/content/test/Makefile.in
>===================================================================

>+    test_bug408231.html \

Use tabs.

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================

>+PRBool
>+nsHTMLDocument::ConvertToMidasInternalCommand(const nsAString & inCommandID,
>+                                              nsACString& outCommandID)

Make this a static non-member function instead.

>+PRBool
>+nsHTMLDocument::ConvertToMidasInternalCommandInner(const nsAString & inCommandID,
>+                                                   const nsAString & inParam,
>+                                                   nsACString& outCommandID,
>+                                                   nsACString& outParam,
>+                                                   PRBool& outIsBoolean,
>+                                                   PRBool& outBooleanValue,
>+                                                   PRBool aIgnoreParams)

Make this a static non-member function instead.

>+  {
>+  

Remove those leading spaces.

>@@ -4475,18 +4504,17 @@ nsHTMLDocument::QueryCommandEnabled(cons

>   nsCAutoString cmdToDispatch, paramStr;
>   PRBool isBool, boolVal;

Remove paramStr, isBool and boolVal declarations.

>-  if (!ConvertToMidasInternalCommand(commandID, commandID,
>-                                     cmdToDispatch, paramStr, isBool, boolVal))
>+  if (!ConvertToMidasInternalCommand(commandID, cmdToDispatch))

>@@ -4638,18 +4666,17 @@ nsHTMLDocument::QueryCommandValue(const 

>   nsCAutoString cmdToDispatch, paramStr;
>   PRBool isBool, boolVal;

Remove paramStr, isBool and boolVal declarations.

>-  if (!ConvertToMidasInternalCommand(commandID, commandID,
>-                                     cmdToDispatch, paramStr, isBool, boolVal))
>+  if (!ConvertToMidasInternalCommand(commandID, cmdToDispatch))

>Index: content/html/document/src/nsHTMLDocument.h
>===================================================================

>   nsresult   GetMidasCommandManager(nsICommandManager** aCommandManager);
>+  

Don't add trailing spaces.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - v1 with Peterv's suggestions

As Patch v1, but with peterv's changes. r/sr+ peterv.

Comment 8

[Reed Loden [:reed]]

Took the opportunity to order some of the tests in the makefile numerically...

Checking in content/html/content/test/Makefile.in;
/cvsroot/mozilla/content/html/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.38; previous revision: 1.37
done
RCS file: /cvsroot/mozilla/content/html/content/test/test_bug408231.html,v
done
Checking in content/html/content/test/test_bug408231.html;
/cvsroot/mozilla/content/html/content/test/test_bug408231.html,v  <--  test_bug408231.html
initial revision: 1.1
done
Checking in content/html/document/src/nsHTMLDocument.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v  <--  nsHTMLDocument.cpp
new revision: 3.767; previous revision: 3.766
done
Checking in content/html/document/src/nsHTMLDocument.h;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.h,v  <--  nsHTMLDocument.h
new revision: 3.223; previous revision: 3.222
done

Comment 9

[Reed Loden [:reed]]

The test for "paste" failed on Linux, so I commented out all three parts in the test corresponding to "paste".

*** 10621 ERROR FAIL | queryCommandEnabled(paste) result=false expected=true |  | /tests/content/html/content/test/test_bug408231.html

Note that the entire test passes on Mac. Weird.