Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071920 Minefield/3.0a7pre ID:2007071920 repro: open gmail press compose mail type some text highlight text and select a background color (highlight color) result: nothing, it's white and remains white, whatever you try I have no idea if this is core, dom , google or something else. This used to work, don't ask me when.

regressionrange works in 20070627_1935_firefox-3.0a6pre.en-US.win32 fails in 20070627_2053_firefox-3.0a6pre.en-US.win32 http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1182998100&maxdate=1183002779&cvsroot=%2Fcvsroot -> bug 237964

This isn't working in general, it seems. See the url that I added.

It looks to me like nsHTMLDocument::ExecCommand() is not being called when JavaScript calls editDoc.execCommand("hilitecolor", false, "#ff0000").

This test case shows that execCommand("hilitecolor", ...) doesn't work. Enter text, select it, and click the "Highlight selected text" button to verify.

I'd accidentally uploaded a test case with execCommand("hilitecolor", true, "#ff0000"), whereas it should be execCommand("hilitecolor", false, "#ff0000")! Sorry!

I take it back, it is running execCommand, but it it's getting down to nsHTMLEditor::SetCSSInlineProperty(), and deciding that since CSS isn't enabled, it should bail out and not try changing anything...

The problem is that nsHTMLEditor::GetIsCSSEnabled(PRBool *aIsCSSEnabled) (defined in editor/libeditor/html/nsHTMLEditorStyle.cpp) has mCSSAware set to false. mCSSAware is initialized in nsHTMLEditor::Init(), and is only true when we're running in composer, not as a design mode IFrame. mCSSAware is only used in nsHTMLEditor::GetIsCSSEnabled(), and here its use is flagged with a comment saying that we should remove the use of it. The return value of GetIsCSSEnabled() is determined by the config parameter editor.use_css. I think the fix for this bug is to remove mCSSAware (patch to follow). The consequences is that by default designMode IFrames will be editing in CSS mode. So you'll need to explicitly call execCommand("styleWithCSS", false, false) in JS to prevent the editor producing CSS (like for bold text, etc).

Have fix, am taking...

This removes all uses of nsHTMLEditor.mCSSAware. This has the consequence that CSS use in designMode IFrames will match the config editor.use_css, which is on by default.

You probably should request review from peterv instead, Glazman doesn't do reviews as far as I know

Comment on attachment 281142 [details] [diff] [review] Remove nsHTMLEditor.mCSSAware, fix background color problem I'm changing the reviewer to peterv, if you don't mind. Unless you specifically ask Daniel to review your patch he wouldn't notice it.

I'm confused, comment 1 claims this is a regression but afaik designMode never edited in CSS mode by default. Either it's a regression and we need to fix the regression or it just doesn't work without CSS mode and we need to fix that. I'm very hesitant to turn on CSS mode by default for designMode.

Yes, it is a regression, and indeed designMode doesn't edit in CSS mode by default. So a fix for this bug should not change that.

(In reply to comment #13) > I'm confused, comment 1 claims this is a regression but afaik designMode never > edited in CSS mode by default. Either it's a regression and we need to fix the > regression or it just doesn't work without CSS mode and we need to fix that. 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 is documented to only work in CSS mode anyway. This makes sense, as the only plain old HTML tags that can have a background color are table and body tags anyway - in order to get a background color we need to use CSS anyway. So without CSS mode it should do nothing, and it should work with CSS mode. > I'm very hesitant to turn on CSS mode by default for designMode. > I wondered it that could be a problem, I'll take another look...

New patch which ensures that mCSSAware is consistently kept in sync with the flags passed into the constructor, and nsHTMLEditor::SetFlags(). Now when you call nsHTMLEditor::SetIsCSSEnabled(), the flags and mCSSAware are updated to match. This allows the hilite command to work when styleWithCSS is on. This doesn't make CSS styling on by default, it just makes mCSSAware be true when styleWithCSS is on.

Could you write a reftest for this bug, please, and include it in your patch? (You can use a utility called cvsdo to cvs add the files so they can be included in the diff; Google will find it easily.) Basically, you write two HTML files -- say, designmode-hilitecolor.html and designmode-hilitecolor-ref.html. In the first file, you write HTML that does whatever you're testing in the way that produces a bug (so in this case, you'd use the hilitecolor command in both CSS mode and non-CSS mode to do output to a page). In the second file, you write *different* HTML that will render exactly the same as the first file, but without using exactly the same methods as you used in the first (so, perhaps, you just inline the HTML+CSS that the desigmode version generates, more or less -- I'm not familiar with exactly what the styling is you'd need to reproduce, if any). This file is the "reference" file, hence the -ref naming convention. The test passes if the two render the same, and it fails if they render differently (although you can tweak this to be pass-if-different, fail-if-same). The goal of a reftest, of course, is to ensure a bug never reoccurs; reftests are run after every checkin, and a failure is then detected immediately instead of whenever someone happens to run across a site that causes the bug to appear. For more details, see the reftest README: http://mxr.mozilla.org/mozilla/source/layout/tools/reftest/README.txt

This is the same as the previous patch, except that it adds a reftest which tests the hilitecolor command with styleWithCSS on and off.

Comment on attachment 282049 [details] [diff] [review] Path with reftest added >Index: editor/libeditor/html/nsHTMLEditor.cpp >=================================================================== >+void >+nsHTMLEditor::UpdateCSSAwareForFlags(PRUint32 aFlags) { Brace on its own line. >+ NS_ENSURE_SUCCESS(GetFlags(&flags), NS_OK); I'd do err = GetFlags(&flags); NS_ENSURE_SUCCESS(err, err); >+ NS_ENSURE_SUCCESS(SetFlags(flags), NS_OK); Ditto.

Previous patch with style changes as per review.

Comment on attachment 283249 [details] [diff] [review] Final patch with style changes as per review carrying over peterv's review

checked in.

This broke mochitests in /tests/editor/composer/test/test_bug384147.htm, so I backed it out.

The mochitest /tests/editor/composer/test/test_bug384147.html is testing the indent and outdent midas command. It is expecting compact HTML to be generated, but with my patch applied, the HTML after the indent/outdent is not as compact. For example, the first test case generates: <ol> <li>Item 1</li> <ol> <li>Item 2</li> </ol> <ol> <li>Item 3</li> </ol> </ol> <ul> <li>Item 4</li> <li>Item 5</li> </ul> Whereas it's expecting to get: <ol> <li>Item 1</li> <ol> <li>Item 2</li> <li>Item 3</li> </ol> </ol> <ul> <li>Item 4</li> <li>Item 5</li> </ul> ...And so the test is failing. These two fragments render equivalently, but are different. I've attached the edited testcase that shows how this is happening (require patch applied of course to reproduce...). Peter: is this side effect a problem? Is it ok to change the failed test case to match the new observed results, or should I look deeper into what causes this?

(In reply to comment #24) > ...And so the test is failing. These two fragments render equivalently, but are > different. They may render the same with the default styling, but add a little different styling of, say, :first-child, and they're not equivalent. I'm pretty sure we do want a single list containing all the list elements, not separate lists for each list element.

We don't want that test failing. Can you try to run the failing test with styleWithCSS explicitly set to false? The only explanation I have is that you did change the default for designMode again.

This test case allows you to set the styleWithCSS parameter and allows you to try the bold and hilite commands in a designMode iframe. This is useful, as we can determine the defaults in FireFox2 and Minefield, and we can see the effect they have.

So I added logging to the midas commands, and it looks like gmail is not setting styleWithCSS on before it calls the hilite command. But FireFox2 the hilite still works. It turns out that FF2 actually runs in styleWithCSS mode by default, whereas Minefield nolonger does. Note that the Midas docs http://developer.mozilla.org/en/docs/Midas say "[styleWithCSS] is used for toggling the format of generated content. By default (at least today), this is true." So styleWithCSS is on in FF2 by default, but it's not now. Is there a reason it got turned off? I've done some testing with my Test Case 2 from above on FF2 and trunk. FF2's behaviour: * Styles in CSS mode by default, e.g. styleWithCSS is on by default. * hilite command works by default, as styleWithCSS is on by default. Trunk Minefield's behaviour: * Does NOT style in CSS mode by default. * Hence hilite command does NOT work by default, as styleWithCSS is off by default. * With styleWithCSS on, it still doesn't style the bold command in CSS, it inserts <b> tags rather than <span style="font-weight:bold">. So either the styleWithCSS command isn't turning toggling properly, or the accessor for what it toggles is broken. * With styleWithCSS on, the hilite command doesn't work. To match FF2's behaviour, we need to fix the following: 1. styleWithCSS is not on by default, but it should be. It was in on FF2, and as it's defined to be in the midas spec. 2. Additionally, either the styleWithCSS command is not actually toggling styleWithCSS, or its accessor does not work properly any more. Does that all make sense? Do people agree?

It looks like this first regressed when the mInteractive field was added to nsEditingSession. This added a new flag nsIPlaintextEditor::eEditorAllowInteraction to the aFlags passed into nsHTMLEditor::Init(). Then the test "mCSSAware = (0 == aFlags)" began to evaluate to false, and so Init() began setting mCSSAware to 0 instead of 1, turning off mCSSAware from then onwards. So we should make a change to preserve the eEditorAllowInteraction mask to the flag, but restore the old behaviour (e.g. mCSSAware = 1).

So, I propose that we do the following: 1. I generate a new patch the same as my previous r+/sr+ patch (which turns styleWithCSS back on) with the test case that failed changed to use todo() instead of ok(). Trunk's behaviour will then again match FF2 and the midas docs. 2. I then file a new bug something along the lines of "midas indent command doesn't compact as much as it could", and reference the failed testcase. What do you guys think? Peter, your call...

[Taking]

Peter, what do you think of my suggestion in comment 30?

We should match the FF2 behaviour for styleWithCSS (even though it sucks), but does that make hilite work? But we also need to figure out why that indent/outdent Mochitest fails.

(In reply to comment #33) > We should match the FF2 behaviour for styleWithCSS (even though it sucks), but > does that make hilite work? Yes, this makes the hilite work, as GetIsCSSEnabled(), which is how we tell whether styleWithCSS is on or not, is dependant on mCSSAware. Here's its definition for reference: nsresult nsHTMLEditor::GetIsCSSEnabled(PRBool *aIsCSSEnabled) { *aIsCSSEnabled = PR_FALSE; if (mCSSAware) { // TBD later : removal of mCSSAware and use only the presence of mHTMLCSSUtils if (mHTMLCSSUtils) { *aIsCSSEnabled = mHTMLCSSUtils->IsCSSPrefChecked(); } } return NS_OK; } So by returning to having mCSSAware on by default, we allow stlyeWithCSS to pref to be accessible. Witout mCSSAware, styleWithCSS is always reported to be off, as the value of that pref is always hidden away by the check in GetIsCSSEnabled(). > But we also need to figure out why that > indent/outdent Mochitest fails. > Ok, the reason this is failing is that nsHTMLEditRules::WillIndent() splits its implementation into WillCSSIndent() and WillHTMLIndent(), and WillCSSIndent() does not have the list compacting code that exists in WillHTMLIndent(). So the easy solution is to just copy and paste it over to get the same behaviour.

Fixes mCSSAware regression as in previous patch, but also copies the "compacting" code for list indenting into the CSS indenting. It's kind of a separate bug, but it needs to be checked in at the same time as the mCSSAware patch, as without the compacting code, the mCSSAware patch will fail tests, and the compacting bug won't show up unless the mCSSAware patch is in.

Highlight is broken for me with the new Gmail release (in early Nov) in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007110804 Minefield/3.0b2pre When I select text and highlight, the cursor just jumps down a few rows.

Comment on attachment 286110 [details] [diff] [review] Patch - fixes indent with css, fixes mCSSAware regression Sorry for the long wait! >Index: editor/libeditor/html/nsHTMLEditRules.cpp >=================================================================== >@@ -3572,46 +3572,77 @@ nsHTMLEditRules::WillCSSIndent(nsISelect Bah, the duplication of code between WillHTMLIndent and WillCSSIndent is ridiculous. Could you file a bug to merge them? >+ } >+ sibling = nsnull; >+ Don't you need this bit from WillHTMLIndent: // check to see if curList is still appropriate. Which it is if // curNode is still right after it in the same list. if (curList) mHTMLEditor->GetPriorHTMLSibling(curNode, address_of(sibling)); >@@ -3621,26 +3652,27 @@ nsHTMLEditRules::WillCSSIndent(nsISelect > if (NS_FAILED(res)) return res; >+ Don't add this. >Index: editor/libeditor/html/nsHTMLEditor.cpp >=================================================================== >+nsHTMLEditor::UpdateCSSAwareForFlags(PRUint32 aFlags) >+{ >+ mCSSAware = ((aFlags & (eEditorNoCSSMask | eEditorMailMask)) == 0); >+} >+ >+ Don't add the two blank lines. >@@ -5439,16 +5446,37 @@ nsHTMLEditor::RemoveAttributeOrEquivalen > nsresult > nsHTMLEditor::SetIsCSSEnabled(PRBool aIsCSSPrefChecked) >+ } else { >+ // Turn on NoCSS, as we're disabling CSS. >+ if (!(flags & eEditorNoCSSMask)) { } else if (... >Index: editor/libeditor/html/nsHTMLEditor.h >=================================================================== >+ void UpdateCSSAwareForFlags(PRUint32 aFlags); I'd just name this UpdateForFlags and inline it.

Patch v7 with Peterv's suggestions. r=peterv.

(In reply to comment #37) > Bah, the duplication of code between WillHTMLIndent and WillCSSIndent is > ridiculous. Could you file a bug to merge them? Filed Bug 403409 for this.

Comment on attachment 288225 [details] [diff] [review] Patch v8 blocking bugs don't need patch approvals at this time..

Checking in editor/libeditor/html/nsHTMLEditRules.cpp; /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v <-- nsHTMLEditRules.cpp new revision: 1.339; previous revision: 1.338 done Checking in editor/libeditor/html/nsHTMLEditor.cpp; /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,v <-- nsHTMLEditor.cpp new revision: 1.563; previous revision: 1.562 done Checking in editor/libeditor/html/nsHTMLEditor.h; /cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.h,v <-- nsHTMLEditor.h new revision: 1.237; previous revision: 1.236 done Checking in layout/reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.200; previous revision: 1.199 done

Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre

This still doesn't work for me using: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111504 Minefield/3.0b2pre ID:2007111504

Henry, what doesn't still work for you, the GMail compose background color?

Yes, the Gmail rich text compose background color doesn't work. If I apply a background color, I get the same behavior as I described earlier (cursor jumps down a few rows).

Damnit, the bug is fixed on Windows, but not on Mac. Reopening, OS -> Mac.

It didn't even work in [Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre], which is the first nightly after it landed. It's most likely not a regression on Mac, unless someone checked something in that broke it after the checkin but before the nightly build on Nov 14.

Ho hum, this is not the same problem as in the Windows version. The original regression that caused the bug on Windows landed on 2007-06-27. In the Windows nightly build for 2007-06-22 gmail hilight works, but in [Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/20070622 Minefield/3.0a6pre] it does not. It also looks like gmail isn't executing the hilite midas command. So I think that the problem on Mac is not the same as the one on Windows, i.e. we have a new problem; the regression that caused this to not work on Mac is different to the Windows one.

It's probably better to file a new bug for that (with the necessary useful info). Also, a regression range for the mac issue would probably be helpful.

(In reply to comment #49) > It's probably better to file a new bug for that (with the necessary useful > info). > Also, a regression range for the mac issue would probably be helpful. > Regression range is 2006-09-28-08 to 2006-09-29-06, I've filed bug 404433 for this.

os back to -> XP status back to -> FIXED

Moving this back to verified, as it works fine on Windows using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007113005 Minefield/3.0b2pre.