388980 - Gmail compose mail (midas), background color doesn't work

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P4)

Description

[Peter van der Woude [:Peter6]]

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.

Comment 1

[Peter van der Woude [:Peter6]]

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

Comment 2

[Martijn Wargers (dead)]

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

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

Comment 4

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

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

Comment 5

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

Attachment: Test case showing designMode hilitecolor execCommand isn't working

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

Comment 6

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

Attachment: [Fixed] Testcase

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

Comment 7

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

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...

Comment 8

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

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).

Comment 9

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

Have fix, am taking...

Comment 10

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

Attachment: Remove nsHTMLEditor.mCSSAware, fix background color problem

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 12

[Martijn Wargers (dead)]

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.

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

[Martijn Wargers (dead)]

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.

Comment 15

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

(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...

Comment 16

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

Attachment: Make use of mCSSAware consistent, fixes bug

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.

Comment 17

[Jeff Walden [:Waldo]]

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

Comment 18

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

Attachment: Path with reftest added

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 19

[Peter Van der Beken [:peterv]]

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.

Comment 20

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

Attachment: Final patch with style changes as per review

Previous patch with style changes as per review.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 283249 [details] [diff] [review]
Final patch with style changes as per review

carrying over peterv's review

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 24

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

Attachment: Modification of the broken mochitest, showing how it fails

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?

Comment 25

[Jeff Walden [:Waldo]]

(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.

Comment 26

[Peter Van der Beken [:peterv]]

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.

Comment 27

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

Attachment: Test case 2 - determine styleWithCSS default values

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.

Comment 28

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

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?

Comment 29

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

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).

Comment 30

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

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...

Comment 31

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

[Taking]

Comment 32

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

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

Comment 33

[Peter Van der Beken [:peterv]]

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.

Comment 34

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

(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.

Comment 35

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

Attachment: Patch - fixes indent with css, fixes mCSSAware regression

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.

Comment 36

[Henry Bridge]

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 37

[Peter Van der Beken [:peterv]]

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.

Comment 38

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

Attachment: Patch v8

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

Comment 39

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

(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 40

[Mike Schroepfer]

Comment on attachment 288225 [details] [diff] [review]
Patch v8

blocking bugs don't need patch approvals at this time..

Comment 41

[Reed Loden [:reed]]

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

Comment 42

[Martijn Wargers (dead)]

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

Comment 43

[Henry Bridge]

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

Comment 44

[Martijn Wargers (dead)]

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

Comment 45

[Henry Bridge]

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).

Comment 46

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

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

Comment 47

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

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.

Comment 48

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

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.

Comment 49

[Martijn Wargers (dead)]

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.

Comment 50

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

(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.

Comment 51

[-fullmetaljacket-]

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

Comment 52

[Marcia Knous [:marcia]]

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.