henktiggelaar Reporter
	Description • 19 years ago

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

The useCSS property of the embedded rich text editor has changed its meaning 
(true is now false and vice versa). This breaks almost every online WYSIWYG 
editor online with respect to clean HTML code generation.

The readonly property is also affected in the same way.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.mozilla.org/editor/midasdemo/
2. Notice how the "Use CSS" does exactly the opposite of what it is told
3. Also notice the same problem occurs with the "Read only" checkbox

Actual Results:  
The editor used css for things like bold and italic when "Use CSS" was turned 
off and standard HTML tags when "Use CSS" was on.

Expected Results:  
The editor should have used css for things like bold and italic when "Use CSS" 
was turned on and standard HTML tags when "Use CSS" was off

	Phil Randal
	Comment 1 • 19 years ago

Confirmed in a build I've made from the trunk CVS this morning.

	Bastian Grupe
	Comment 2 • 19 years ago

Confirmed for the official trunk build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050723
Firefox/1.0+ ID:2005072306

	Asa Dotzler [:asa]
	Comment 3 • 19 years ago

Johnny, is this something you can look into for 1.5?

	Martijn Wargers (dead)
	Comment 4 • 19 years ago

This regressed between 2005-06-01 and 2005-06-02, so bug 285873 is very likely
to be the culprit, especially this change:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/editor/composer/src&command=DIFF_FRAMESET&file=nsComposerDocumentCommands.cpp&rev1=1.11&rev2=1.12&root=/cvsroot
I guess the changing from "else if" to "if" is causing this.

	Kathleen :Brade
	Comment 5 • 19 years ago

I haven't had a build since I upgraded my OS.  However, I suggest someone put a
breakpoint in HTMLEditor::SetIsCSSEnabled() and see if mHTMLCSSUtils is
set/initialized.  Maybe nsHTMLDocument is setting the flag before the editor is
done initializing itself so the state gets toggled backwards?

http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditor.cpp#5484

I don't think the problem is in the command code.

	Martijn Wargers (dead)
	Comment 6 • 19 years ago

Well, I get: $2 = (nsHTMLCSSUtils *) 0xdc0d758
when I turn Use CSS on.

	Martijn Wargers (dead)
	Comment 7 • 19 years ago

Ok, this part of the patch from bug 285873 seems to be the cause, I've checked
it by manually backing it out:
-      NS_ConvertUCS2toUTF8 convertedParam(inParam);
-
       if (outIsBoolean) {
         // if this is a boolean value and it's not explicitly false
         // (e.g. no value) we default to "true"
-        outBooleanValue = convertedParam.Equals("false",
-                                   nsCaseInsensitiveCStringComparator());
+        outBooleanValue = !inParam.LowerCaseEqualsLiteral("false");
         outParam.SetLength(0);
       }
       else {
+        NS_ConvertUCS2toUTF8 convertedParam(inParam);

	henktiggelaar Reporter
	Comment 8 • 19 years ago

I don't understand why this bug is set to not block FF 1.5. It would be pretty 
bad if this bug found its way into a release version.

	Martijn Wargers (dead)
	Comment 9 • 19 years ago

I bet it should be this.
Because you can see it in the code that caused it:
-	 outBooleanValue = convertedParam.Equals("false",
-				    nsCaseInsensitiveCStringComparator());
+	 outBooleanValue = !inParam.LowerCaseEqualsLiteral("false");

	Johnny Stenback (:jst)
	Comment 10 • 19 years ago

(In reply to comment #9)
> Created an attachment (id=193394) [edit]
> patch
> 
> I bet it should be this.
> Because you can see it in the code that caused it:
> -	 outBooleanValue = convertedParam.Equals("false",
> -				    nsCaseInsensitiveCStringComparator());
> +	 outBooleanValue = !inParam.LowerCaseEqualsLiteral("false");
> 

So here's the problem, look at that code, if convetedParam equals "false",
shouldn't the resulting boolean value be false? The reason I changed that was
that another boolean argument that I added just worked plain backwards, period.
This proposed change would make that argument ("insertBrOnReturn") work
backwards, period.

Could this be fixed only for old (mozilla specific) "broken" boolean arguments
that have been broken for a long time instead of leaving this logic completely
backwards?

I do think this is something we should think *hard* about for 1.5.

	Kathleen :Brade
	Comment 11 • 19 years ago

Somewhere, something is wrong.  When I looked at the code in nsHTMLDocument.cpp,
I couldn't understand why the old code didn't match the comment that I wrote for
it.  Perhaps there is something subtle going on which didn't get documented in
the code?

It seems to me that the code is correct now (as it is checked in).

It would be helpful if someone could step through the code in
nsHTMLDocument::ExecCommand() and see where it seems to go wrong.  I don't have
a tree to step through.  The key part spans from where we set the boolean value
on cmdParams through the DoCommand call (which eventually calls
htmleditor->SetIsCSSEnabled(desireCSS)).

	Brendan Eich [:brendan]
	Comment 12 • 19 years ago

Compatibility trumps consistency, truth and beauty lose.

This needs major documentation in the code and on devmo.

Plus'ing for fx1.5/moz1.8.

/be

	Kathleen :Brade
	Comment 13 • 19 years ago

Comment on attachment 193394 [details] [diff] [review]
patch

I don't mean to be a pain.  I am fine with what Brendan says.  However, making
the paragraph vs br command backward isn't right either.  This patch is
incomplete imo so I'm minusing.

brade: r-

	Brendan Eich [:brendan]
	Comment 14 • 19 years ago

(In reply to comment #13)
> (From update of attachment 193394 [details] [diff] [review] [edit])
> I don't mean to be a pain.  I am fine with what Brendan says.  However, making
> the paragraph vs br command backward isn't right either.  This patch is
> incomplete imo so I'm minusing.

I think we all agree that new commands should not be backwards.

Perhaps we should present a uniform model by adding synonyms for the backwards
commands that have the correct boolean sense.

/be

	Kathleen :Brade
	Comment 15 • 19 years ago

Ooops! I mistyped in comment 13.  I meant to say:
  However, making the paragraph vs br command BROKEN isn't right either.

It's not at all clear to my why there are "old" commands and "new" commands. 
I'd really like to understand why they behave differently but I don't have the
cycles to debug it myself.  Could someone try what I suggest in comment 11 and
let me know what they find?  Thanks!

	Peter Van der Beken [:peterv] Assignee
	Comment 16 • 19 years ago

(In reply to comment #15)
> It's not at all clear to my why there are "old" commands and "new" commands. 

Because some of the commands (the "old" ones) have always been backwards afaik.
If you look at the source of http://www.mozilla.org/editor/midasdemo/ you'll see
that it does:

function usecss(source)
{
  document.getElementById('edit').contentWindow.document.execCommand("useCSS",
false, !(source));  
}
...
<input checked type="checkbox" onclick="usecss(this.checked)">

Note the '!(source)'. It is the exact oposite of what you actually mean:
checking the "Use CSS" checkbox should mean set useCSS to true and thus enable
CSS in the editor but you've always had to set it to false to enable CSS in the
editor.

> I'd really like to understand why they behave differently but I don't have the
> cycles to debug it myself.  Could someone try what I suggest in comment 11 and
> let me know what they find?  Thanks!

I don't think there's much to understand, the code was backwards from the start:
see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.463&mark=4272-4275#4270
and
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/composer/src/nsComposerDocumentCommands.cpp&rev=1.1&mark=140-145#135.
Jst just tried to make the commands he added behave sanely, which broke the
"old" commands.

	Peter Van der Beken [:peterv] Assignee
	Comment 17 • 19 years ago

Taking.

	Peter Van der Beken [:peterv] Assignee
	Comment 18 • 19 years ago

This adds two new commands (I've named them styleWithCSS and contentReadOnly,
but I'm open to better suggestions). I also added some code to map the "old"
useCSS and readonly commands to the new ones. We actually invert the value when
someone uses the old commands so that we're backwards (no pun intended)
compatible.

	Kathleen :Brade
	Comment 19 • 19 years ago

Comment on attachment 193605 [details] [diff] [review]
v1

This patch seems fine to me.  I like the new command names and the remapping to
get backward compatibility.

What I'd also like to see checked in with this patch is:
 * updates to editor/doc/ files to add new commands and remove useCSS and
readonly commands.
 * update Midas example to use the new (correct) commands.

Peter--THANK YOU for tracking down the problem.  I am relieved that it was a
bad example / test case (as opposed to a core problem).

	Brendan Eich [:brendan]
	Comment 20 • 19 years ago

Comment on attachment 193605 [details] [diff] [review]
v1

sr+a=me, thanks!  Maybe we should get deb@dria.org in on this so devmo can
document midas fully.

/be

	Peter Van der Beken [:peterv] Assignee
	Comment 22 • 19 years ago

Comment on attachment 193610 [details] [diff] [review]
update of midas documentation

Thanks.

>Index: midas-spec.html
>===================================================================

>   </tbody>

I'd also add this here:

  <tbody>
    <tr>
      <td style="vertical-align: top;" colspan="3">DEPRECATED COMMANDS</td>
    </tr>
    <tr>
      <td style="vertical-align: top;">readonly</td>
      <td style="vertical-align: top;">true<br>false</td>
      <td style="vertical-align: top;">This command has been replaced
with contentReadOnly. It takes the same values as contentReadOnly, but the
meaning of true and false are inversed.</td>
    </tr>
    <tr>
      <td style="vertical-align: top;">useCSS</td>
      <td style="vertical-align: top;">truefalse</td>
      <td style="vertical-align: top;">This command has been replaced
with styleWithCSS. It takes the same values as styleWithCSS, but the
meaning of true and false are inversed.</td>
    </tr>
  </tbody>

> </table>

	Martijn Wargers (dead)
	Comment 23 • 19 years ago

Brade, maybe also add the inserthtml command? See bug 184509. It was never added
to the documentation.

	Kathleen :Brade
	Comment 24 • 19 years ago

Martijn Wargers (comment 23) -- Actually that checkin was made to the mozilla
source tree.  The problem is that no one updated the mozilla.org web site
(http://www.mozilla.org/editor/midas-spec.html).  At the moment, I don't have a
tree for the web site; can someone else update it by copying the file from the
source tree into the web site tree?  (Or is there a shortcut way to do this that
I've forgotten about?)

PeterV (comment 22) -- I'm not a big fan of documented deprecated commands.  :-(

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 25 • 19 years ago

Brade, you can log in with your CVS account using the link Edit this Page.

http://doctor.mozilla.org/?file=mozilla-org/html/editor/midas-spec.html

	Kathleen :Brade
	Comment 26 • 19 years ago

I checked in the editor/docs/midas-spec.html file (trunk).

	Peter Van der Beken [:peterv] Assignee
	Comment 27 • 19 years ago

(In reply to comment #24)
> I'm not a big fan of documented deprecated commands.  :-(

Older clients (FF 1.0) don't support the new commands, so if people want to
support multiple versions of our product they need to know about the deprecated
commands. We really should document midas properly.

We also should implement queryCommandSupported, right now there's no way to
support multiple versions of our product without checking for version numbers.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 28 • 19 years ago

I'd be happy to start getting the existing Editor and Midas documents migrated
to Devmo if someone could point me at the current/relevant docs.  The only
version of the midas-spec I can find at mozilla.org says "Last revised: 
November 15, 2002", while the patch is for a file that says "Last revised: 
May 30, 2003".

Is the most current version of this (and any other related docs) on the web
somewhere?  If they are, please tell me where and I'll find a spot for it in the
Devmo wiki.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 29 • 19 years ago

Also, do we want everything under http://www.mozilla.org/editor/ migrated into
Devmo?

	Peter Van der Beken [:peterv] Assignee
	Comment 30 • 19 years ago

The latest version of that file is at
http://lxr.mozilla.org/seamonkey/source/editor/docs/midas-spec.html
I have no idea if we want to migrate everything under
http://www.mozilla.org/editor/, maybe brade can comment on that?

	Peter Van der Beken [:peterv] Assignee
	Comment 31 • 19 years ago

Checked in on trunk and branch.

	Kathleen :Brade
	Comment 32 • 19 years ago

stephen.donner@gmail.com (comment 25): I used doctor as you suggested.  Sadly,
it doesn't let me upload a file; instead I had to copy/paste the contents which
wasn't very convenient for me (I use vi for editing).  Is there a bugzilla
component to suggest fixes/features to doctor?

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 33 • 19 years ago

(In reply to comment #32)
> stephen.donner@gmail.com (comment 25): I used doctor as you suggested.  Sadly,
> it doesn't let me upload a file; instead I had to copy/paste the contents which
> wasn't very convenient for me (I use vi for editing).  Is there a bugzilla
> component to suggest fixes/features to doctor?

Yes, Product: Webtools, Component: Doctor.

	Kathleen :Brade
	Comment 34 • 19 years ago

from comment 32 and comment 33, rfe bug filed:  bug 306277