180303 - Insert HTML broken

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[[e-mail]]

nsIController.doCommand("cmd_insertHTML") throws NS_ERROR_NOT_IMPLEMENTED

Comment 1

[[e-mail]]

This is fallout from bug 174439.

Comment 2

[Charles Manske]

*** Bug 180100 has been marked as a duplicate of this bug. ***

Comment 3

[Charles Manske]

Attachment: fix v1

Also contains some minor optimizations for the JS command controller

Comment 4

[Charles Manske]

Attachment: fix v2

Update, including necessary change to make sure controller ID starts at 1.

Comment 5

[Simon Fraser [no longer active]]

-  commandManager.registerCommand("cmd_insertHTML",    nsInsertHTMLCommand);
+  commandManager.registerCommand("cmd_insertHTMLDlg", nsInsertHTMLDlgCommand);

Please rename cmd_insertHTMLDlg to something more comprehensible.

Before we go having special commands that bring up UI, I'd like us to come up
with a policy of when commands can trigger UI. We have to address this for Midas
and emebedders sonner or later.

Comment 6

[Charles Manske]

I have fix -- taking bug

Comment 7

[Kathleen :Brade]

Comment on attachment 106443 [details] [diff] [review]
fix v2

I don't understand the controller changes.  I think those should be part of
another patch/bug.  That will help if any regressions are caused by the
controller changes (less to back out and clearer what the original problem
is/was.  In particular, I don't understand this block:
+  // Don't need to do this if already done
+  if (gComposerWindowControllerID)
+    return;
Why isn't there a similar optimization for gComposerJSCommandControllerID?

I don't think cmd_insertHTML should be in this list:
    || commandID == "cmd_insertHTML"
+   || commandID == "cmd_insertHTMLDlg"

Comment 8

[Charles Manske]

Attachment: fix v3

I'll put the controller changes in a different bug.

Comment 9

[Akkana Peck]

Comment on attachment 106724 [details] [diff] [review]
fix v3

noUIStateUpdateNeeded still refers to cmd_insertHTML.  Isn't that command
nonexistant now?

cmanske says that whole switch table is going away as part of some other bug,
so it's not a big deal, but just in case that one gets delayed, it might be
nice to change the instance as part of this checkin (if you do that, no need to
make another patch).

Comment 10

[Simon Fraser [no longer active]]

What's the deal with commands that bring up UI? Should we look at the midas APIs
to decide how to do this? Maybe it should just be a param (or lack of params)
that indicates that UI should be thrown?

Comment 11

[Kathleen :Brade]

I wonder if we should be consistent and use "Properties" instead of "Dialog"
like cmd_objectProperties and other commands already have?  For this particular
bug, we should do something to fix the regression.  A new bug should be filed to
address the general issue of "UI" param for all appropriate commands.

Akkana points out:
>noUIStateUpdateNeeded still refers to cmd_insertHTML.  Isn't that command
>nonexistant now?
She is partially right, cmd_insertHTML shouldn't appear in that list.  The
command does exist but it's in C++ and shouldn't be in that list.  (The reason
we have a problem today is that there are two commands with identical string and
we are finding the wrong one.)

Comment 12

[kinmoz]

Comment on attachment 106724 [details] [diff] [review]
fix v3

sr=[e-mail]

Comment 13

[Charles Manske]

checked into 1.3a trunk

Comment 14

[Dan M]

Not working for me in trunk build from yesterday morning.
However it does work for me if I make this modification:

Index: editor/ui/composer/content/editorOverlay.xul
===================================================================
RCS file: /cvsroot/mozilla/editor/ui/composer/content/editorOverlay.xul,v
retrieving revision 1.222
diff -r1.222 editorOverlay.xul
123c123
<     <command id="cmd_insertHTMLWithDialog"
oncommand="goDoCommand('cmd_insertHTMLDlg')"  label="&insertHTMLCmd.label;"/>
---
>     <command id="cmd_insertHTMLWithDialog"
oncommand="goDoCommand('cmd_insertHTMLWithDialog')"  label="&insertHTMLCmd.label;"/>

Comment 15

[Charles Manske]

Oops! That change is in the "fix v3". Simply forgot to check that file in.

Comment 16

[Jason Bassford]

*** Bug 183699 has been marked as a duplicate of this bug. ***

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 106724 [details] [diff] [review]
fix v3

a=asa for checkin to 1.3a

Comment 18

[Charles Manske]

editorOverlay.xul checked into 1.3a trunk.

Comment 19

[Kathleen :Brade]

I think this is still broken in mail compose; mail compose is not using the
correct command string in 2 places

Comment 20

[Charles Manske]

Attachment: fix for mail compose v1

doh!

Comment 21

[kinmoz]

Comment on attachment 109030 [details] [diff] [review]
fix for mail compose v1

sr=[e-mail]

Comment 22

[Charles Manske]

checked into 1.3b trunk

Comment 23

[[e-mail]]

I think some extra changes got checked in by mistake :-(

Comment 24

[[e-mail]]

Attachment: cvs diff -u -r1.168 -r1.169 ComposerCommands.js

Comment 25

[[e-mail]]

Attachment: Back out setTimeout changes

I'm just backing out the added ()s, not the whitespace changes, so the patch
isn't quite identical to the previous diff.

Comment 26

[Charles Manske]

Comment on attachment 109821 [details] [diff] [review]
Back out setTimeout changes

doh!

Comment 27

[kinmoz]

Comment on attachment 109821 [details] [diff] [review]
Back out setTimeout changes

sr=[e-mail]

Comment 28

[Charles Manske]

checked into 1.3b trunk

Comment 29

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.04.10 comm bits.