279330 - Midas hilitecolor command only works in CSS mode

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Derek Davenport]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

In the page at http://derekdev.com/mozilla/hilite.html the hilitecolor command
only works when in CSS mode. When not in CSS mode nothing happens (but the
method still returns true).

Reproducible: Always

Steps to Reproduce:
1. Turn design mode "on" on a document (doc points to a document):
doc.designMode = "on";
2. Turn CSS mode off (true is on, false is off): doc.execCommand("useCSS",
false, false);
3. Select some text in the document and then try to highlight it (here I use
Red): doc.execCommand("hilitecolor", false, "Red");

Actual Results:  
Nothing changes.

Expected Results:  
Highlight the selected text Red.

In CSS mode when you highlight something, a span tag wraps around the selection
and has the background-color style set. But all tags inside the selection are
also given the background-color: style and the new highlight span stops and then
starts before and after them. Like this:

here is <b>some</b> text

becomes

<span style="background-color: Red;">here is </span><b style="background-color:
Red;">some</b><span style="background-color: Red;"> text</span>

In my project I was not using CSS mode, but when I added the highlight ability
it did not work. I discovered it only worked when in CSS mode so I turned on CSS
mode before the highlight then after I turned it off. But I found that when I
did this all tags inside the selection get altered as I outlined above. This is
quite a mess I'd have to clean up as after the user is done I need to convert
the HTML into another format (<u> to [u] and so on which is why I'm not using
CSS mode).

Comment 1

[Alfonso Martinez]

Moving to Core->Editor

Comment 2

[Chris Moschini]

This bug actually gets worse:

1) Click anywhere in a document in DesignMode.
2) Turn CSS mode off.
3) DON'T select any text (collapsed selection)
4) Turn on hiliteColor

Nothing appears to happen. However:

5) Type some text.

Visually, still, nothing occurs but in the underlying HTML Firefox has just wrapped the text typed in a font tag with a bgcolor attribute - which it further will not render.

At step 3 the HTML might look like this:
Some text

End of step 4 it looks the same. End of step 5 it looks like this:
Some text<font bgcolor="#ffff33">Some highlighted text</font>

Comment 3

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

The midas docs are here: http://developer.mozilla.org/en/docs/Midas

They say: "[The hilitecolor] command will set the hilite color of the selection or at the insertion point. It only works with styleWithCSS enabled."

So hilitecolor should only work in CSS mode. This makes sense, as there's no way to set the background attribute in plain old HTML.

Bug 388980 is related to this one, and my proposed patch for that prevents bad things happening reported in Comment #2.

Comment 4

[Chris Moschini]

Doing nothing in non-CSS mode works well for us - kicking in a font tag on first letter typed creates a problem. Should the fix in the related bug fix the delayed font tag issue?

Interestingly I can't repro the delayed font tag in the attached test case. In our usage, when the user chooses a highlight/background color with a collapsed selection, we immediately return focus back to the editable field, programatically, while in the testcase focus moves to the clicked button. Maybe the change in focus is important to reproducing the delayed font tag problem.

Comment 5

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

With the patch applied, the hilitecolor command will never do anything when we're not in styleWithCSS mode, which means it won't insert a font tag. So since styleWithCSS will be on by default, and the hilitecolor command now honours that, FireFox should not insert font tags, preventing your delayed font tag problem.

Comment 6

[Chris Moschini]

Great - thanks for the fix!

Comment 7

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

(In reply to comment #3)
> The midas docs are here: http://developer.mozilla.org/en/docs/Midas
> 
> They say: "[The hilitecolor] command will set the hilite color of the selection
> or at the insertion point. It only works with styleWithCSS enabled."
> 
> So hilitecolor should only work in CSS mode. This makes sense, as there's no
> way to set the background attribute in plain old HTML.
> 
> Bug 388980 is related to this one, and my proposed patch for that prevents bad
> things happening reported in Comment #2.
> 

Since the midas hilite command is defined to only work with styleWithCSS anyway, I'm marking this as WONTFIX.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

There is now a spec for this: <http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-hilitecolor-command>.  It says hilitecolor works the same regardless of styleWithCSS.  This matches WebKit and Opera (IE doesn't implement the command).  This also causes failures in richtext2.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

The change to nsHTMLEditorStyle.cpp is mostly outdenting.  I'll attach a separate patch with -w so it can be reviewed more easily.


This fixes the following richtext2 tests:

1015 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, HC:blue_TEXT-1_SI, dM] used to fail, but it just started passing - 1 should equal 1
1018 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, HC:blue_TEXT-1_SI, body] used to fail, but it just started passing - 1 should equal 1
1021 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, HC:blue_TEXT-1_SI, div] used to fail, but it just started passing - 1 should equal 1
1282 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1284 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1286 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1288 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1292 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1294 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1296 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1298 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1302 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1304 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1306 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1308 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, body] used to fail, but it just started passing - 1 should equal 1


I searched comm-central's mxr for uses of the removed method and didn't find any beyond the ones I'm removing from mozilla-central.


I don't really like the way this patch works.  There are already a bunch of commands that only work via HTML, but this makes hiliteColor the only command that only works via CSS, and the way I did it is a bit weird.  But it seems to work, and I don't see a better way given the abstractions at work here.  Let me know if you want it done some other way.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Diff of nsHTMLEditorStyle.cpp with -w for readability

Comment 11

[(no longer active)]

Comment on attachment 612818 [details] [diff] [review]
Patch v1

Review of attachment 612818 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is acceptable given the current way the things work.  :/

Comment 12

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 612818
	Branch: mozilla-central => try
Patch 612818 could not be applied to mozilla-central.
patching file editor/libeditor/html/tests/browserscope/lib/richtext/currentStatus.js
Hunk #1 FAILED at 16
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/tests/browserscope/lib/richtext/currentStatus.js.rej
patching file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js
Hunk #1 FAILED at 4536
Hunk #3 FAILED at 5909
Hunk #4 FAILED at 5944
Hunk #5 FAILED at 5979
Hunk #8 FAILED at 9373
Hunk #9 FAILED at 23824
6 out of 9 hunks FAILED -- saving rejects to file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

New try run (together with bug 738385): https://tbpl.mozilla.org/?tree=Try&rev=863b4acac1d4

Comment 14

[Mozilla RelEng Bot]

Try run for 863b4acac1d4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=863b4acac1d4
Results (out of 227 total builds):
    success: 173
    warnings: 54
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-863b4acac1d4

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Convert a reftest to a mochitest

The try run turned up a reftest failure from bug 388980.  I didn't catch this locally because I don't run reftests when working on editor code, certainly not in layout/reftests/.  This converts the reftest to a mochitest and moves it to editor/libeditor/html/tests/, which is where one would expect it.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Remove erroneous NS_ERRORs

The patch caused editor/libeditor/html/crashtests/448329-3.html to start failing with assertions like

###!!! ASSERTION: JoinNode called with a non-text left node!: 'Error', file ../../../../editor/txtsvc/src/nsTextServicesDocument.cpp, line 1904

I looked at the stack traces and think that the assertions are bogus.  nsEditor::JoinNodes is supposed to be callable with non-text nodes; it can be used to join elements.  It calls DidJoinNodes() on each listener.  This has the desired effect for nsHTMLEditRules::DidJoinNodes; for nsTextServicesDocument::DidJoinNodes it does nothing.  I don't think that calling it for nsTextServicesDocument is an error, so it should just return instead of calling NS_ERROR.  If it were really an error to pass a non-text node to DidJoinNodes, the parameter should be nsIDOMTextNode instead of nsIDOMNode.

However, I'm not familiar enough with this code to be sure this is right.  Specifically, I don't know what exactly nsEditor and nsTextServicesDocument are, or why they register different listeners.  So maybe I'm wrong here.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v2 -- execCommand("hilitecolor") should work even in non-CSS mode

This is identical to v1, which you already r+'d, except that it updates test_bug388980.html (which was converted to a mochitest by the new part 1 of this series).

These three patches should now cause no failures.  See <https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3> (failures are only due to additional patches on top of these three); and <https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c> (which I hope should be green).

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Note: setting styleWithCSS to false by default (bug 738366) means that hiliteColor now does nothing by default, which is a regression that hits RTE demos (bug 745543).  It probably doesn't affect real-world editors, because they all set styleWithCSS to false anyway, but it's still a regression.  In retrospect, we should have landed this before bug 738366, but it's too late for that now.  Given this, r+ with comments or followups might be preferable to r- if there are problems with the new patches.

Comment 20

[(no longer active)]

Comment on attachment 615130 [details] [diff] [review]
Patch part 1, v1 -- Convert a reftest to a mochitest

Review of attachment 615130 [details] [diff] [review]:
-----------------------------------------------------------------

We should not convert this to a mochitest.  Reftests are lighter wait tests and do not depend on any permissions, which are both good properties.  Plus, they can potentially be shared with other engines at some point.

Please let me know if you had very specific reasons to convert this to a mochitest.

Comment 21

[(no longer active)]

Comment on attachment 615131 [details] [diff] [review]
Patch part 2, v1 -- Remove erroneous NS_ERRORs

Review of attachment 615131 [details] [diff] [review]:
-----------------------------------------------------------------

Please also return NS_OK in those cases instead of NS_ERROR_FAILURE.

Comment 22

[(no longer active)]

Comment on attachment 615132 [details] [diff] [review]
Patch part 3, v2 -- execCommand("hilitecolor") should work even in non-CSS mode

As I said I don't think we should convert the test to a mochitest.  But I don't think this required another round of reviews anyways.  :-)

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> We should not convert this to a mochitest.  Reftests are lighter wait tests
> and do not depend on any permissions, which are both good properties.  Plus,
> they can potentially be shared with other engines at some point.

Are they really lighter-weight?  They have to load two pages, not one.  I'd have guessed that for trivial files like this, page-load time would swamp everything else.  Also, mochitests that just use ok() are easy to share with other engines too.  And I find mochitests significantly easier to read, because you only have to look at one file and can just read it straight through.  And you can test all sorts of things that can't be tested with a reftest.

But okay.

> Please let me know if you had very specific reasons to convert this to a
> mochitest.

Just that before I post a patch, I run editing-related tests locally, namely editor/ and content/html/document/.  I'm not going to run all of layout/reftests/bugs/ for every patch.  There's no reftests directory for editor/ -- maybe the test could be moved to content/html/document/reftests/?  There are already editing-related tests in content/html/document/test/ because that's where execCommand() lives.

(In reply to Ehsan Akhgari [:ehsan] from comment #21)
> Please also return NS_OK in those cases instead of NS_ERROR_FAILURE.

Will do.

(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> As I said I don't think we should convert the test to a mochitest.  But I
> don't think this required another round of reviews anyways.  :-)

Noted.

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e2737720a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/459b7d9d63d7

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/52e2737720a5
https://hg.mozilla.org/mozilla-central/rev/459b7d9d63d7

Comment 26

[(no longer active)]

(In reply to Aryeh Gregor from comment #23)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > We should not convert this to a mochitest.  Reftests are lighter wait tests
> > and do not depend on any permissions, which are both good properties.  Plus,
> > they can potentially be shared with other engines at some point.
> 
> Are they really lighter-weight?  They have to load two pages, not one.  I'd
> have guessed that for trivial files like this, page-load time would swamp
> everything else.  Also, mochitests that just use ok() are easy to share with
> other engines too.  And I find mochitests significantly easier to read,
> because you only have to look at one file and can just read it straight
> through.  And you can test all sorts of things that can't be tested with a
> reftest.

Oh, sorry, they're ligher weight in terms of the framework.  Generally I prefer writing tests in the smallest least privileged framework possible.  The reftest/crashtest framework runs tests with the same privilege as regular web pages, hence my preference.

> > Please let me know if you had very specific reasons to convert this to a
> > mochitest.
> 
> Just that before I post a patch, I run editing-related tests locally, namely
> editor/ and content/html/document/.  I'm not going to run all of
> layout/reftests/bugs/ for every patch.  There's no reftests directory for
> editor/ -- maybe the test could be moved to content/html/document/reftests/?
> There are already editing-related tests in content/html/document/test/
> because that's where execCommand() lives.

OK, I see.  Note that we have quite a few reftests in layout/reftests/editor, and you should run them locally in order to verify your patches.  We also have crashtests in editor/crashtest.list, which should also be run for editor changes.  But we do have this general problem where tests for a component are spread all over the tree (some of it being my own fault), and if you ever wanted to move more of the editor related reftests to layout/reftests/editor, and crashtests and mochitests to editor/, you'll have my r+ and gratitude.  :-)

But in general, I usually run mochitest-plain/mochitest-chrome/crashtest in editor/ and reftest in layout/reftests/editor, and leave the rest to the try server.  :-)