297494 - queryCommandState returns error when giving it certain commands

Status: NEW

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

Description

[Mike Badgley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

When attempting to find the results (true/false) of a given command,
queryCommandState will give a nasty error response for the following commands:

1. createlink
2. outdent
3. indent
4. hilitecolor
5. forecolor
6. formatblock

All other commands (i.e. bold) return true or false if they are enabled/disabled
by execCommand.

Reproducible: Always

Steps to Reproduce:
1. Turn designMode on for a particular IFRAME
2. Run a queryCommandState on a particular command (i.e. createlink)

Actual Results:  
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.queryCommandState]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
http://www.mikelovesjulia.com/test/RTE.js :: BuildText :: line 238"  data: no]
Source File: http://www.mikelovesjulia.com/test/RTE.js
Line: 238

Expected Results:  
Returned true or false

Comment 1

[Derek Davenport]

Indeed. This is what I do to workaround this:
ForeColor:
queryCommandValue("forecolor") != '' || queryCommandValue("forecolor") != rgb(0,
0, 0)
I believe on Mac OS X it might return the default color rgb(0, 0, 0)
hilitecolor:
queryCommandValue("hilitecolor") != "transparent")
Only works in CSS mode.
CreateLink:
To see if you're in a link I just had to write a routine to check the selection
range
Indent:
queryCommandEnabled("Outdent")
but this also returns true when inside lists.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The page in the URL field is 404... is there a page showing the problem?

Comment 3

[Niels]

Attachment: document.queryCommandState() crashes always

document.queryCommandState() now crashes always, independent of its parameters. This happens with Firefox as well as with SeaMonkey. This is a serious problem because many rich-text editors do not work because of this bug. The web is full of complaints.

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.queryCommandState]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/queryCommandState-testcase.html :: onclick :: line 1"  data: no]

Firefox: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

SeaMonkey: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060130 MultiZilla/1.8.1.0p SeaMonkey/1.0

Comment 4

[Boris Zbarsky [:bzbarsky]]

Niels, does it actually crash?  Or just throw?

When I load that testcase, I see it throw because mEditingIsOn is false in the document.  Which is probably because your onload handler is set on the _document_, not on the _window_ (which is where it would actually fire).

If I fix that in your HTML, I still get an exception from QueryCommandState, but that's because we end up walking up to the Document node in nsHTMLEditor::GetCSSBackgroundColorState (due to the preparatory fix for bug 283897), and nsHTMLCSSUtils::GetElementContainerOrSelf freaks out when passed a Document node.  I've filed bug 328331 on that.

Comment 5

[Niels]

Attachment: testcase (fixed)

Comment 6

[Niels]

The browser doesn't crash.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So what is the problem, then.  Most of the buttons in that testcase work just fine, no?

Comment 8

[Niels]

(In reply to comment #7)
> So what is the problem, then.  Most of the buttons in that testcase work just
> fine, no?

26 out of 39 do not work, I would not call that 'most' :-)

The only ones working are bold, insertorderedlist, insertunorderedlist, italic, justifycenter, justifyfull, justifyleft, justifyright, strikethrough, subscript, superscript, underline, and unlink.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Brade, I just realized that we have the following code in QueryCommandState:

4143                 else {
4144                   rv = cmdParams->GetBooleanValue("state_all", _retval);
4145                   if (NS_FAILED(rv))
4146                     *_retval = PR_FALSE;
4147                 }

since GetBooleanValue returns NS_ERROR_FAILURE if there is not hashtable entry, shouldn't we also reset rv to NS_OK in addition to setting the retval?  Alternately, shouldn't we be ensuring that "state_all" is always set in commands?

Comment 10

[Kathleen :Brade]

Here is my interpretation:
 queryCommandSupported -- do we offer this command?
 queryCommandEnabled -- is icon/button for this command enabled or disabled?
 queryCommandState -- does the command apply to all of the text or partial?

When I implemented it, I went under the assumption that indent/outdent (and others) would use queryCommandEnabled but it didn't make sense to ask queryCommandState for them.  That is a limiting interpretation.

What would be good is to have a more complete specification of IE 6's behavior for each of the commands in question.  What is returned if the selection contains some text that is indented and some that is not?  I assume it is similar to bold and other style commands but it would be nice to know for sure.

We should set state_all for every command (if there is a command that shouldn't have "state_all" we could fall back to another state in this code).  I think some commands (such as hilitecolor) were added later and didn't meet all of the needs for Midas.  Those should be fixed.

I'm ok with setting rv=NS_OK but it is deceptive to return false when we don't really know its state (which is why I return/throw an error).

Comment 11

[Darin Fisher]

Not going to block 1.8.1 for this bug, but we'll happily consider a baked-on-trunk patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sounds like we need someone to look at what IE does...

Comment 13

[christian]

Hello,
Is someone working on it?
It would be great to have news on that bug.

Comment 14

[Nickolay_Ponomarev]

> Sounds like we need someone to look at what IE does...
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#dom-document-querycommandstate ?

Comment 15

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.