If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Insert HTML broken

VERIFIED FIXED in mozilla1.3alpha

Status

()

Core
Editor
--
major
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Charles Manske)

Tracking

({regression})

Trunk
mozilla1.3alpha
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
nsIController.doCommand("cmd_insertHTML") throws NS_ERROR_NOT_IMPLEMENTED
(Reporter)

Comment 1

15 years ago
This is fallout from bug 174439.
Keywords: regression
(Assignee)

Comment 2

15 years ago
*** Bug 180100 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

15 years ago
Created attachment 106418 [details] [diff] [review]
fix v1

Also contains some minor optimizations for the JS command controller
(Assignee)

Updated

15 years ago
Attachment #106418 - Flags: superreview?(sfraser)
Attachment #106418 - Flags: review?(brade)
(Assignee)

Comment 4

15 years ago
Created attachment 106443 [details] [diff] [review]
fix v2

Update, including necessary change to make sure controller ID starts at 1.
Attachment #106418 - Attachment is obsolete: true

Comment 5

15 years ago
-  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.
(Assignee)

Comment 6

15 years ago
I have fix -- taking bug
Assignee: brade → cmanske
Whiteboard: [FIX IN HAND] need r=,sr=
Target Milestone: --- → mozilla1.3alpha

Comment 7

15 years ago
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"

Updated

15 years ago
Severity: normal → major
OS: Windows 95 → All
Hardware: PC → All
(Assignee)

Comment 8

15 years ago
Created attachment 106724 [details] [diff] [review]
fix v3

I'll put the controller changes in a different bug.
Attachment #106443 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #106724 - Flags: superreview?(sfraser)
Attachment #106724 - Flags: review?(akkana)

Comment 9

15 years ago
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).
Attachment #106724 - Flags: review?(akkana) → review+

Comment 10

15 years ago
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

15 years ago
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.)

Updated

15 years ago
Attachment #106418 - Flags: superreview?(sfraser) → superreview-
(Assignee)

Updated

15 years ago
Attachment #106724 - Flags: superreview?(sfraser) → superreview?(kin)

Updated

15 years ago
Attachment #106418 - Flags: review?(brade) → review-

Comment 12

15 years ago
Comment on attachment 106724 [details] [diff] [review]
fix v3

sr=kin@netscape.com
Attachment #106724 - Flags: superreview?(kin) → superreview+
(Assignee)

Comment 13

15 years ago
checked into 1.3a trunk
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND] need r=,sr=

Comment 14

15 years ago
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;"/>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

15 years ago
Oops! That change is in the "fix v3". Simply forgot to check that file in.
(Assignee)

Updated

15 years ago
Attachment #106724 - Flags: approval1.3a?
(Reporter)

Updated

15 years ago
Flags: blocking1.3a?

Comment 16

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

Comment 17

15 years ago
Comment on attachment 106724 [details] [diff] [review]
fix v3

a=asa for checkin to 1.3a
Attachment #106724 - Flags: approval1.3a? → approval1.3a+
(Assignee)

Comment 18

15 years ago
editorOverlay.xul checked into 1.3a trunk.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Flags: blocking1.3a?

Comment 19

15 years ago
I think this is still broken in mail compose; mail compose is not using the
correct command string in 2 places
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

15 years ago
Created attachment 109030 [details] [diff] [review]
fix for mail compose v1

doh!
Attachment #106724 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #109030 - Flags: superreview?(kin)
Attachment #109030 - Flags: review?(brade)
(Assignee)

Updated

15 years ago
Whiteboard: [FIX IN HAND]need r=,sr=

Comment 21

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

sr=kin@netscape.com
Attachment #109030 - Flags: superreview?(kin) → superreview+

Updated

15 years ago
Attachment #109030 - Flags: review?(brade) → review+
(Assignee)

Comment 22

15 years ago
checked into 1.3b trunk
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Whiteboard: [FIX IN HAND]need r=,sr=
(Reporter)

Comment 23

15 years ago
I think some extra changes got checked in by mistake :-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 24

15 years ago
Created attachment 109820 [details]
cvs diff -u -r1.168 -r1.169 ComposerCommands.js
(Reporter)

Comment 25

15 years ago
Created attachment 109821 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #109821 - Flags: superreview?(kin)
Attachment #109821 - Flags: review?(cmanske)
(Assignee)

Comment 26

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

doh!
Attachment #109821 - Flags: review?(cmanske) → review+

Comment 27

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

sr=kin@netscape.com
Attachment #109821 - Flags: superreview?(kin) → superreview+
(Assignee)

Comment 28

15 years ago
checked into 1.3b trunk
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
vrfy'd fixed with 2003.04.10 comm bits.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.